Messages in this thread | | | Date | Tue, 24 Feb 2015 13:14:32 -0800 (PST) | From | David Rientjes <> | Subject | Re: [PATCH 2/3 v5] kernel/fork.c: avoid division by zero |
| |
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 <xypron.glpk@gmx.de> > --- > 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 <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. */ > @@ -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)
| |