Re: [BUG] SLOB breaks Crypto

[Paul Mundt <lethal@linux-sh.org>]

(adding Christoph and dwmw2 to the Cc..)

On Wed, May 19, 2010 at 07:35:07AM +0900, Paul Mundt wrote:
> On Tue, May 18, 2010 at 02:20:21PM -0700, David Miller wrote:
> > From: Pekka Enberg <penberg@cs.helsinki.fi>
> > Date: Wed, 19 May 2010 00:15:46 +0300
> > 
> > > On Tue, May 18, 2010 at 11:59 PM, David Miller <davem@davemloft.net> wrote:
> > >> From: Matt Mackall <mpm@selenic.com>
> > >> Date: Tue, 18 May 2010 14:33:55 -0500
> > >>
> > >>> SLOB honors ARCH_KMALLOC_MINALIGN. If your arch has alignment
> > >>> requirements, I recommend you set it.
> > >>
> > >> I recommend that the alignment provided by the allocator is not
> > >> determined by which allocator I happen to have enabled.
> > >>
> > >> The values and ifdef'ery should be identical in all of our
> > >> allocators.
> > > 
> > > 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.
> > 
> SLAB/SLUB/SLOB all used to have the same BYTES_PER_WORD alignment
> guarantee, with SLAB and SLUB having moved away from this to unsigned
> long long in b46b8f19 and 47bfdc0d respectively. This was due to mixing
> 64-bit integers in data structures, which in the SLAB case resulted in
> misaligned structures and also broke redzoning (architecture overrides
> also disabled it completely). The SLUB change was made a couple of days
> earlier for the same structure misalignment reasons (64-bit integers on
> 32-bit platforms).
> 
> The default changes in SLAB/SLUB at least assume that 32-bit
> architectures can only address 64-bit values on a 64-bit boundary. While
> this is true for most cases, these have always been handled through the
> bumping of the architecture minalign values in the past. Indeed, this was
> the rationale I had for adding the architecture-specific slab minalign
> override in the first place. The kmalloc one on the other hand is largely
> just overriden for platforms with DMA requirements -- usually a
> cacheline boundary.
> 
> > It's already obvious from these reports that such dependencies do
> > exist.
> > 
> These dependencies were then introduced after SLAB/SLUB changed the
> rules, suggesting that not enough testing was done.
> 
> > 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 see any reason to punish SLOB for the assumptions that SLAB/SLUB
> arbitrarily took up, presumably on an architecture that should have
> specified its own alignment requirements and simply couldn't be bothered.
> Making SLAB redzoning work with arbitrary alignment is another matter
> entirely, and something that should probably be revisited.
> 
> Anything that assumes more than BYTES_PER_WORD is simply broken and
> should be reverted.