* [PATCH] random: Fix crashes with sparse node ids @ 2016-07-30 14:23 Michael Ellerman 2016-07-30 20:27 ` Linus Torvalds 0 siblings, 1 reply; 4+ messages in thread From: Michael Ellerman @ 2016-07-30 14:23 UTC (permalink / raw) To: Linus Torvalds; +Cc: tytso, linux-kernel, linuxppc-dev On a system with sparse node ids, eg. a powerpc system with 4 nodes numbered like so: node 0: [mem 0x0000000000000000-0x00000007ffffffff] node 1: [mem 0x0000000800000000-0x0000000fffffffff] node 16: [mem 0x0000001000000000-0x00000017ffffffff] node 17: [mem 0x0000001800000000-0x0000001fffffffff] The code in rand_initialize() will allocate 4 pointers for the pool array, and initialise them correctly. However when go to use the pool, in eg. extract_crng(), we use the numa_node_id() to index into the array. For the higher numbered node ids this leads to random memory corruption, depending on what was kmalloc'ed adjacent to the pool array. Fix it by using nr_node_ids to size the pool array. Fixes: 1e7f583af67b ("random: make /dev/urandom scalable for silly userspace programs") Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- drivers/char/random.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/char/random.c b/drivers/char/random.c index 7f0622426b97..ea03dfe2f21c 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -249,6 +249,7 @@ #include <linux/genhd.h> #include <linux/interrupt.h> #include <linux/mm.h> +#include <linux/nodemask.h> #include <linux/spinlock.h> #include <linux/kthread.h> #include <linux/percpu.h> @@ -1656,7 +1657,6 @@ static int rand_initialize(void) { #ifdef CONFIG_NUMA int i; - int num_nodes = num_possible_nodes(); struct crng_state *crng; struct crng_state **pool; #endif @@ -1666,7 +1666,7 @@ static int rand_initialize(void) crng_initialize(&primary_crng); #ifdef CONFIG_NUMA - pool = kmalloc(num_nodes * sizeof(void *), + pool = kmalloc(nr_node_ids * sizeof(void *), GFP_KERNEL|__GFP_NOFAIL|__GFP_ZERO); for_each_online_node(i) { crng = kmalloc_node(sizeof(struct crng_state), -- 2.7.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] random: Fix crashes with sparse node ids 2016-07-30 14:23 [PATCH] random: Fix crashes with sparse node ids Michael Ellerman @ 2016-07-30 20:27 ` Linus Torvalds 2016-07-31 3:27 ` Michael Ellerman 0 siblings, 1 reply; 4+ messages in thread From: Linus Torvalds @ 2016-07-30 20:27 UTC (permalink / raw) To: Michael Ellerman Cc: Theodore Ts'o, Linux Kernel Mailing List, linuxppc-dev list On Sat, Jul 30, 2016 at 7:23 AM, Michael Ellerman <mpe@ellerman.id.au> wrote: > #ifdef CONFIG_NUMA > - pool = kmalloc(num_nodes * sizeof(void *), > + pool = kmalloc(nr_node_ids * sizeof(void *), > GFP_KERNEL|__GFP_NOFAIL|__GFP_ZERO); > for_each_online_node(i) { > crng = kmalloc_node(sizeof(struct crng_state), Ugh. Can we please also just change that kmalloc to kcalloc()? Get rid of the odd multiplication and the unusual GFP mask bit crud? And instead of using "sizeof(void *)", just use the pool entry size, ie "sizeof(*pool)". Yes, we have other places where we depend on void pointers having the same size as others, but it's the RightThing(tm) to do anyway, and it makes more sense when you grep things ("Oh, we're allocating 'nr_node_id' copes of *pool entries" even without knowing what type is behind the "pool" pointer). IOW, can you confirm that you could just use pool = kcalloc(nr_node_ids, sizeof(*pool), GFP_KERNEL); instead? I'd much rather apply that patch. Linus ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] random: Fix crashes with sparse node ids 2016-07-30 20:27 ` Linus Torvalds @ 2016-07-31 3:27 ` Michael Ellerman 2016-07-31 3:56 ` Linus Torvalds 0 siblings, 1 reply; 4+ messages in thread From: Michael Ellerman @ 2016-07-31 3:27 UTC (permalink / raw) To: Linus Torvalds Cc: Theodore Ts'o, Linux Kernel Mailing List, linuxppc-dev list Linus Torvalds <torvalds@linux-foundation.org> writes: > On Sat, Jul 30, 2016 at 7:23 AM, Michael Ellerman <mpe@ellerman.id.au> wrote: >> #ifdef CONFIG_NUMA >> - pool = kmalloc(num_nodes * sizeof(void *), >> + pool = kmalloc(nr_node_ids * sizeof(void *), >> GFP_KERNEL|__GFP_NOFAIL|__GFP_ZERO); >> for_each_online_node(i) { >> crng = kmalloc_node(sizeof(struct crng_state), > > Ugh. Can we please also just change that kmalloc to kcalloc()? Get rid > of the odd multiplication and the unusual GFP mask bit crud? > > And instead of using "sizeof(void *)", just use the pool entry size, > ie "sizeof(*pool)". Yes, we have other places where we depend on void > pointers having the same size as others, but it's the RightThing(tm) > to do anyway, and it makes more sense when you grep things ("Oh, we're > allocating 'nr_node_id' copes of *pool entries" even without knowing > what type is behind the "pool" pointer). > > IOW, can you confirm that you could just use > > pool = kcalloc(nr_node_ids, sizeof(*pool), GFP_KERNEL); > > instead? I'd much rather apply that patch. Dropping NOFAIL means we need to handle allocation failures, which makes the patch a bit bigger, and less of a pure fix. Here's a separate patch to do those cleanups, which you can squash with the first if you prefer. I did test the allocation failure case for both the whole pool and individual nodes. cheers >From 2f5ab5fa2b9e4997fe01053fc12f689fb2117f45 Mon Sep 17 00:00:00 2001 From: Michael Ellerman <mpe@ellerman.id.au> Date: Sun, 31 Jul 2016 12:54:40 +1000 Subject: [PATCH] random: Clean up NUMA allocations Use kcalloc(), rather than doing the multiply by hand. Use sizeof(*pool) rather than assuming it's == sizeof(void *). kcalloc() zeroes by default so we don't need __GFP_ZERO. Drop the __GFP_NOFAILs, we can easily handle allocation failures, the code is already written to cope with a NULL crng_node_pool, or a NULL entry for a given node in the pool. Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- drivers/char/random.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/char/random.c b/drivers/char/random.c index ea03dfe2f21c..22c8ac173666 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -1666,11 +1666,15 @@ static int rand_initialize(void) crng_initialize(&primary_crng); #ifdef CONFIG_NUMA - pool = kmalloc(nr_node_ids * sizeof(void *), - GFP_KERNEL|__GFP_NOFAIL|__GFP_ZERO); + pool = kcalloc(nr_node_ids, sizeof(*pool), GFP_KERNEL); + if (!pool) + return 0; + for_each_online_node(i) { - crng = kmalloc_node(sizeof(struct crng_state), - GFP_KERNEL | __GFP_NOFAIL, i); + crng = kmalloc_node(sizeof(struct crng_state), GFP_KERNEL, i); + if (!crng) + continue; + spin_lock_init(&crng->lock); crng_initialize(crng); pool[i] = crng; -- 2.7.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] random: Fix crashes with sparse node ids 2016-07-31 3:27 ` Michael Ellerman @ 2016-07-31 3:56 ` Linus Torvalds 0 siblings, 0 replies; 4+ messages in thread From: Linus Torvalds @ 2016-07-31 3:56 UTC (permalink / raw) To: Michael Ellerman Cc: Theodore Ts'o, Linux Kernel Mailing List, linuxppc-dev list On Sat, Jul 30, 2016 at 8:27 PM, Michael Ellerman <mpe@ellerman.id.au> wrote: > > Dropping NOFAIL means we need to handle allocation failures, which makes > the patch a bit bigger, and less of a pure fix. Hmm. If you get allocation failures for something like this at init time, I think you're basically screwed anyway. And I really only meant for the initial array allocation, and the __GFP_ZERO thing. Yes, __GFP_ZERO does work for kmalloc() too, but unlike the other GPF flags, we do have special zalloc versions for zeroing that are generally preferred. Linus ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-07-31 3:56 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-30 14:23 [PATCH] random: Fix crashes with sparse node ids Michael Ellerman 2016-07-30 20:27 ` Linus Torvalds 2016-07-31 3:27 ` Michael Ellerman 2016-07-31 3:56 ` Linus Torvalds
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).