Re: [PATCH] sysinfo: Saturate 16-bit procs rather than wrapping

[Eric W. Biederman]

Josh Triplett <josh@xxxxxxxxxxxxxxxx> writes:

> struct sysinfo has a 16-bit field for the number of processes. Current
> systems can easily exceed this. Rather than wrapping around, saturate
> the value at U16_MAX. This is still incorrect, but more likely to
> help the user know what's going on; a caller can then (for instance)
> parse the full value out of /proc/loadavg.
>
> Signed-off-by: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> ---
>
> Not sure what tree changes to kernel/sys.c should flow through. Andrew,
> could you take this through your tree (assuming you agree with it), or
> suggest what tree it should go through instead?


Mind if I ask what the motivation for this is?

I looked at debian code search and there are a lot of uses of the
sysinfo system call.  Most of the uses were for load average or memory
occupancy.  The only use of procs that I could find was in samba.  I did
not trace the code far enough but it clearly had an embedded assumption
that 16 bits was enough to report the number of processes on a linux
system.

I looked at glibc and if I read things correctly the sysinfo system
call is just a pass through to the kernel.


I looked because just saturating the 16bit field feels like a hack
that will continue to encourage buggy programs to stay buggy.

If there is real value in sysinfo returning a this information someone
could go through the work and update the kernel to return the high
bits of the process count in info->pad that is immediately after
info->procs, and then update the apps or libc to find those high bits.

Otherwise I think it makes most sense to encourage programs to
use /proc/loadavg, where this information has always been returned
correctly as it is a text file.  We could do it like:

	/*
	 * Reliably fail when there are more than 64k processes.
         * Userspace should use /proc/loadavg instead.
         */
	info->procs = (nr_threads <= U16_MAX) ? nr_threads : 0;

If saturating does make sense can we please have a comment documenting
why saturating and encouraging confused userspace programs to stay
confused makes sense?


Eric


> diff --git a/kernel/sys.c b/kernel/sys.c
> index 495cd87d9bf4..ba05fca26927 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2699,7 +2699,7 @@ static int do_sysinfo(struct sysinfo *info)
>  
>  	get_avenrun(info->loads, 0, SI_LOAD_SHIFT - FSHIFT);
>  
> -	info->procs = nr_threads;
> +	info->procs = min_t(typeof(nr_threads), nr_threads, U16_MAX);
>  
>  	si_meminfo(info);
>  	si_swapinfo(info);