Re: [PATCH] mm/smaps_rollup: return empty file for kthreads instead of ESRCH

[Alex Xu (Hello71)]

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 +++

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.

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.

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

Cheers,
Alex.