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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87mt3m7ynz.fsf@email.froward.int.ebiederm.org>
Date:   Wed, 05 Apr 2023 17:27:12 -0500
From:   "Eric W. Biederman" <ebiederm@...ssion.com>
To:     Josh Triplett <josh@...htriplett.org>
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Joey Gouly <joey.gouly@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alexey Gladkov <legion@...nel.org>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] sysinfo: Saturate 16-bit procs rather than wrapping

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