Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 13 Apr 2022 16:06:13 -0700
From:   Andrew Morton <akpm@linux-foundation.org>
To:     "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca>
Cc:     Alexey Dobriyan <adobriyan@gmail.com>,
        Daniel Colascione <dancol@google.com>,
        linux-api@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Vlastimil Babka <vbabka@suse.cz>
Subject: Re: [PATCH] mm/smaps_rollup: return empty file for kthreads instead
 of ESRCH
Message-Id: <20220413160613.385269bf45a9ebb2f7223ca8@linux-foundation.org>
In-Reply-To: <1649886492.rqei1nn3vm.none@localhost>
References: <20220413211357.26938-1-alex_y_xu.ref@yahoo.ca>
        <20220413211357.26938-1-alex_y_xu@yahoo.ca>
        <20220413142748.a5796e31e567a6205c850ae7@linux-foundation.org>
        <1649886492.rqei1nn3vm.none@localhost>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 13 Apr 2022 18:25:53 -0400 "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> wrote:

> Excerpts from Andrew Morton's message of April 13, 2022 5:27 pm:
> > On Wed, 13 Apr 2022 17:13:57 -0400 "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca> wrote:
> > 
> >> This restores the behavior prior to 258f669e7e88 ("mm:
> >> /proc/pid/smaps_rollup: convert to single value seq_file"), making it
> >> once again consistent with maps and smaps, and allowing patterns like
> >> awk '$1=="Anonymous:"{x+=$2}END{print x}' /proc/*/smaps_rollup to work.
> >> Searching all Debian packages for "smaps_rollup" did not find any
> >> programs which would be affected by this change.
> > 
> > Thanks.
> > 
> > 258f669e7e88 was 4 years ago, so I guess a -stable backport isn't
> > really needed.
> > 
> > However, we need to be concerned about causing new regressions, and I
> > don't think you've presented enough information for this to be determined.
> > 
> > So please provide us with a full description of how the smaps_rollup
> > output will be altered by this patch.  Quoting example output would be
> > helpful.
> > 
> > 
> 
> Current behavior (4.19+):
> 
> $ cat /proc/2/smaps; echo $?
> 0
> $ cat /proc/2/smaps_rollup; echo $?
> cat: /proc/2/smaps_rollup: No such process
> 1
> $ strace -yP /proc/2/smaps_rollup cat /proc/2/smaps_rollup
> openat(AT_FDCWD</>, "/proc/2/smaps_rollup", O_RDONLY) = 3</proc/2/smaps_rollup>
> newfstatat(3</proc/2/smaps_rollup>, "", {st_mode=S_IFREG|0444, st_size=0, ...}, AT_EMPTY_PATH) = 0
> fadvise64(3</proc/2/smaps_rollup>, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
> read(3</proc/2/smaps_rollup>, 0x7fa475f5d000, 131072) = -1 ESRCH (No such process)
> cat: /proc/2/smaps_rollup: No such process
> close(3</proc/2/smaps_rollup>)          = 0
> +++ exited with 1 +++
> 
> Pre-4.19 and post-patch behavior:
> 
> $ cat /proc/2/smaps; echo $?
> 0
> $ cat /proc/2/smaps_rollup; echo $?
> 0
> $ strace -yP /proc/2/smaps_rollup cat /proc/2/smaps_rollup
> openat(AT_FDCWD</>, "/proc/2/smaps_rollup", O_RDONLY) = 3</proc/2/smaps_rollup>
> newfstatat(3</proc/2/smaps_rollup>, "", {st_mode=S_IFREG|0444, st_size=0, ...}, AT_EMPTY_PATH) = 0
> fadvise64(3</proc/2/smaps_rollup>, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
> read(3</proc/2/smaps_rollup>, "", 131072) = 0
> close(3</proc/2/smaps_rollup>)          = 0
> +++ exited with 0 +++

OK, thanks.

But the current behaviour is appropriate, isn't it?  An attempt to read
the maps of a process which has no maps returns -ESRCH.  Seems sensible
enough.

On the other hand, returning a zero-length read() is also appropriate.

> I agree that this type of change must be done carefully to avoid 
> introducing inadvertent regressions. However, I think this particular 
> change is highly unlikely to introduce regressions for the following 
> reasons:
> 
> 1. I cannot think of a plausible case which would be affected. The only 
>    case I can possibly imagine is a program checking whether a process 
>    is a kernel thread, but this seems like a particularly silly method. 
>    Moreover, the method is already broken on kernels before 4.14 
>    (because smaps_rollup does not exist) and before 4.19 (because 
>    smaps_rollup worked like smaps). A plausible method would be opening 
>    /proc/x/(s)maps and checking that it is empty, which some programs 
>    actually do.

Well, I suppose a poorly coded application could do something like

	if (read(fd, buf, 1000) >= 0)
		assume_buf_now_contains_data()

> 2. Research on Debian Code Search did not find any apparent cases. I also 
>    searched GitHub Code Search but found too many irrelevant results with 
>    no useful way to filter them out.

I don't think this will work very well.  smaps_rollup is the sort of
system tuning thing for which organizations will develop in-house
tooling which never get relesaed externally.

> 3. As mentioned previously, this was already the behavior between 4.14 
>    and 4.18 (inclusive).
> 

Yup.  Hm, tricky.  I'd prefer to leave it alone if possible.  How
serious a problem is this, really?