linux-kernel - Re: [PATCH 1/3 v5] kernel/fork.c: new function for max

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: <54EE1DA8.7030504@gmx.de>
Date:	Wed, 25 Feb 2015 20:08:24 +0100
From:	Heinrich Schuchardt <xypron.glpk@....de>
To:	Ingo Molnar <mingo@...nel.org>,
	David Rientjes <rientjes@...gle.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Aaron Tomlin <atomlin@...hat.com>,
	Andy Lutomirski <luto@...capital.net>,
	Davidlohr Bueso <dave@...olabs.net>,
	"David S. Miller" <davem@...emloft.net>,
	Fabian Frederick <fabf@...net.be>,
	Guenter Roeck <linux@...ck-us.net>,
	"H. Peter Anvin" <hpa@...or.com>, Jens Axboe <axboe@...com>,
	Joe Perches <joe@...ches.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Kees Cook <keescook@...omium.org>,
	Michael Marineau <mike@...ineau.org>,
	Oleg Nesterov <oleg@...hat.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Prarit Bhargava <prarit@...hat.com>,
	Rik van Riel <riel@...hat.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Vladimir Davydov <vdavydov@...allels.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3 v5] kernel/fork.c: new function for max_threads

On 25.02.2015 11:17, Ingo Molnar wrote:
> 
> * David Rientjes <rientjes@...gle.com> wrote:
> 
>> The problem is with the structure of your patchset.  You 
>> want three patches.  There's one bugfix patch, a 
>> preparation patch, and a feature patch.  The bugfix patch 
>> should come first so that it can be applied, possibly, to 
>> stable kernels and doesn't depend on unnecessary 
>> preparation patches for features.
>>
>> 1/3: change the implementation of fork_init(), with 
>> commentary, to avoid the divide by zero on certain 
>> arches, enforce the limits, and deal with variable types 
>> to prevent overflow.  This is the most urgent patch and 
>> fixes a bug.
>>
>> 2/3: simply extract the fixed fork_init() implementation 
>> into a new set_max_threads() in preparation to use it for 
>> threads-max, (hint: UINT_MAX and ignored arguments should 
>> not appear in this patch),
>>
>> 3/3: use the new set_max_threads() implementation for 
>> threads-max with an update to the documentation.
> 
> I disagree strongly: it's better to first do low-risk 
> cleanups, then do any fix and other changes.
> 
> This approach has four advantages:
> 
>   - it makes the bug fix more apparent, in the context of 
>     an already cleaned up code.
> 
>   - it strengthens the basic principle that 'substantial 
>     work should be done on clean code'.
> 
>   - on the off chance that the bugfix introduces problems 
>     _upstream_, it's much easier to revert in a late -rc 
>     kernel, than to first revert the cleanups. This 
>     happens in practice occasionally, so it's not a
>     theoretical concern.
> 
>   - the _backport_ to the -stable kernel will be more 
>     robust as well, because under your proposed structure, 
>     what gets tested upstream is the fix+cleanup, while the
>     backport will only be the fix, which does not get 
>     tested by upstream in isolation. If there's any 
>     (unintended) side effect of the cleanup that happens to
>     be an improvement, then we'll break -stable!
> 
> It is true that this makes backports a tiny bit more 
> involved (2 cherry-picks instead of just one), but -stable 
> kernels can backport preparatory patches just fine, and 
> it's actually an advantage to have -stable kernel code 
> match the upstream version as much as possible.
> 
> Thanks,
> 
> 	Ingo
> --

Hello David,

to my understanding the biggest no-go for you was that patch 1/3
introduces a function argument that is not needed until patch 3/3.

Could you accept the sequence proposed by Ingo if I leave away the
argument max_threads_suggested in patch 1 and 2 and only introduce it in
patch 3?

So patch 1/4 will refactor the code that may result in division by zero
to new function static void set_max_threads(void). Furthermore it will
change the interface of fork_init to void __init fork_init(void).

Patch 2/4 keeps the interface static void set_max_threads(void) but
corrects the coding using div64_u64.

Patch 3/4 changes the interface to
static void set_max_threads(unsigned int max_threads_suggested),
calls this function in fork_init with value UINT_MAX and calls it
when threads-max is set with the user value.

Patch 4/4 adds the necessary information about theads-max in
Documentation/sysctl/kernel.txt.
I would not like to put this in patch 3/4 as Jonathan Corbet uses a
separate git tree.

Best regards

Heinrich
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/