Frozen machine with adding a tc filter

Hi!

This is a (really early) bug report on one i just caught when setting a
filter for traffic control.

It makes the machine to frozen solid, without even an oops. Keyboard is not
responsive (neither sysrq nor the num/caps/screen lock leds), everything
seems quite dead.

It's fully reproducible, i tried both 2.4.13 and 2.4.19-rc3 (the last one
with htb patches, but i don't think it's related).

The machine is an iPII with 256Mb RAM, it has been working well for about a
year and a half.


Basically a main cbq class with two (also cbq) leaves:
tc qdisc add dev eth1 root handle 20:0 cbq bandwidth 100Mbit avpkt 1000
tc class add dev eth1 parent 20:0 classid 20:1 $EST cbq bandwidth 100Mbit \
	rate 2800Kbit bounded isolated $CBQ_PARAMS

($EST is empty and $CBQ_PARAMS is "allot 1514 avpkt 1000")

Both leaves are basically the same, just a cbq class with parent 20:1 and
then sfq attached as qdisc; really simple. I have another set pretty much
like this one, only with one leave, on eth0.

The command which frozes the machine is
tc filter add dev eth1 parent 20:0 protocol ip handle 5 fw classid 20:0

I know it's weird, but anyway i don't think that a lockup is the error the
user deserves =)

Setting classid 20:X (X != 0) works as expected.

As this is quite early (just hit it) i don't know if i can reproduce it
using another scenario.

Tomorrow i'll try to dig into it on a testing machine and i'll post the
results.


Thanks,
		Alberto

Hello!

I know it's weird, but anyway i don't think that a lockup is the error the
user deserves =)

You really deserved this.

Alexey

On Tue, Jul 23, 2002 at 12:46:44AM +0400, kuznet@ms2.inr.ac.ru wrote:

Hello!

quoted

I know it's weird, but anyway i don't think that a lockup is the error the
user deserves =)

You really deserved this.

I also reported this - should tc check this? Or the kernel? I think each
qdisc implements its own filter hooks and rules, so tc may be a nice place
to do at least simple checking, although it cannot easily spot complicated
loops.

                if ((cl = (void*)res.class) == NULL) {
	                if (TC_H_MAJ(res.classid))
                                cl = cbq_class_lookup(q, res.classid); 
                	else if ((cl = defmap[res.classid&TC_PRIO_MAX]) == NULL
                        	cl = defmap[TC_PRIO_BESTEFFORT];

                     	if (cl == NULL || cl->level >= head->level)
                                goto fallback;
  		}


Aren't the last 2 lines meant to prevent this from happening?
(net/sched/sch_cbq.c).

Please tell me your ideas, I hope to fix this. I know people who've had this
happen by accident and they really hate it.

Regards,

bert

-- 
http://www.PowerDNS.com          Versatile DNS Software & Services
http://www.tk                              the dot in .tk
http://lartc.org           Linux Advanced Routing & Traffic Control HOWTO

Hello!

I also reported this

Sorry, I do not remeber this.

	 - should tc check this? Or the kernel?

The kernel. This funny feature was supposed to disappear ages ago, in 2.2.
It did not, hence, it did not. :-)

Aren't the last 2 lines meant to prevent this from happening?

They are.

Please tell me your ideas,

Well, before all figure out where it deadlocks.

Alexey

On Tue, Jul 23, 2002 at 02:57:14AM +0400, kuznet@ms2.inr.ac.ru wrote:

quoted

Please tell me your ideas,

Well, before all figure out where it deadlocks.

I tried this in User Mode Linux and after some help by Jeff, I figured out
that the deadlock is within cbq_classify() in sch_cbq.c. The problem is that
the 'no upwards classifying' check:

	if (cl == NULL || cl->level >= head->level)
		goto fallback;

is only applied if the classifyer did not return an answer:

	if (!head->filter_list || (result = tc_classify(skb,
		head->filter_list, &res)) < 0)
			goto fallback;

	if ((cl = (void*)res.class) == NULL) {
		if (TC_H_MAJ(res.classid))
			cl = cbq_class_lookup(q, res.classid);
		else if ((cl = defmap[res.classid&TC_PRIO_MAX]) == NULL)
			cl = defmap[TC_PRIO_BESTEFFORT];

		if (cl == NULL || cl->level >= head->level)
			goto fallback;
	}

However, with the commandlines specified, the classifier returns.. head, and
the loop prevention test is never performed.

Suggested fix is to move down the test to the end of the loop so it is
always invoked before iterating. Another test that might sense is to see if
head and cl differ before moving on. A loop is guaranteed in that case.

I can build this into a patch if needed, but I suspect Alexey will want to
write his own.

Thanks for your time.

Regards,

bert

-- 
http://www.PowerDNS.com          Versatile DNS Software & Services
http://www.tk                              the dot in .tk
http://lartc.org           Linux Advanced Routing & Traffic Control HOWTO

Hello!

However, with the commandlines specified, the classifier returns.. head,

This is strange. cbq_bind_filter() is supposed to return nil class,
unless reference has deeper level.

Alexey

On Wed, Jul 24, 2002 at 01:56:22AM +0400, kuznet@ms2.inr.ac.ru wrote:

Hello!

quoted

However, with the commandlines specified, the classifier returns.. head,

This is strange. cbq_bind_filter() is supposed to return nil class,
unless reference has deeper level.

These are the commandlines:

tc qdisc add dev eth0 handle 1:0 root cbq bandwidth 12500000bps avpkt 1000      
tc class add dev eth0 parent 1:0 classid 1:2 cbq bandwidth 12500000bps rate \
	10000bps allot 1500 avpkt 1000 maxburst 5 bounded                              
tc filter add dev eth0 parent 1:0 protocol ip prio 1 handle 1:0:0 u32 \
          divisor 1
tc filter add dev eth0 parent 1:0 protocol ip prio 1 u32 match u32 \             
	0xd5f4a8d2 0xffffffff at 16 classid 1:0  

In the last command, cbq_bind_filter is called with parent==0:

static unsigned long cbq_bind_filter(struct Qdisc *sch, unsigned long parent,
				     u32 classid)
{
	struct cbq_sched_data *q = (struct cbq_sched_data *)sch->data;
	struct cbq_class *p = (struct cbq_class*)parent;
	struct cbq_class *cl = cbq_class_lookup(q, classid);

	if (cl) {
		if (p && p->level <= cl->level)
			return 0;
		cl->filters++;
		return (unsigned long)cl;
	}
	return 0;
}

cl->level is in fact 1.


Regards,

bert

-- 
http://www.PowerDNS.com          Versatile DNS Software & Services
http://www.tk                              the dot in .tk
http://lartc.org           Linux Advanced Routing & Traffic Control HOWTO

Hello!

In the last command, cbq_bind_filter is called with parent==0:

Damn, indeed. Congratulations!

It should look like:

+	if (p == NULL)
+	         p = &q->link;
        if (cl) {
-                if (p && p->level <= cl->level)
+                if (p->level <= cl->level)
                        return 0;
                 cl->filters++;


Alexey