[net-next-2.6] Null pointer dereference in dev_gso_skb_destructor()

[net-next-2.6] Null pointer dereference in dev_gso_skb_destructor()

[Jay Cliburn]

It appears as though the following net-next-2.6 commit (pulled Oct 1
2008) exposes a null pointer dereference in
dev.c:dev_gso_skb_destructor().

commit 242f8bfefe4bed626df4e4727ac8f315d80b567a
Author: David S. Miller <davem@davemloft.net>
Date:   Mon Sep 22 22:15:30 2008 -0700

    pkt_sched: Make qdisc->gso_skb a list.
    
    The idea is that we can use this to get rid of
    ->requeue().
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

>From what I can tell, dev_gso_skb_descructor() is being handed an skb
with skb->next set to NULL.  I verified this by inserting a BUG_ON as
indicated here.

static void dev_gso_skb_destructor(struct sk_buff *skb)
{
	struct dev_gso_cb *cb;

	BUG_ON(!skb->next);  <---------

	do {
		struct sk_buff *nskb = skb->next;

		skb->next = nskb->next;
		nskb->next = NULL;
		kfree_skb(nskb);
	} while (skb->next);

	cb = DEV_GSO_CB(skb);
	if (cb->destructor)
		cb->destructor(skb);
}

Sure enough, I hit the BUG_ON, but unfortunately no trace was produced.

I don't know enough about net core to understand why, but I encounter
the bug only when commit 242f8bfe is applied (as confirmed by
git-bisect). Due to merge conflicts, I'm unable to revert the commit.

This is reproducible using bog standard iperf runs (iperf -s at one
end, iperf -c <addr> at the other).  The oops occurs at the client end.

NB: I just verified the bug is present in current net-next-2.6 (HEAD
e69c4e0f1210450841e40716894ba6a877b31d52).

Here's the oops captured from a serial console.

[  737.313347] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
[  737.314127] IP: [<ffffffff81222025>] dev_gso_skb_destructor+0x14/0x3e
[  737.314127] PGD aade8067 PUD aace4067 PMD 0 
[  737.314127] Oops: 0000 [1] SMP DEBUG_PAGEALLOC
[  737.314127] CPU 1 
[  737.314127] Modules linked in: atl1 sit tunnel4 fuse nf_conntrack_ipv4 ipt_REJECT iptable_filter ip_tables ip6t_REJECT xt_tcpudp nf_conntrack_ipv6 xt_state nf_conntrack ip6tabl]
[  737.314127] Pid: 8298, comm: iperf Not tainted 2.6.27-rc8 #4
[  737.314127] RIP: 0010:[<ffffffff81222025>]  [<ffffffff81222025>] dev_gso_skb_destructor+0x14/0x3e
[  737.314127] RSP: 0018:ffff8800be5d38f8  EFLAGS: 00010296
[  737.314127] RAX: 0000000000000100 RBX: ffff8800c8d564e8 RCX: ffff8800c8d564e8
[  737.314127] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000000
[  737.314127] RBP: ffff8800be5d3908 R08: 0000000000000001 R09: ffff8800d3c52ca0
[  737.314127] R10: 0000000000000001 R11: ffff8800be5d3958 R12: ffff8800c8d564e8
[  737.314127] R13: ffff8800d0c1e588 R14: ffff8800d3c520a0 R15: ffff8800d0c1e578
[  737.314127] FS:  0000000040f4c950(0063) GS:ffff8800d78474b0(0000) knlGS:0000000000000000
[  737.314127] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  737.314127] CR2: 0000000000000000 CR3: 00000000aacc0000 CR4: 00000000000006e0
[  737.314127] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  737.314127] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  737.314127] Process iperf (pid: 8298, threadinfo ffff8800be5d2000, task ffff8800c46e5e80)
[  737.314127] Stack:  ffff8800c8d564e8 ffff8800c8d564e8 ffff8800be5d3928 ffffffff8121d89f
[  737.314127]  ffff8800be5d3958 ffff8800c8d564e8 ffff8800be5d3948 ffffffff8121cdd8
[  737.314127]  ffff8800c8d564e8 ffff8800aaddfb80 ffff8800be5d3958 ffffffff8121ce73
[  737.314127] Call Trace:
[  737.314127]  [<ffffffff8121d89f>] skb_release_head_state+0x6d/0xcb
[  737.314127]  [<ffffffff8121cdd8>] __kfree_skb+0x16/0x85
[  737.314127]  [<ffffffff8121ce73>] kfree_skb+0x2c/0x2e
[  737.314127]  [<ffffffff8122393e>] dev_hard_start_xmit+0x284/0x295
[  737.314127]  [<ffffffff812335dc>] __qdisc_run+0x124/0x23d
[  737.314127]  [<ffffffff81223de5>] dev_queue_xmit+0x383/0x4bb
[  737.314127]  [<ffffffff81223baf>] ? dev_queue_xmit+0x14d/0x4bb
[  737.314127]  [<ffffffff81249218>] ip_finish_output+0x213/0x258
[  737.314127]  [<ffffffff812492fd>] ip_output+0xa0/0xa5
[  737.314127]  [<ffffffff812483cf>] ip_local_out+0x25/0x29
[  737.314127]  [<ffffffff81248c4f>] ip_queue_xmit+0x2e9/0x33f
[  737.314127]  [<ffffffff8124fd46>] ? sk_stream_alloc_skb+0x3d/0xf2
[  737.314127]  [<ffffffff81259724>] tcp_transmit_skb+0x616/0x659
[  737.314127]  [<ffffffff8125c0d8>] __tcp_push_pending_frames+0x747/0x85d
[  737.314127]  [<ffffffff812a6d82>] ? _spin_unlock_bh+0x34/0x38
[  737.314127]  [<ffffffff81218e6f>] ? release_sock+0x36/0xdb
[  737.314127]  [<ffffffff81250747>] tcp_sendmsg+0x94c/0xa6a
[  737.314127]  [<ffffffff81216273>] sock_aio_write+0x13c/0x150
[  737.314127]  [<ffffffff8105345d>] ? lock_hrtimer_base+0x2a/0x50
[  737.314127]  [<ffffffff81149999>] ? _raw_spin_lock+0x68/0x10a
[  737.314127]  [<ffffffff810b1476>] do_sync_write+0xec/0x132
[  737.314127]  [<ffffffff810505f1>] ? autoremove_wake_function+0x0/0x3d
[  737.314127]  [<ffffffff810b2534>] ? fget_light+0x50/0xe4
[  737.314127]  [<ffffffff8110de7c>] ? security_file_permission+0x16/0x18
[  737.314127]  [<ffffffff810b1def>] vfs_write+0xc6/0x15c
[  737.314127]  [<ffffffff810b1f53>] sys_write+0x4c/0x75
[  737.314127]  [<ffffffff8100c19a>] system_call_fastpath+0x16/0x1b
[  737.314127] 
[  737.314127] 
[  737.314127] Code: ff ff 4c 89 e7 4d 8b 24 24 4c 39 ef 75 c9 5b 41 5c 41 5d 41 5e c9 c3 55 48 89 e5 53 48 83 ec 08 e8 61 9f de ff 48 89 fb 48 8b 3b <48> 8b 07 48 89 03 48 c7 07  
[  737.314127] RIP  [<ffffffff81222025>] dev_gso_skb_destructor+0x14/0x3e
[  737.314127]  RSP <ffff8800be5d38f8>
[  737.314127] CR2: 0000000000000000
[  738.502779] ---[ end trace df7cc55b4ac2d88b ]---
[  738.516617] Kernel panic - not syncing: Aiee, killing interrupt handler!

--
Jay

[PATCH] Re: [net-next-2.6] Null pointer dereference in dev_gso_skb_destructor()

[Jarek Poplawski]

On 05-10-2008 20:24, Jay Cliburn wrote:
> It appears as though the following net-next-2.6 commit (pulled Oct 1
> 2008) exposes a null pointer dereference in
> dev.c:dev_gso_skb_destructor().
> 
> commit 242f8bfefe4bed626df4e4727ac8f315d80b567a
> Author: David S. Miller <davem@davemloft.net>
> Date:   Mon Sep 22 22:15:30 2008 -0700
> 
>     pkt_sched: Make qdisc->gso_skb a list.

I think, this should help.

Thanks,
Jarek P.

--------------------->

pkt_sched: Fix handling of gso skbs on requeuing

Jay Cliburn noticed and diagnosed a bug triggered in
dev_gso_skb_destructor() after last change from qdisc->gso_skb
to qdisc->requeue list. Since gso_segmented skbs can't be queued
to another list this patch brings back qdisc->gso_skb for them.

Reported-by: Jay Cliburn <jcliburn@gmail.com>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 include/net/sch_generic.h |    1 +
 net/sched/sch_generic.c   |   22 +++++++++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 3b983e8..3fe49d8 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -52,6 +52,7 @@ struct Qdisc
 	u32			parent;
 	atomic_t		refcnt;
 	unsigned long		state;
+	struct sk_buff		*gso_skb;
 	struct sk_buff_head	requeue;
 	struct sk_buff_head	q;
 	struct netdev_queue	*dev_queue;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 5e7e0bd..3db4cf1 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -44,7 +44,10 @@ static inline int qdisc_qlen(struct Qdisc *q)
 
 static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
 {
-	__skb_queue_head(&q->requeue, skb);
+	if (unlikely(skb->next))
+		q->gso_skb = skb;
+	else
+		__skb_queue_head(&q->requeue, skb);
 
 	__netif_schedule(q);
 	return 0;
@@ -52,7 +55,10 @@ static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
 
 static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
 {
-	struct sk_buff *skb = skb_peek(&q->requeue);
+	struct sk_buff *skb = q->gso_skb;
+
+	if (!skb)
+		skb = skb_peek(&q->requeue);
 
 	if (unlikely(skb)) {
 		struct net_device *dev = qdisc_dev(q);
@@ -60,10 +66,15 @@ static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
 
 		/* check the reason of requeuing without tx lock first */
 		txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
-		if (!netif_tx_queue_stopped(txq) && !netif_tx_queue_frozen(txq))
-			__skb_unlink(skb, &q->requeue);
-		else
+		if (!netif_tx_queue_stopped(txq) &&
+		    !netif_tx_queue_frozen(txq)) {
+			if (q->gso_skb)
+				q->gso_skb = NULL;
+			else
+				__skb_unlink(skb, &q->requeue);
+		} else {
 			skb = NULL;
+		}
 	} else {
 		skb = q->dequeue(q);
 	}
@@ -548,6 +559,7 @@ void qdisc_destroy(struct Qdisc *qdisc)
 	module_put(ops->owner);
 	dev_put(qdisc_dev(qdisc));
 
+	kfree_skb(qdisc->gso_skb);
 	__skb_queue_purge(&qdisc->requeue);
 
 	kfree((char *) qdisc - qdisc->padded);

Re: [PATCH] Re: [net-next-2.6] Null pointer dereference in dev_gso_skb_destructor()

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 6 Oct 2008 09:45:43 +0000

> pkt_sched: Fix handling of gso skbs on requeuing
> 
> Jay Cliburn noticed and diagnosed a bug triggered in
> dev_gso_skb_destructor() after last change from qdisc->gso_skb
> to qdisc->requeue list. Since gso_segmented skbs can't be queued
> to another list this patch brings back qdisc->gso_skb for them.
> 
> Reported-by: Jay Cliburn <jcliburn@gmail.com>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Applied thanks Jarek.

[PATCH] pkt_sched: Simplify dev_requeue_skb and dequeue_skb

[Jarek Poplawski]

On Mon, Oct 06, 2008 at 09:55:01AM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 6 Oct 2008 09:45:43 +0000
> 
> > pkt_sched: Fix handling of gso skbs on requeuing
> > 
> > Jay Cliburn noticed and diagnosed a bug triggered in
> > dev_gso_skb_destructor() after last change from qdisc->gso_skb
> > to qdisc->requeue list. Since gso_segmented skbs can't be queued
> > to another list this patch brings back qdisc->gso_skb for them.
> > 
> > Reported-by: Jay Cliburn <jcliburn@gmail.com>
> > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> 
> Applied thanks Jarek.


Actually, I was just about to send take 2...

So, here is some PS.

Thanks,
Jarek P.
 
--------------------->

pkt_sched: Simplify dev_requeue_skb and dequeue_skb

qdisc->requeue was planned to universally replace all requeuing code,
but at the top level we never requeue more than one skb, so qdisc->
gso_skb is enough for this. qdisc->requeue would be used on the lower
levels only for one level deep requeuing (like in sch_hfsc) after
finishing all the changes.

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

diff -Nurp a/net/sched/sch_generic.c b/net/sched/sch_generic.c
--- a/net/sched/sch_generic.c	2008-10-06 19:07:28.000000000 +0200
+++ b/net/sched/sch_generic.c	2008-10-06 18:26:29.000000000 +0200
@@ -44,12 +44,9 @@ static inline int qdisc_qlen(struct Qdis
 
 static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
 {
-	if (unlikely(skb->next))
-		q->gso_skb = skb;
-	else
-		__skb_queue_head(&q->requeue, skb);
-
+	q->gso_skb = skb;
 	__netif_schedule(q);
+
 	return 0;
 }
 
@@ -57,24 +54,16 @@ static inline struct sk_buff *dequeue_sk
 {
 	struct sk_buff *skb = q->gso_skb;
 
-	if (!skb)
-		skb = skb_peek(&q->requeue);
-
 	if (unlikely(skb)) {
 		struct net_device *dev = qdisc_dev(q);
 		struct netdev_queue *txq;
 
 		/* check the reason of requeuing without tx lock first */
 		txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
-		if (!netif_tx_queue_stopped(txq) &&
-		    !netif_tx_queue_frozen(txq)) {
-			if (q->gso_skb)
-				q->gso_skb = NULL;
-			else
-				__skb_unlink(skb, &q->requeue);
-		} else {
+		if (!netif_tx_queue_stopped(txq) && !netif_tx_queue_frozen(txq))
+			q->gso_skb = NULL;
+		else
 			skb = NULL;
-		}
 	} else {
 		skb = q->dequeue(q);
 	}

Re: [PATCH] pkt_sched: Simplify dev_requeue_skb and dequeue_skb

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 6 Oct 2008 19:38:42 +0200

> pkt_sched: Simplify dev_requeue_skb and dequeue_skb
> 
> qdisc->requeue was planned to universally replace all requeuing code,
> but at the top level we never requeue more than one skb, so qdisc->
> gso_skb is enough for this. qdisc->requeue would be used on the lower
> levels only for one level deep requeuing (like in sch_hfsc) after
> finishing all the changes.
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

I'll apply this, thanks.