alloc_skb cannot be called with GFP_DMA

alloc_skb cannot be called with GFP_DMA

[Andrea Arcangeli]

I think it's good idea to apply this patch to mainline:

--- 2.4.8pre4aa1/mm/slab.c.~1~	Sun Aug  5 16:46:58 2001
+++ 2.4.8pre4aa1/mm/slab.c	Sun Aug  5 17:44:09 2001
@@ -1172,7 +1172,6 @@
 
 static inline void kmem_cache_alloc_head(kmem_cache_t *cachep, int flags)
 {
-#if DEBUG
 	if (flags & SLAB_DMA) {
 		if (!(cachep->gfpflags & GFP_DMA))
 			BUG();
@@ -1180,7 +1179,6 @@
 		if (cachep->gfpflags & GFP_DMA)
 			BUG();
 	}
-#endif
 }
 
 static inline void * kmem_cache_alloc_one_tail (kmem_cache_t *cachep,


This will trap anybody trying calling alloc_skb using GFP_DMA, that
usage is illegal, it would work by luck only if _everybody_ calling
alloc_skb would be passing GFP_DMA too (because the gfp_mask is passed
to the GFP in order to get right things like GFP_WAIT) which is
obviously not the case as the skb cache is shared by everybody.

Actually such bugcheck is been triggered by an IKD user because IKD
#defines DEBUG, but I think we should enable such bugcheck it in
mainline too because it's too easy to forget about such requirement of
the slab cache.

In order to fix those bugs correctly and avoid memory corruption a new
skbuff_head_cache_isadma skb cache will be needed.

A fast grep revealed a few buggy network drivers already (of course the
bug is going to affect only ISA drivers so it's not a showstopper).

Andrea

Re: alloc_skb cannot be called with GFP_DMA

[Alan Cox]

> In order to fix those bugs correctly and avoid memory corruption a new
> skbuff_head_cache_isadma skb cache will be needed.
> 
> A fast grep revealed a few buggy network drivers already (of course the
> bug is going to affect only ISA drivers so it's not a showstopper).

"It doesnt affect my computer so its not a problem" - humm 

If somoe can add the needed isa dma handling to alloc_skb (or alloc_isa_skb)
I'll go and fix all the drivers up. 

Re: alloc_skb cannot be called with GFP_DMA

[Andrea Arcangeli]

On Sun, Aug 05, 2001 at 05:03:12PM +0100, Alan Cox wrote:
> > A fast grep revealed a few buggy network drivers already (of course the
> > bug is going to affect only ISA drivers so it's not a showstopper).
> 
> "It doesnt affect my computer so its not a problem" - humm 

I never said anything like that, with "it's not a showstopper" I only
meant "it doesn't affect 99% of the hardware configurations out there".

> If somoe can add the needed isa dma handling to alloc_skb (or alloc_isa_skb)

alloc_isa_skb will avoid to slowdown alloc_skb so I prefer it compared
to hiding the logic inside alloc_skb.

> I'll go and fix all the drivers up. 

Thanks. With the bugcheck patch I wanted to make _sure_ that somebody
will do that (which is very far from "... so it's not a problem" :).

Andrea

Re: alloc_skb cannot be called with GFP_DMA

[kuznet]

Hello!

> alloc_isa_skb will avoid to slowdown alloc_skb so I prefer it compared
> to hiding the logic inside alloc_skb.

Stop! This is redundant. GFP_DMA is for skb data, not head!

So that, it is enough and right to clear GFP_DMA inside
alloc_skb when allocating skb head.

Alexey

Re: alloc_skb cannot be called with GFP_DMA

[Andrea Arcangeli]

On Sun, Aug 05, 2001 at 09:18:11PM +0400, kuznet@ms2.inr.ac.ru wrote:
> Hello!
> 
> > alloc_isa_skb will avoid to slowdown alloc_skb so I prefer it compared
> > to hiding the logic inside alloc_skb.
> 
> Stop! This is redundant. GFP_DMA is for skb data, not head!
> 
> So that, it is enough and right to clear GFP_DMA inside
> alloc_skb when allocating skb head.

ah it's all metadata, so this should fix it (the bugcheck will still
trap any skb_clone caller that uses GFP_DMA because it doesn't make
sense to call skb_clone with GFP_DMA):

--- 2.4.8pre4aa1/net/core/skbuff.c.~1~	Sat Jul 21 00:04:34 2001
+++ 2.4.8pre4aa1/net/core/skbuff.c	Sun Aug  5 20:30:00 2001
@@ -180,7 +180,7 @@
 	/* Get the HEAD */
 	skb = skb_head_from_pool();
 	if (skb == NULL) {
-		skb = kmem_cache_alloc(skbuff_head_cache, gfp_mask);
+		skb = kmem_cache_alloc(skbuff_head_cache, gfp_mask & ~__GFP_DMA);
 		if (skb == NULL)
 			goto nohead;
 	}

Andrea

Re: alloc_skb cannot be called with GFP_DMA

[Manfred Spraul]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

[ok, I admit that this doesn't qualify as a timely reply]

Andrea wrote on Sun, 5 Aug 2001 17:53:34 +0200
  >
 > I think it's good idea to apply this patch to mainline:
 >
 > --- 2.4.8pre4aa1/mm/slab.c.~1~ Sun Aug 5 16:46:58 2001
 > +++ 2.4.8pre4aa1/mm/slab.c Sun Aug 5 17:44:09 2001
 > @@ -1172,7 +1172,6 @@
 >
 > static inline void kmem_cache_alloc_head(kmem_cache_t *cachep, int flags)
 > {
 > -#if DEBUG
 > if (flags & SLAB_DMA) {
 > if (!(cachep->gfpflags & GFP_DMA))
 > BUG();
 > @@ -1180,7 +1179,6 @@
 > if (cachep->gfpflags & GFP_DMA)
 > BUG();
 > }
 > -#endif
 > }
 >
 > static inline void * kmem_cache_alloc_one_tail (kmem_cache_t *cachep,
 >
 > This will trap anybody trying calling alloc_skb using GFP_DMA, that
 > usage is illegal, it would work by luck only if _everybody_ calling
 > alloc_skb would be passing GFP_DMA too (because the gfp_mask is passed
 > to the GFP in order to get right things like GFP_WAIT) which is
 > obviously not the case as the skb cache is shared by everybody.

If someone calls alloc_skb(GFP_DMA), then he wants the data buffer in
DMA space, noone does DMA to/from the skb structure fields. The right
approach is to mask the GFP_DMA bit before calling
kmem_cache_alloc(skbuff_head_cache), and to forward the bit to the
kmalloc of the skb data buffer. This is in 2.4.20-pre5, the right fix.

The check was indended to catch a feature that is present in the 2.2
slab allocator, but missing in 2.4: In 2.2, it was possible to create a
custom cachep with kmem_cache_create(), and then allocate some objects
with GFP_DMA set, some without GFP_DMA set. The allocator in 2.4/5
doesn't support that anymore.

But something else is wrong in kmem_cache_alloc_head - the common error,
GFP_WAIT allocation from interrupt, is not checked - neither in DEBUG
nor without DEBUG.

What about the attached patch?
- put the GFP_DMA test again under DEBUG: GFP_DMA errors are virtually
impossible by design.
- add an in_interrupt() check into alloc_head()

I've put the in_interrupt() check under DEBUG, too: It's icache
pollution, and the second test in kmem_cache_grow() will catch abusers
sooner or later.

--
	Manfred


[-- Attachment #2: patch-slab-debug --]
[-- Type: text/plain, Size: 668 bytes --]

--- 2.5/mm/slab.c	Sun Sep 22 06:25:17 2002
+++ build-2.5/mm/slab.c	Thu Sep 26 23:02:40 2002
@@ -1259,6 +1259,13 @@
 
 static inline void kmem_cache_alloc_head(kmem_cache_t *cachep, int flags)
 {
+#if DEBUG
+	/* check if someone calls us from interrupt with GFP_WAIT set
+	 * With debug disabled, this only happens in kmem_cache_grow,
+	 * to reduce the critical path length.
+	 */
+	if (in_interrupt() && (flags & __GFP_WAIT))
+		BUG();
 	if (flags & SLAB_DMA) {
 		if (!(cachep->gfpflags & GFP_DMA))
 			BUG();
@@ -1266,6 +1273,7 @@
 		if (cachep->gfpflags & GFP_DMA)
 			BUG();
 	}
+#endif
 }
 
 static inline void * kmem_cache_alloc_one_tail (kmem_cache_t *cachep,