On Wed, 13 Apr 2022 18:25:53 -0400 "Alex Xu (Hello71)" <alex_y_xu@xxxxxxxx> 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@xxxxxxxx> 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?
