Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752861AbbBXVOi (ORCPT ); Tue, 24 Feb 2015 16:14:38 -0500 Received: from mail-ie0-f177.google.com ([209.85.223.177]:42685 "EHLO mail-ie0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752309AbbBXVOg (ORCPT ); Tue, 24 Feb 2015 16:14:36 -0500 Date: Tue, 24 Feb 2015 13:14:32 -0800 (PST) From: David Rientjes X-X-Sender: rientjes@chino.kir.corp.google.com To: Heinrich Schuchardt cc: Andrew Morton , Aaron Tomlin , Andy Lutomirski , Davidlohr Bueso , "David S. Miller" , Fabian Frederick , Guenter Roeck , "H. Peter Anvin" , Ingo Molnar , Jens Axboe , Joe Perches , Johannes Weiner , Kees Cook , Michael Marineau , Oleg Nesterov , "Paul E. McKenney" , Peter Zijlstra , Prarit Bhargava , Rik van Riel , Rusty Russell , Steven Rostedt , Thomas Gleixner , Vladimir Davydov , linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/3 v5] kernel/fork.c: avoid division by zero In-Reply-To: <1424806701-30099-3-git-send-email-xypron.glpk@gmx.de> Message-ID: References: <1424722477-23758-1-git-send-email-xypron.glpk@gmx.de> <1424806701-30099-1-git-send-email-xypron.glpk@gmx.de> <1424806701-30099-3-git-send-email-xypron.glpk@gmx.de> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3371 Lines: 105 On Tue, 24 Feb 2015, Heinrich Schuchardt 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. > This should only appear in one patch in the series, and I think this is the appropriate patch for that to happen. > 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 > --- > kernel/fork.c | 38 +++++++++++++++++++++++++++----------- > 1 file changed, 27 insertions(+), 11 deletions(-) > > diff --git a/kernel/fork.c b/kernel/fork.c > index 460b044..880c78d 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -88,6 +88,16 @@ > #include > > /* > + * 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. */ > @@ -254,23 +264,29 @@ EXPORT_SYMBOL_GPL(__put_task_struct); > void __init __weak arch_task_cache_init(void) { } > > /* > - * set_max_threads > - * The argument is ignored. > + * set_max_threads tries to set the default limit to the suggested value. I'm not sure that is true. At this point in the patch series, set_max_threads() is only called with UINT_MAX and that's not what the implementation tries to set max_threads to. > */ > static void set_max_threads(unsigned int max_threads_suggested) > { > - /* > - * 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; > + threads = div64_u64((u64) totalram_pages * (u64) PAGE_SIZE, > + (u64) THREAD_SIZE * 8UL); The artithmetic works here, but I'm wondering what guarantee is in place to ensure that totalram_pages * PAGE_SIZE does not overflow on 64-bit arches? > + > + if (threads > max_threads_suggested) > + threads = max_threads_suggested; Ok, so now the function argument just shows an implementation issue. You're capping threads at UINT_MAX because you need max_threads to be an int and relying on the caller to enforce that. set_max_threads() should be handling all of these details itself. > + > + if (threads > MAX_THREADS) > + threads = MAX_THREADS; > + > + if (threads < MIN_THREADS) > + threads = MIN_THREADS; > + > + max_threads = (int) threads; > } > > void __init fork_init(void) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/