From: Andrew Morton <akpm@linux-foundation.org>
To: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
Peter Zijlstra <peterz@infradead.org>,
Oleg Nesterov <oleg@redhat.com>, Rik van Riel <riel@redhat.com>,
Vladimir Davydov <vdavydov@parallels.com>,
Thomas Gleixner <tglx@linutronix.de>,
David Rientjes <rientjes@google.com>,
Kees Cook <keescook@chromium.org>,
linux-kernel@vger.kernel.org, Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH 1/1 v2] kernel/fork.c: avoid division by zero
Date: Tue, 17 Feb 2015 15:15:11 -0800 [thread overview]
Message-ID: <20150217151511.95143a25ff83c165f4199d08@linux-foundation.org> (raw)
In-Reply-To: <1424199698-7607-1-git-send-email-xypron.glpk@gmx.de>
On Tue, 17 Feb 2015 20:01:38 +0100 Heinrich Schuchardt <xypron.glpk@gmx.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.
>
> The futex implementation assumes that tids fit into the FUTEX_TID_MASK.
> This limits the number of allowable threads.
>
> ...
>
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -74,6 +74,7 @@
> #include <linux/uprobes.h>
> #include <linux/aio.h>
> #include <linux/compiler.h>
> +#include <linux/math64.h>
>
> #include <asm/pgtable.h>
> #include <asm/pgalloc.h>
> @@ -255,6 +256,8 @@ void __init __weak arch_task_cache_init(void) { }
>
> void __init fork_init(unsigned long mempages)
> {
> + u64 temp;
That's a really poor name. We should always try to make names
meaningful. Here, something like "threads" would be better.
> #ifndef CONFIG_ARCH_TASK_STRUCT_ALLOCATOR
> #ifndef ARCH_MIN_TASKALIGN
> #define ARCH_MIN_TASKALIGN L1_CACHE_BYTES
> @@ -273,7 +276,16 @@ void __init fork_init(unsigned long mempages)
> * value: the thread structures can take up at most half
> * of memory.
> */
> - max_threads = mempages / (8 * THREAD_SIZE / PAGE_SIZE);
> + temp = div64_u64((u64) mempages * (u64) PAGE_SIZE,
> + (u64) THREAD_SIZE * 8UL);
> +
> + /*
> + * The futex code assumes that tids fit into the FUTEX_TID_MASK.
> + */
> + if (temp < FUTEX_TID_MASK)
> + max_threads = temp;
> + else
> + max_threads = FUTEX_TID_MASK;
Seems rather complicated. How about
max_threads = mempages / (8 * THREAD_SIZE);
max_threads *= PAGE_SIZE;
max_threads = min(max_threads, FUTEX_TID_MASK);
And while we're there, I do think the comments need a refresh. What
does "the thread structures can take up at most half of memory" mean?
And what's the reasoning behind that "8"? I suggest we just delete all
that and make a new attempt at explaining why the code is this way.
next prev parent reply other threads:[~2015-02-17 23:15 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-17 19:01 Heinrich Schuchardt
2015-02-17 23:15 ` Andrew Morton [this message]
2015-02-18 19:38 ` Guenter Roeck
2015-02-18 19:50 ` Heinrich Schuchardt
2015-02-18 20:23 ` Guenter Roeck
2015-02-18 19:47 ` Heinrich Schuchardt
2015-02-18 20:28 ` Andrew Morton
2015-02-21 22:19 ` [PATCH 0/3 v3] kernel/fork.c max_thread handling Heinrich Schuchardt
2015-02-21 22:19 ` [PATCH 1/3 v3] kernel/fork.c: avoid division by zero Heinrich Schuchardt
2015-02-22 7:58 ` Ingo Molnar
2015-02-23 20:14 ` [PATCH 0/4 v4] max_threadx handling Heinrich Schuchardt
2015-02-23 20:14 ` [PATCH 1/4 v4] kernel/fork.c: new function for max_threads Heinrich Schuchardt
2015-02-23 20:14 ` [PATCH 2/4 v4] kernel/fork.c: avoid division by zero Heinrich Schuchardt
2015-02-23 21:10 ` Peter Zijlstra
2015-02-23 21:29 ` Heinrich Schuchardt
2015-02-24 7:35 ` Ingo Molnar
2015-02-23 20:14 ` [PATCH 3/4 v4] kernel/sysctl.c: threads-max observe limits Heinrich Schuchardt
2015-02-23 20:14 ` [PATCH 4/4 v4] kernel/fork.c: memory hotplug updates max_threads Heinrich Schuchardt
2015-02-23 20:50 ` Oleg Nesterov
2015-02-23 20:54 ` Oleg Nesterov
2015-02-23 21:11 ` Heinrich Schuchardt
2015-02-23 21:46 ` Oleg Nesterov
2015-02-24 19:38 ` [PATCH 0/3 v5] max_threadx handling Heinrich Schuchardt
2015-02-24 19:38 ` [PATCH 1/3 v5] kernel/fork.c: new function for max_threads Heinrich Schuchardt
2015-02-24 21:03 ` David Rientjes
2015-02-24 21:23 ` Heinrich Schuchardt
2015-02-24 22:16 ` David Rientjes
2015-02-25 7:21 ` Heinrich Schuchardt
2015-02-25 10:17 ` Ingo Molnar
2015-02-25 19:08 ` Heinrich Schuchardt
2015-02-25 21:07 ` David Rientjes
2015-02-24 19:38 ` [PATCH 2/3 v5] kernel/fork.c: avoid division by zero Heinrich Schuchardt
2015-02-24 21:14 ` David Rientjes
2015-02-24 19:38 ` [PATCH 3/3] kernel/sysctl.c: threads-max observe limits Heinrich Schuchardt
2015-02-24 21:17 ` David Rientjes
2015-02-24 21:31 ` Heinrich Schuchardt
2015-02-24 22:20 ` David Rientjes
2015-02-25 18:47 ` Heinrich Schuchardt
2015-02-25 20:47 ` David Rientjes
2015-02-21 22:19 ` [PATCH 2/3 v3] " Heinrich Schuchardt
2015-02-21 22:19 ` [PATCH 3/3 v3] kernel/fork.c: memory hotplug updates max_threads Heinrich Schuchardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150217151511.95143a25ff83c165f4199d08@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=keescook@chromium.org \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=oleg@redhat.com \
--cc=peterz@infradead.org \
--cc=riel@redhat.com \
--cc=rientjes@google.com \
--cc=tglx@linutronix.de \
--cc=vdavydov@parallels.com \
--cc=xypron.glpk@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®