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;
From:   "Eric W. Biederman" <ebiederm@xmission.com>
To:     Josh Triplett <josh@joshtriplett.org>
Cc:     linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Joey Gouly <joey.gouly@arm.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alexey Gladkov <legion@kernel.org>,
        "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Mark Brown <broonie@kernel.org>
In-Reply-To: <e32ea9a03d0797ce2b8e7a82ed59c0dad9431f2b.1680407255.git.josh@joshtriplett.org>
        (Josh Triplett's message of "Sun, 2 Apr 2023 12:57:29 +0900")
References: <e32ea9a03d0797ce2b8e7a82ed59c0dad9431f2b.1680407255.git.josh@joshtriplett.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
Date:   Wed, 05 Apr 2023 17:27:12 -0500
Message-ID: <87mt3m7ynz.fsf@email.froward.int.ebiederm.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] sysinfo: Saturate 16-bit procs rather than wrapping
Precedence: bulk

Josh Triplett <josh@joshtriplett.org> 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@joshtriplett.org>
> ---
>
> 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);