New: u32_classify Kernel Panic

Bugme-new Andrew Morton 5 participants 13 messages Sep 16, 2008 – Jan 6, 2009 1 message not in the archive

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]/

Andrew Morton

author

2008-09-16 16:20 UTC #1 lore raw

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 74 lines · On Mon, 15 Sep 2008 03:35:16 -0700 (PDT) [email protected] wrote:

http://bugzilla.kernel.org/show_bug.cgi?id=11571
           Summary: u32_classify Kernel Panic
           Product: Networking
           Version: 2.5
     KernelVersion: 2.6.26.5
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: high
          Priority: P1
         Component: Other
        AssignedTo: [email protected]
        ReportedBy: [email protected]
Distribution: Debian
Problem Description:
Kernel panic
[<c023f2d8>]    dev_queue_xmit+0x175/0x2a1
[<c0243861>]    neigh_resolve_output+0x1f8/0x2la
[<c025a784>]    ip_finish_output+0x1d7/0x200
[<c025aa2f>]    ip_output+0x6f/0x81
[<c0258218>]    ip_forwardjinish+0x2c/0x2e
[<c0257223>]    ip_rcv_f inish+0x263/0x27f
[<c023cc62>]    netif_receive_skb+0x2c1/0x32b
[<f886f26d>]    e1000_clean_rx_irq+0x395/0x46f [e1000]
[<f886f5f7>]    e1000_clean+0x52/0x1db [e1000]
[<c013e8e4>]    net_rx_action+0x8a/0x153
[<c0128bfa>]    __do_softirq+0x5d/0xc1
[<c0128c98>]    do_softirq+0x32/0x36
[<c0185cb8>]    do_IRQ+0x52/0x66
[<c01887fa>]    mwait_idle+0x8/0x32
[<c018418b>]    common_interrupt+0x23/0x28
[<c01887fa>]    mwait_idle+0xB/0x32
[<c0188829>]    muait_idle+0x2f/0x32
[<c0182545>]    cpu_ i d1e+0x88/0x9c


Code: 0c 8b 80 90 00 00 00 c7 44 24 14 00 00 00 00 c7 44 24 18 00 00 00 00 89
44
24 18 8b 54 24 0c 8b 74 aa 18 85 f6 0f 84 a0 01 00 00 <8b> 46 38 83 00 01 83 50 04 00 8b 4c 24 04 8b 46 38 23 81 88 00
EIP: [<f8bf3670>] u32_classify+0x41/0x23f [cls_u32] SS:ESP 8868:f746fd44
Kernel panic - not syncing: Fatal exception in interrupt
Steps to reproduce:
tc qdisc add dev eth1 root handle 1: htb
tc class add dev eth1 parent 1: classid 1:1 htb rate 3600Kbit
tc class add dev eth1 parent 1:1 classid 1:11 htb rate 2800Kbit prio 0 tc class add dev eth1 parent 1:1 classid 1:15 htb rate 100Kbit ceil 2800Kbit prio 0
tc class add dev eth1 parent 1:1 classid 1:19 htb rate 100Kbit ceil 500Kbit prio 2
N from 10 to 2000
tc class add dev eth1 parent 1:{11,15,19} classid 1:$N htb rate 1Kbit ceil {$SPEED}Kbit
tc filter add dev eth1 parent 1: protocol ip pref $N u32 match ip dst $IP flowid 1:$N
Everything worked with N smaller then 2000. The problem first acquired with kernel 2.6.18-6-686 and is still present in 2.6.26.5
Signature · 3 lines
Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

Jarek Poplawski

2008-09-17 19:38 UTC #2 lore raw

Andrew Morton wrote, On 09/16/2008 06:15 PM:

Quoted 8 lines

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).
Quoted 3 lines · On Mon, 15 Sep 2008 03:35:16 -0700 (PDT) [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=11571
Summary: u32_classify Kernel Panic

...

Could you add more details:
- .config
- gzipped cls_u32.o (compiled with CONFIG_DEBUG_INFO on)
- the first part of OOPS if possible.
- more exactly these tc commands from your script

Does this happen while creating or deleting something and is this easy to reproduce?

Thanks,
Jarek P.

Jarek Poplawski

2008-09-17 22:07 UTC #3 lore raw

On Wed, Sep 17, 2008 at 09:38:32PM +0200, Jarek Poplawski wrote:
...

Quoted 3 lines

Quoted 1 line
Quoted 1 line
http://bugzilla.kernel.org/show_bug.cgi?id=11571
Does this happen while creating or deleting something and is this easy to reproduce?

If accidentally there is any deleting around try this patch, please.

Jarek P.

---

 net/sched/cls_u32.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

net/sched/cls_u32.c +2 -0

index 246f906..9912ad5 100644
@@ -433,7 +433,9 @@ static int u32_delete(struct tcf_proto *tp, unsigned long arg)
 
 	if (ht->refcnt == 1) {
 		ht->refcnt--;
+		tcf_tree_lock(tp);
 		u32_destroy_hnode(tp, ht);
+		tcf_tree_unlock(tp);
 	} else {
 		return -EBUSY;
 	}

m0sia

2008-09-18 07:34 UTC #4 lore raw

In reply to Jarek Poplawski

Jarek Poplawski пишет:

Quoted 21 lines

Andrew Morton wrote, On 09/16/2008 06:15 PM:
Quoted 10 lines
(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).
Quoted 4 lines · On Mon, 15 Sep 2008 03:35:16 -0700 (PDT) [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=11571
Summary: u32_classify Kernel Panic
...
Could you add more details:
- .config
- gzipped cls_u32.o (compiled with CONFIG_DEBUG_INFO on)
- the first part of OOPS if possible.
- more exactly these tc commands from your script

Quoted 6 lines

Does this happen while creating or deleting something and is this easy to reproduce?
Thanks,
Jarek P.

It happens on a production server and i can't experiment with this bug. Now i'm using MARK iptable target and fw mark filter, because of this bug. I think it happens when deleting or adding filters(it occur automatically by script, when adding new user or user change speed). I'll try this patch on test server.

Jarek Poplawski

2008-09-18 07:53 UTC #5 lore raw

On Thu, Sep 18, 2008 at 01:05:28PM +0600, m0sia wrote:
...

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
http://bugzilla.kernel.org/show_bug.cgi?id=11571
Summary: u32_classify Kernel Panic

...

Quoted 5 lines

It happens on a production server and i can't experiment with this bug. Now i'm using MARK iptable target and fw mark filter, because of this bug. I think it happens when deleting or adding filters(it occur automatically by script, when adding new user or user change speed). I'll try this patch on test server.

OK, no hurry. BTW, it looks like some traffic is needed in a qdisc while its filters are modified to trigger this. Probably, turning off CONFIG_CLS_U32_PERF can make this less visible. On the other hand, turning on memory debugging: CONFIG_DEBUG_SLAB or CONFIG_SLUB_DEBUG_ON should be helpful here.

Jarek P.

Jarek Poplawski

2009-01-05 13:52 UTC #6 lore raw

Quoted 3 lines · On Thu, Sep 18, 2008 at 01:05:28PM +0600, m0sia wrote:

...
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
http://bugzilla.kernel.org/show_bug.cgi?id=11571

...
(take 2)

It seems there could be a problem with testing if this patch fixes this bug, but IMHO it's quite probable, and needed anyway.

Jarek P.

----------------->
pkt_sched: cls_u32: Fix locking in u32_change()

New nodes are inserted in u32_change() under rtnl_lock() with wmb(), so without tcf_tree_lock() like in other classifiers (e.g. cls_fw). This isn't enough without rmb() on the read side, but on the other hand adding such barriers doesn't give any savings, so the lock is added instead.

Reported-by m0sia <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

---

 net/sched/cls_u32.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

net/sched/cls_u32.c +2 -1

index 05d1780..07372f6 100644
@@ -638,8 +638,9 @@ static int u32_change(struct tcf_proto *tp, unsigned long base, u32 handle,
 				break;
 
 		n->next = *ins;
-		wmb();
+		tcf_tree_lock(tp);
 		*ins = n;
+		tcf_tree_unlock(tp);
 
 		*arg = (unsigned long)n;
 		return 0;

David Miller

maintainer

2009-01-06 02:14 UTC #7 lore raw

From: Jarek Poplawski <[email protected]>
Date: Mon, 5 Jan 2009 13:52:45 +0000

Quoted 10 lines

pkt_sched: cls_u32: Fix locking in u32_change()
New nodes are inserted in u32_change() under rtnl_lock() with wmb(), so without tcf_tree_lock() like in other classifiers (e.g. cls_fw). This isn't enough without rmb() on the read side, but on the other hand adding such barriers doesn't give any savings, so the lock is added instead.
Reported-by m0sia <[email protected]>
Signed-off-by Jarek Poplawski <[email protected]>

Applied and queued up for -stable, thanks Jarek.

Jarek Poplawski

2008-10-11 11:17 UTC #8 lore raw

pkt_sched: cls_u32: Fix locking in u32_delete()

In reply to Jarek Poplawski

pkt_sched: cls_u32: Fix locking in u32_delete()

While looking for a possible reason of bugzilla [Bug 11571] "u32_classify Kernel Panic" reported by [email protected] I found that tcf_tree_lock() is missing in u32_delete() during u32_destroy_hnode() call. Other paths calling this function use this lock. It haven't been acknowledged this fixes the bug, but I think this patch is needed here anyway.

Signed-off-by Jarek Poplawski <[email protected]>

---

 net/sched/cls_u32.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

net/sched/cls_u32.c +2 -0

index 246f906..9912ad5 100644
@@ -433,7 +433,9 @@ static int u32_delete(struct tcf_proto *tp, unsigned long arg)
 
 	if (ht->refcnt == 1) {
 		ht->refcnt--;
+		tcf_tree_lock(tp);
 		u32_destroy_hnode(tp, ht);
+		tcf_tree_unlock(tp);
 	} else {
 		return -EBUSY;
 	}

Herbert Xu

2008-10-11 14:10 UTC #9 lore raw

Re: pkt_sched: cls_u32: Fix locking in u32_delete()

Quoted 21 lines · Jarek Poplawski

pkt_sched: cls_u32: Fix locking in u32_delete()
While looking for a possible reason of bugzilla [Bug 11571] "u32_classify Kernel Panic" reported by [email protected] I found that tcf_tree_lock() is missing in u32_delete() during u32_destroy_hnode() call. Other paths calling this function use this lock. It haven't been acknowledged this fixes the bug, but I think this patch is needed here anyway.
Signed-off-by Jarek Poplawski <[email protected]>
---
net/sched/cls_u32.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
net/sched/cls_u32.c +0 -0
index 246f906..9912ad5 100644

net/sched/cls_u32.c static int u32_delete(struct tcf_proto *tp, unsigned long arg) · line 438

Quoted 6 lines

net/sched/cls_u32.c +2 -0

@@ -433,7 +433,9 @@ static int u32_delete(struct tcf_proto *tp, unsigned long arg)
 
       if (ht->refcnt == 1) {
               ht->refcnt--;
+               tcf_tree_lock(tp);
               u32_destroy_hnode(tp, ht);
+               tcf_tree_unlock(tp);

Well if you were going to protect you'd need to lock before the reference count check. However, this is actually unecessary because the reference count can only be increased the RTNL which we're already holding.

Also, if the reference count is 1, then there must be no live references in the system to the hash table so we can safely delete it.

So whatever the problem is this isn't it :)

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Miller

maintainer

2008-10-11 19:24 UTC #10 lore raw

Re: pkt_sched: cls_u32: Fix locking in u32_delete()

From: Herbert Xu <[email protected]>
Date: Sat, 11 Oct 2008 22:10:22 +0800

Quoted 34 lines

Quoted 21 lines · Jarek Poplawski
pkt_sched: cls_u32: Fix locking in u32_delete()
While looking for a possible reason of bugzilla [Bug 11571] "u32_classify Kernel Panic" reported by [email protected] I found that tcf_tree_lock() is missing in u32_delete() during u32_destroy_hnode() call. Other paths calling this function use this lock. It haven't been acknowledged this fixes the bug, but I think this patch is needed here anyway.
Signed-off-by Jarek Poplawski <[email protected]>
---
net/sched/cls_u32.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
net/sched/cls_u32.c +0 -0
index 246f906..9912ad5 100644
net/sched/cls_u32.c static int u32_delete(struct tcf_proto *tp, unsigned long arg) · line 438
Quoted 6 lines
net/sched/cls_u32.c +2 -0
@@ -433,7 +433,9 @@ static int u32_delete(struct tcf_proto *tp, unsigned long arg)
 
       if (ht->refcnt == 1) {
               ht->refcnt--;
+               tcf_tree_lock(tp);
               u32_destroy_hnode(tp, ht);
+               tcf_tree_unlock(tp);
Well if you were going to protect you'd need to lock before the reference count check. However, this is actually unecessary because the reference count can only be increased the RTNL which we're already holding.

Quoted 5 lines

Also, if the reference count is 1, then there must be no live references in the system to the hash table so we can safely delete it.
So whatever the problem is this isn't it :)

Agreed, the synchronization is already what is necessary here.

As Herbert stated, the refcounts only change under RTNL and when we see it hit 0 we can be sure we are the only reference to it.

Next, my understanding is that:

1) tc_h_common is a per sched tree object
2) we quiesced the whole sched tree, from the root, before getting to this code

Which means that the hash list deletion in u32_destroy_hnode() is safe as well.

But hey, we could be missing something here, so I'd be happy to hear that Jarek can still see some hole here :) Because it is true that we have seen some weird crashes still and u32 seems common amongst those report.

Jarek Poplawski

2008-10-11 22:03 UTC #11 lore raw

Re: pkt_sched: cls_u32: Fix locking in u32_delete()

Quoted 2 lines · David Miller

From: Herbert Xu <[email protected]>
Date: Sat, 11 Oct 2008 22:10:22 +0800

...

Quoted 5 lines

But hey, we could be missing something here, so I'd be happy to hear that Jarek can still see some hole here :) Because it is true that we have seen some weird crashes still and u32 seems common amongst those report.

Yes, you could be missing something, but I've forgotten what, because when I found this I was much younger and brighter... and alas now I've a busy weekend, so let it wait until monday.

Thanks you both for looking at this,
Jarek P.

David Miller

maintainer

2008-10-11 22:06 UTC #12 lore raw

Re: pkt_sched: cls_u32: Fix locking in u32_delete()

From: Jarek Poplawski <[email protected]>
Date: Sun, 12 Oct 2008 00:04:18 +0200

Quoted 3 lines

Yes, you could be missing something, but I've forgotten what, because when I found this I was much younger and brighter... and alas now I've a busy weekend, so let it wait until monday.

You euros and your "weekends". :-)

Jarek Poplawski

2008-10-13 06:47 UTC #13 lore raw

Re: pkt_sched: cls_u32: Fix locking in u32_delete()

Quoted 6 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Sun, 12 Oct 2008 00:04:18 +0200
Quoted 3 lines
Yes, you could be missing something, but I've forgotten what, because when I found this I was much younger and brighter... and alas now I've a busy weekend, so let it wait until monday.

OK, I give up! You both didn't miss anything and this patch is useless.

Quoted 1 line

You euros and your "weekends". :-)

I wish you were right with this too...

Thanks again,
Jarek P.