linux-kernel - Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero

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: <55087E40.1020800@gmx.de>
Date:	Tue, 17 Mar 2015 20:19:28 +0100
From:	Heinrich Schuchardt <xypron.glpk@....de>
To:	Ingo Molnar <mingo@...nel.org>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Aaron Tomlin <atomlin@...hat.com>,
	Andy Lutomirski <luto@...capital.net>,
	Davidlohr Bueso <dave@...olabs.net>,
	David Rientjes <rientjes@...gle.com>,
	"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>,
	Jonathan Corbet <corbet@....net>,
	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, linux-doc@...r.kernel.org
Subject: Re: [PATCH 2/4 v6] kernel/fork.c: avoid division by zero

On 16.03.2015 08:41, Ingo Molnar wrote:
> 
> * Heinrich Schuchardt <xypron.glpk@....de> wrote:
> 
>> PAGE_SIZE is not guaranteed to be equal to or less than 8 times the
>> THREAD_SIZE.
>>
>> E.g. architecture hexagon may have page size 1M and thread size 4096.
>> This would lead to a division by zero in the calculation of max_threads.
>>
>> With 32-bit calculation there is no solution which delivers valid results
>> for all possible combinations of the parameters.
>> The code is only called once.
>> Hence a 64-bit calculation can be used as solution.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@....de>
>> ---
>>  kernel/fork.c | 35 ++++++++++++++++++++++++++---------
>>  1 file changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index bf1ff00..69ff08f 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -88,6 +88,16 @@
>>  #include <trace/events/task.h>
>>  
>>  /*
>> + * Minimum number of threads to boot the kernel
>> + */
>> +#define MIN_THREADS 20
>> +
>> +/*
>> + * Maximum number of threads
>> + */
>> +#define MAX_THREADS FUTEX_TID_MASK
>> +
>> +/*
>>   * Protected counters by write_lock_irq(&tasklist_lock)
>>   */
>>  unsigned long total_forks;	/* Handle normal Linux uptimes. */
>> @@ -258,18 +268,25 @@ void __init __weak arch_task_cache_init(void) { }
>>   */
>>  static void set_max_threads(void)
>>  {
>> -	/*
>> -	 * The default maximum number of threads is set to a safe
>> -	 * value: the thread structures can take up at most half
>> -	 * of memory.
>> -	 */
>> -	max_threads = totalram_pages / (8 * THREAD_SIZE / PAGE_SIZE);
>> +	u64 threads;
>>  
>>  	/*
>> -	 * we need to allow at least 20 threads to boot a system
>> +	 * The number of threads shall be limited such that the thread
>> +	 * structures may only consume a small part of the available memory.
>>  	 */
>> -	if (max_threads < 20)
>> -		max_threads = 20;
>> +	if (fls64(totalram_pages) + fls64(PAGE_SIZE) > 64)
>> +		threads = MAX_THREADS;
>> +	else
>> +		threads = div64_u64((u64) totalram_pages * (u64) PAGE_SIZE,
>> +				    (u64) THREAD_SIZE * 8UL);
>> +
>> +	if (threads > MAX_THREADS)
>> +		threads = MAX_THREADS;
>> +
>> +	if (threads < MIN_THREADS)
>> +		threads = MIN_THREADS;
>> +
>> +	max_threads = (int) threads;
>>  }
> 
> So why does this patch do two things:
> 
> 	- parametrizes set_max_threads() via defines
> 	- fixes a bug
> 
> ?
> 
> Those two things should be done in two separate patches, first the 
> introduction of parameters, then the fixing of the bug.
> 
> I suggested this in my first review: separate out and keep the fix 
> portion of the series minimal.

Hello Ingo,

you requested me in you first review to separate the move to a separate
function and the code fix. That was already done in a previous version
of the patch.

With this patch version set_max_threads does not have any parameters
all. A parameter is introduced in a later patch. It is not needed before.

Maybe you wanted to refer to constants?

Introduction of constant MAX_THREADS before fixing the bug does not make
any sense because the problematic code moved to set_max_threads with the
division by zero bug would not use it. Introducing it in a later patch
does not make sense because checking for conversion overflow when
converting u64 to u32 is necessary.

Splitting of patches is advisable if we assume that on some releases
only part of the patch series is used. This is not applicable to this patch.

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/