SLOB breaks Crypto
in [Kernel]
|
Prev: [RELEASE] LTTng 0.214 for kernel 2.6.33.4
Next: vlynq: make whole Kconfig-menu dependant on architecture
From: David Woodhouse on 18 May 2010 19:30 On Tue, 2010-05-18 at 14:20 -0700, David Miller wrote: > I think it does make sense to expect that, whatever my architecture > defines or does not define, I can expect the allocators to provide the > same minimum alignment guarentee. In a sense, they do. The minimum alignment guarantee is sizeof(long). You are guaranteed that no allocator will return memory blocks which are aligned to anything less than that. Some allocators may sometimes return memory blocks which are better-aligned than that -- but that's not guaranteed behaviour. If you _depend_ on that behaviour and it happens to vary with the phase of the moon, that's your problem. It would be better if the minimum alignment was exposed to generic code though -- you're right that the CPP tests in linux/crypto.h really shouldn't have to exist. If it wasn't for that, then the crypto problem wouldn't have occurred. -- David Woodhouse Open Source Technology Centre David.Woodhouse(a)intel.com Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Herbert Xu on 18 May 2010 21:10 On Wed, May 19, 2010 at 12:20:34AM +0100, David Woodhouse wrote: > > It would be better if the minimum alignment was exposed to generic code > though -- you're right that the CPP tests in linux/crypto.h really > shouldn't have to exist. If it wasn't for that, then the crypto problem > wouldn't have occurred. While this problem wouldn't have occurred, we would instead have data corruption/alignment faults on architectures such as sparc32 or ARM that require 64-bit alignment for 64-bit objects. So no getting rid of them isn't going to fix things either. Of course I have no objections to moving this into slab.h or a similar location should anyone be willing to do the hard work. Thanks, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert(a)gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Herbert Xu on 18 May 2010 22:00 On Tue, May 18, 2010 at 09:32:54PM +0200, Adrian-Ken Rueegsegger wrote: > > When doing the revert it is necessary to either have > ARCH_KMALLOC_MINALIGN defined or explicitly define CRYPTO_MINALIGN in > the case where it is not. Otherwise shash compilation fails because it > needs CRYPTO_MINALIGN. I've added the following patch. Thanks! commit 18eb8ea6ee4cc9ed39b45f95b734f523bcfb586b Author: Herbert Xu <herbert(a)gondor.apana.org.au> Date: Wed May 19 11:50:58 2010 +1000 crypto: shash - Remove usage of CRYPTO_MINALIGN The macro CRYPTO_MINALIGN is not meant to be used directly. This patch replaces it with crypto_tfm_ctx_alignment. Signed-off-by: Herbert Xu <herbert(a)gondor.apana.org.au> diff --git a/crypto/shash.c b/crypto/shash.c index 91f7b9d..22fd943 100644 --- a/crypto/shash.c +++ b/crypto/shash.c @@ -37,7 +37,7 @@ static int shash_setkey_unaligned(struct crypto_shash *tfm, const u8 *key, u8 *buffer, *alignbuffer; int err; - absize = keylen + (alignmask & ~(CRYPTO_MINALIGN - 1)); + absize = keylen + (alignmask & ~(crypto_tfm_ctx_alignment() - 1)); buffer = kmalloc(absize, GFP_KERNEL); if (!buffer) return -ENOMEM; -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert(a)gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Herbert Xu on 19 May 2010 00:20 On Tue, May 18, 2010 at 02:20:21PM -0700, David Miller wrote: > > I'll add the define for sparc, but saying "sparc's fault" is bogus > because I defined what was necessary to get SLAB/SLUB to provide the > necessary alignment. SLOB pays for choosing not to use the same > calculations for minimum alignment as the other allocators, and > therefore pays for being different in this regard. Once that macro hits the main tree I'll push the crypto fix out. Thanks! -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert(a)gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Pekka Enberg on 19 May 2010 01:50
Hi David, On Wed, May 19, 2010 at 12:20 AM, David Miller <davem(a)davemloft.net> wrote: >> Why? It doesn't make much sense for SLOB, which tries to be as space >> efficient as possible, as a default. If things break on sparc, it >> really needs to set ARCH_KMALLOC_MINALIGN as slab default alignment is >> not something you really want to depend on. > > I think it does make sense to expect that, whatever my architecture > defines or does not define, I can expect the allocators to provide the > same minimum alignment guarentee. �Otherwise it is no guarantee at all. They're not a guarantee, the default values are just "oh, you don't care about alignment, let me provide one for you". > I'll add the define for sparc, but saying "sparc's fault" is bogus > because I defined what was necessary to get SLAB/SLUB to provide the > necessary alignment. �SLOB pays for choosing not to use the same > calculations for minimum alignment as the other allocators, and > therefore pays for being different in this regard. > > And in fact I do know that the ifdef'ery in SLAB/SLUB is derived from > a change long ago that was specifically added to handle platforms like > sparc. > > So one of two things should happen: > > 1) SLOB conforms to SLAB/SLUB in it's test > > 2) SLAB/SLUB conforms to SLOB in it's test > > And yes this is an either-or, you can't say they are both valid. I don't agree with that. The default values are subject to change and as pointed out by Paul, they have done so in the past. If you architecture has alignment requirements for _correctness_ you absolutely need to define ARCH_KMALLOC_MINALIGN. Pekka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |