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 for Android: free password hash cracker in your pocket

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

Message-ID: <20150225101751.GB7134@gmail.com>
Date:	Wed, 25 Feb 2015 11:17:51 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	David Rientjes <rientjes@...gle.com>
Cc:	Heinrich Schuchardt <xypron.glpk@....de>,
	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


* 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
--
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/