| Message ID | 1326450942.2272.20.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Le vendredi 13 janvier 2012 à 11:35 +0100, Eric Dumazet a écrit : > I wonder ... > > With 4096 cpus, do we really want to reserve 5242880 bytes of memory for > this function ? > > What about following patch instead ? > > (Trying a dynamic memory allocation, and fallback on a single > pre-allocated bloc of memory, shared by all cpus, protected by a > spinlock) > > diff --git a/crypto/sha512_generic.c b/crypto/sha512_generic.c > index 9ed9f60..5c80a76 100644 > --- a/crypto/sha512_generic.c > +++ b/crypto/sha512_generic.c > @@ -21,7 +21,6 @@ > #include <linux/percpu.h> > #include <asm/byteorder.h> > > -static DEFINE_PER_CPU(u64[80], msg_schedule); > > static inline u64 Ch(u64 x, u64 y, u64 z) > { > @@ -87,10 +86,16 @@ static void > sha512_transform(u64 *state, const u8 *input) > { > u64 a, b, c, d, e, f, g, h, t1, t2; > - > + static u64 msg_schedule[80]; > + static DEFINE_SPINLOCK(msg_schedule_lock); > int i; > - u64 *W = get_cpu_var(msg_schedule); > + u64 *W = kzalloc(sizeof(msg_schedule), GFP_ATOMIC | __GFP_NOWARN); > And a plain kmalloc() is enough, since we fully initialize the array a bit later. for (i = 0; i < 16; i++) LOAD_OP(i, W, input); for (i = 16; i < 80; i++) { BLEND_OP(i, W); } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 13, 2012 at 11:35:42AM +0100, Eric Dumazet wrote: > Le vendredi 13 janvier 2012 à 18:08 +1100, Herbert Xu a écrit : > > On Fri, Jan 13, 2012 at 02:55:14AM +0300, Alexey Dobriyan wrote: > > > > > > Herbert, I couldn't come up with a single scenario. :-( > > > But the bug is easy to reproduce. > > > > OK, does this patch work for you? > > > > commit 31f4e55c09c1170f8b813c14b1299b70f50db414 > > Author: Herbert Xu <herbert@gondor.apana.org.au> > > Date: Fri Jan 13 18:06:50 2012 +1100 > > > > crypto: sha512 - Fix msg_schedule race > > > > The percpu msg_schedule setup was unsafe as a user in a process > > context can be interrupted by a softirq user which would then > > scribble over the exact same work area. This was discovered by > > Steffen Klassert. > > > > This patch based on ideas from Eric Dumazet fixes this by using > > two independent work areas. > > > > Reported-by: Alexey Dobriyan <adobriyan@gmail.com> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > > > I wonder ... > > With 4096 cpus, do we really want to reserve 5242880 bytes of memory for > this function ? > > What about following patch instead ? > > (Trying a dynamic memory allocation, and fallback on a single > pre-allocated bloc of memory, shared by all cpus, protected by a > spinlock) If we want to do dynamic memory allocation, we could place it to the shash_desc context where we already store the intermediate digest value. This is preallocated anyway, so we don't need to do another allocation. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Trying a dynamic memory allocation, and fallback on a single > pre-allocated bloc of memory, shared by all cpus, protected by a > spinlock ... > - > + static u64 msg_schedule[80]; > + static DEFINE_SPINLOCK(msg_schedule_lock); > int i; > - u64 *W = get_cpu_var(msg_schedule); > + u64 *W = kzalloc(sizeof(msg_schedule), GFP_ATOMIC | __GFP_NOWARN); > > + > + if (!W) { > + spin_lock_bh(&msg_schedule_lock); > + W = msg_schedule; > + } If this code can be called from an ISR is the kmalloc() call safe? If the above is safe, wouldn't it be better to: 1) try to use the static buffer 2) try to kalloc() a buffer 3) spinwait for the static buffer to be free David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le vendredi 13 janvier 2012 à 11:45 +0000, David Laight a écrit : > > Trying a dynamic memory allocation, and fallback on a single > > pre-allocated bloc of memory, shared by all cpus, protected by a > > spinlock > ... > > - > > + static u64 msg_schedule[80]; > > + static DEFINE_SPINLOCK(msg_schedule_lock); > > int i; > > - u64 *W = get_cpu_var(msg_schedule); > > + u64 *W = kzalloc(sizeof(msg_schedule), GFP_ATOMIC | > __GFP_NOWARN); > > > > + > > + if (!W) { > > + spin_lock_bh(&msg_schedule_lock); > > + W = msg_schedule; > > + } > > If this code can be called from an ISR is the kmalloc() > call safe? > Yes, obviously, kmalloc() is IRQ safe. > If the above is safe, wouldn't it be better to: > 1) try to use the static buffer > 2) try to kalloc() a buffer > 3) spinwait for the static buffer to be free > No idea of what you mean, and why you think its better. kmalloc() propably can give us a block already hot in cpu cache. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 13, 2012 at 12:02:55PM +0100, Steffen Klassert wrote: > > If we want to do dynamic memory allocation, we could place it to the > shash_desc context where we already store the intermediate digest value. > This is preallocated anyway, so we don't need to do another allocation. The problem with that is that some call paths place shash_desc on the stack too. Cheers,
diff --git a/crypto/sha512_generic.c b/crypto/sha512_generic.c index 9ed9f60..5c80a76 100644 --- a/crypto/sha512_generic.c +++ b/crypto/sha512_generic.c @@ -21,7 +21,6 @@ #include <linux/percpu.h> #include <asm/byteorder.h> -static DEFINE_PER_CPU(u64[80], msg_schedule); static inline u64 Ch(u64 x, u64 y, u64 z) { @@ -87,10 +86,16 @@ static void sha512_transform(u64 *state, const u8 *input) { u64 a, b, c, d, e, f, g, h, t1, t2; - + static u64 msg_schedule[80]; + static DEFINE_SPINLOCK(msg_schedule_lock); int i; - u64 *W = get_cpu_var(msg_schedule); + u64 *W = kzalloc(sizeof(msg_schedule), GFP_ATOMIC | __GFP_NOWARN); + + if (!W) { + spin_lock_bh(&msg_schedule_lock); + W = msg_schedule; + } /* load the input */ for (i = 0; i < 16; i++) LOAD_OP(i, W, input); @@ -128,8 +133,11 @@ sha512_transform(u64 *state, const u8 *input) /* erase our data */ a = b = c = d = e = f = g = h = t1 = t2 = 0; - memset(W, 0, sizeof(__get_cpu_var(msg_schedule))); - put_cpu_var(msg_schedule); + memset(W, 0, sizeof(msg_schedule)); + if (W == msg_schedule) + spin_unlock_bh(&msg_schedule_lock); + else + kfree(W); } static int