Bug in 2.6.11.11 - udp_poll(), fragments + CONFIG_HIGHMEM

Bug in 2.6.11.11 - udp_poll(), fragments + CONFIG_HIGHMEM

[Steven Hand]

Summary: with CONFIG_HIGHMEM set, net/ipv4/udp.c::udp_poll() 
         fails when rx'ing an skb with fragments. 

         Present in [at least] 2.6.11.*, 2.6.10 


Details: 

User space code polling on a blocking socket fd receives an
skb with fragments -- this is very unlikely in the common 
case but can happen with encapsulation etc; however its pretty
much guaranteed to happen under Xen 2.x when communicating 
between dom0 and domU. 


Example backtrace from console: 

 kernel: Badness in local_bh_enable at kernel/softirq.c:140
 kernel:  [local_bh_enable+130/144] local_bh_enable+0x82/0x90
 kernel:  [skb_checksum+317/704] skb_checksum+0x13d/0x2c0
 kernel:  [udp_poll+154/352] udp_poll+0x9a/0x160
 kernel:  [sock_poll+41/64] sock_poll+0x29/0x40
 kernel:  [do_pollfd+149/160] do_pollfd+0x95/0xa0
 kernel:  [do_poll+106/208] do_poll+0x6a/0xd0
 kernel:  [sys_poll+353/576] sys_poll+0x161/0x240
 kernel:  [sys_gettimeofday+60/144] sys_gettimeofday+0x3c/0x90
 kernel:  [__pollwait+0/208] __pollwait+0x0/0xd0
 kernel:  [syscall_call+7/11] syscall_call+0x7/0xb


Reconstructed forward trace: 

   net/ipv4/udp.c:1334   spin_lock_irq() 
   net/ipv4/udp.c:1336   udp_checksum_complete() 
net/core/skbuff.c:1069   skb_shinfo(skb)->nr_frags > 1
net/core/skbuff.c:1086   kunmap_skb_frag()
net/core/skbuff.h:1087   local_bh_enable()
 kernel/softirq.c:0140   WARN_ON(irqs_disabled());

Re: Bug in 2.6.11.11 - udp_poll(), fragments + CONFIG_HIGHMEM

[Andrew Morton]

Steven Hand <Steven.Hand@cl.cam.ac.uk> wrote:
>
> Summary: with CONFIG_HIGHMEM set, net/ipv4/udp.c::udp_poll() 
>           fails when rx'ing an skb with fragments. 
> 
>           Present in [at least] 2.6.11.*, 2.6.10 
> 
> 
>  Details: 
> 
>  User space code polling on a blocking socket fd receives an
>  skb with fragments -- this is very unlikely in the common 
>  case but can happen with encapsulation etc; however its pretty
>  much guaranteed to happen under Xen 2.x when communicating 
>  between dom0 and domU. 
> 
> 
>  Example backtrace from console: 
> 
>   kernel: Badness in local_bh_enable at kernel/softirq.c:140
>   kernel:  [local_bh_enable+130/144] local_bh_enable+0x82/0x90
>   kernel:  [skb_checksum+317/704] skb_checksum+0x13d/0x2c0
>   kernel:  [udp_poll+154/352] udp_poll+0x9a/0x160
>   kernel:  [sock_poll+41/64] sock_poll+0x29/0x40
>   kernel:  [do_pollfd+149/160] do_pollfd+0x95/0xa0
>   kernel:  [do_poll+106/208] do_poll+0x6a/0xd0
>   kernel:  [sys_poll+353/576] sys_poll+0x161/0x240
>   kernel:  [sys_gettimeofday+60/144] sys_gettimeofday+0x3c/0x90
>   kernel:  [__pollwait+0/208] __pollwait+0x0/0xd0
>   kernel:  [syscall_call+7/11] syscall_call+0x7/0xb
> 

Yes, that still seems to be there:

  udp_poll
  ->spin_lock_irq()
  ->udp_checksum_complete
    ->__udp_checksum_complete
      ->skb_checksum
        ->kmap_skb_frag
          ->local_bh_disable

That local_bh_disable() in kmap_skb_frag() looks weird and might be
unnecessary.  Does anyone know what it's there for?  Replace it with
local_irq_save()?

Re: Bug in 2.6.11.11 - udp_poll(), fragments + CONFIG_HIGHMEM

[David S. Miller]

From: Andrew Morton <akpm@osdl.org>
Date: Mon, 30 May 2005 14:17:14 -0700

> That local_bh_disable() in kmap_skb_frag() looks weird and might be
> unnecessary.  Does anyone know what it's there for?  Replace it with
> local_irq_save()?

The SKB kmap types are to be used only from BH context.
So the local_bh_disable() is really necessary.

This limitation causes problems elsewhere too, for example if the tg3
driver has to do the 4GB DMA boundary workaround on transmit, then it
tries to do a skb_copy() in IRQ disabled context, which thusly also
tries to do some SKB kmapping and triggers the same assertion seen
here.

Both UDP and tg3 need to be fixed to not do these operations from such
illegal contexts.

It really stinks that this error on triggers with highmem enabled.
We would have seen both bugs much earlier on otherwise.

Re: Bug in 2.6.11.11 - udp_poll(), fragments + CONFIG_HIGHMEM

[Herbert Xu]

Steven Hand <Steven.Hand@cl.cam.ac.uk> wrote:
> 
> Reconstructed forward trace: 
> 
>   net/ipv4/udp.c:1334   spin_lock_irq() 
>   net/ipv4/udp.c:1336   udp_checksum_complete() 
> net/core/skbuff.c:1069   skb_shinfo(skb)->nr_frags > 1
> net/core/skbuff.c:1086   kunmap_skb_frag()
> net/core/skbuff.h:1087   local_bh_enable()
> kernel/softirq.c:0140   WARN_ON(irqs_disabled());

Thanks for catching this.  The receive queue lock is never taken
in IRQs (and should never be) so we can simply substitute bh for
irq.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -738,7 +738,7 @@ int udp_ioctl(struct sock *sk, int cmd, 
 			unsigned long amount;
 
 			amount = 0;
-			spin_lock_irq(&sk->sk_receive_queue.lock);
+			spin_lock_bh(&sk->sk_receive_queue.lock);
 			skb = skb_peek(&sk->sk_receive_queue);
 			if (skb != NULL) {
 				/*
@@ -748,7 +748,7 @@ int udp_ioctl(struct sock *sk, int cmd, 
 				 */
 				amount = skb->len - sizeof(struct udphdr);
 			}
-			spin_unlock_irq(&sk->sk_receive_queue.lock);
+			spin_unlock_bh(&sk->sk_receive_queue.lock);
 			return put_user(amount, (int __user *)arg);
 		}
 
@@ -848,12 +848,12 @@ csum_copy_err:
 	/* Clear queue. */
 	if (flags&MSG_PEEK) {
 		int clear = 0;
-		spin_lock_irq(&sk->sk_receive_queue.lock);
+		spin_lock_bh(&sk->sk_receive_queue.lock);
 		if (skb == skb_peek(&sk->sk_receive_queue)) {
 			__skb_unlink(skb, &sk->sk_receive_queue);
 			clear = 1;
 		}
-		spin_unlock_irq(&sk->sk_receive_queue.lock);
+		spin_unlock_bh(&sk->sk_receive_queue.lock);
 		if (clear)
 			kfree_skb(skb);
 	}
@@ -1334,7 +1334,7 @@ unsigned int udp_poll(struct file *file,
 		struct sk_buff_head *rcvq = &sk->sk_receive_queue;
 		struct sk_buff *skb;
 
-		spin_lock_irq(&rcvq->lock);
+		spin_lock_bh(&rcvq->lock);
 		while ((skb = skb_peek(rcvq)) != NULL) {
 			if (udp_checksum_complete(skb)) {
 				UDP_INC_STATS_BH(UDP_MIB_INERRORS);
@@ -1345,7 +1345,7 @@ unsigned int udp_poll(struct file *file,
 				break;
 			}
 		}
-		spin_unlock_irq(&rcvq->lock);
+		spin_unlock_bh(&rcvq->lock);
 
 		/* nothing to see, move along */
 		if (skb == NULL)

Re: Bug in 2.6.11.11 - udp_poll(), fragments + CONFIG_HIGHMEM

[David S. Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 31 May 2005 07:49:30 +1000

> Thanks for catching this.  The receive queue lock is never taken
> in IRQs (and should never be) so we can simply substitute bh for
> irq.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks Herbert.