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

[Heinrich Schuchardt]

On 16.03.2015 08:41, Ingo Molnar wrote:
> 
> * Heinrich Schuchardt <xypron.glpk@xxxxxx> 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@xxxxxx>
>> ---
>>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/