linux-kernel - Re: [PATCH] random: Fix crashes with sparse node ids

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87y44io5be.fsf@concordia.ellerman.id.au>
Date:	Sun, 31 Jul 2016 13:27:17 +1000
From:	Michael Ellerman <mpe@...erman.id.au>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Theodore Ts'o <tytso@....edu>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linuxppc-dev list <linuxppc-dev@...abs.org>
Subject: Re: [PATCH] random: Fix crashes with sparse node ids

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Sat, Jul 30, 2016 at 7:23 AM, Michael Ellerman <mpe@...erman.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@...erman.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@...erman.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