Skip to contentSkip to conversation

sch_htb: let skb->priority refer to non-leaf class

PATCH Superseded Harry Mason 5 participants 10 messages Jan 16, 2014 – Jan 23, 2014

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Harry Mason

author
#0 lore raw

PATCH 1/1 sch_htb: let skb->priority refer to non-leaf class

If the class in skb->priority is not a leaf, apply filters from the selected class, not the qdisc. This allows netfilter or user space to partially classify the packet and tc filters to finish it off.

Signed-off-by Harry Mason <[email protected]>
---
 net/sched/sch_htb.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
net/sched/sch_htb.c +0 -0
index 717b210..e50ab65 100644
@@ -223,7 +223,13 @@ static struct htb_class *htb_classify(struct

sk_buff *skb, struct Qdisc *sch,

        return cl;

    *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;

- tcf = q->filter_list;
+
+ /* Start with inner filter chain if a non-leaf class is selected */
+ if (cl)
+ tcf = cl->filter_list;
+ else
+ tcf = q->filter_list;
+

    while (tcf && (result = tc_classify(skb, tcf, &res)) >= 0) {
 #ifdef CONFIG_NET_CLS_ACT
        switch (result) {
Signature · 1 line
1.7.10.4
#1 lore raw
Quoted 16 lines · Harry Mason

If the class in skb->priority is not a leaf, apply filters from the selected class, not the qdisc. This allows netfilter or user space to partially classify the packet and tc filters to finish it off.

Signed-off-by Harry Mason <[email protected]>
---
 net/sched/sch_htb.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
net/sched/sch_htb.c +0 -0
index 717b210..e50ab65 100644
@@ -223,7 +223,13 @@ static struct htb_class *htb_classify(struct
 sk_buff *skb, struct Qdisc *sch,
        return cl;

net/sched/sch_htb.c static struct htb_class *htb_classify(struct · line 233

Quoted 10 lines
net/sched/sch_htb.c +7 -1
 
    *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
-   tcf = q->filter_list;
+
+   /* Start with inner filter chain if a non-leaf class is selected */
+   if (cl)
+       tcf = cl->filter_list;
+   else
+       tcf = q->filter_list;
+

Interesting idea.

Could this break some existing htb setups ?

Also we test cl being NULL at line 222, it would be nice to not test it again...

author
#2 lore raw

PATCH v2 1/1 sch_htb: let skb->priority refer to non-leaf class

If the class in skb->priority is not a leaf, apply filters from the selected class, not the qdisc. This lets netfilter or user space partially classify the packet.

Signed-off-by Harry Mason <[email protected]>

---

Quoted 9 lines · Eric Dumazet

Review

Quoted 5 lines · Harry Mason

+ /* Start with inner filter chain if a non-leaf class is selected */
+ if (cl)

(quoted diff) +3 -0
+     tcf = cl->filter_list;
+ else
+     tcf = q->filter_list;

Could this break some existing htb setups ?

I think it is unlikely. Setting skb->priority to a non-leaf class would be equivalent to setting it to the base qdisc. In theory an application might rely on this if it expects the classes to be dynamic, but adding a filter could restore the old behaviour.

To me this is intuitively how it should behave, and reproduces what would happen if a tc filter instead of netfilter had first assigned the non-leaf class.

Quoted 2 lines

Also we test cl being NULL at line 222, it would be nice to not test it again...

Updated below.

 net/sched/sch_htb.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
net/sched/sch_htb.c +7 -3
index 717b210..8073d92 100644
@@ -219,11 +219,15 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch,
 	if (skb->priority == sch->handle)
 		return HTB_DIRECT;	/* X:0 (direct flow) selected */
 	cl = htb_find(skb->priority, sch);
-	if (cl && cl->level == 0)
-		return cl;
+	if (cl) {
+		if (cl->level == 0)
+			return cl;
+		/* Start with inner filter chain if a non-leaf class is selected */
+		tcf = cl->filter_list;
+	} else
+		tcf = q->filter_list;
 
 	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
-	tcf = q->filter_list;
 	while (tcf && (result = tc_classify(skb, tcf, &res)) >= 0) {
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
Signature · 1 line
1.7.10.4
#3 lore raw

Hello.

Quoted 42 lines · Harry Mason

If the class in skb->priority is not a leaf, apply filters from the selected class, not the qdisc. This lets netfilter or user space partially classify the packet.

Signed-off-by Harry Mason <[email protected]>

---

Quoted 9 lines · Eric Dumazet

net/sched/sch_htb.c static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch, · line 228

Quoted 5 lines · Harry Mason

+ /* Start with inner filter chain if a non-leaf class is selected */
+ if (cl)

net/sched/sch_htb.c +3 -0
+     tcf = cl->filter_list;
+ else
+     tcf = q->filter_list;

Could this break some existing htb setups ?

I think it is unlikely. Setting skb->priority to a non-leaf class would be equivalent to setting it to the base qdisc. In theory an application might rely on this if it expects the classes to be dynamic, but adding a filter could restore the old behaviour.

To me this is intuitively how it should behave, and reproduces what would happen if a tc filter instead of netfilter had first assigned the non-leaf class.

Quoted 2 lines

Also we test cl being NULL at line 222, it would be nice to not test it again...

Updated below.

  net/sched/sch_htb.c |   10 +++++++---
  1 file changed, 7 insertions(+), 3 deletions(-)
net/sched/sch_htb.c +0 -0
index 717b210..8073d92 100644
@@ -219,11 +219,15 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch,
  	if (skb->priority == sch->handle)
  		return HTB_DIRECT;	/* X:0 (direct flow) selected */

net/sched/sch_htb.c static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch, · line 228

Quoted 10 lines
net/sched/sch_htb.c +7 -2
  	cl = htb_find(skb->priority, sch);
-	if (cl && cl->level == 0)
-		return cl;
+	if (cl) {
+		if (cl->level == 0)
+			return cl;
+		/* Start with inner filter chain if a non-leaf class is selected */
+		tcf = cl->filter_list;
+	} else
+		tcf = q->filter_list;

There should be {} in the *else* arm, since it's in another arm of *if*
already -- see Documentation/CodingStyle.

WBR, Sergei

author
#4 lore raw

PATCH v3 1/1 sch_htb: let skb->priority refer to non-leaf class

If the class in skb->priority is not a leaf, apply filters from the selected class, not the qdisc. This lets netfilter or user space partially classify the packet.

Signed-off-by Harry Mason <[email protected]>

---

Quoted 11 lines · Sergei Shtylyov

Review

Quoted 7 lines · Harry Mason
(quoted diff) +7 -0
+	if (cl) {
+		if (cl->level == 0)
+			return cl;
+		/* Start with inner filter chain if a non-leaf class is selected */
+		tcf = cl->filter_list;
+	} else
+		tcf = q->filter_list;

There should be {} in the *else* arm, since it's in another arm of *if*
already -- see Documentation/CodingStyle.

Thanks, updated.

 net/sched/sch_htb.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
net/sched/sch_htb.c +8 -3
index 717b210..e9c8c08 100644
@@ -219,11 +219,16 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch,
 	if (skb->priority == sch->handle)
 		return HTB_DIRECT;	/* X:0 (direct flow) selected */
 	cl = htb_find(skb->priority, sch);
-	if (cl && cl->level == 0)
-		return cl;
+	if (cl) {
+		if (cl->level == 0)
+			return cl;
+		/* Start with inner filter chain if a non-leaf class is selected */
+		tcf = cl->filter_list;
+	} else {
+		tcf = q->filter_list;
+	}
 
 	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
-	tcf = q->filter_list;
 	while (tcf && (result = tc_classify(skb, tcf, &res)) >= 0) {
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
Signature · 1 line
1.7.10.4
maintainer
#5 lore raw

From: Harry Mason <[email protected]>
Date: Fri, 17 Jan 2014 13:22:32 +0000

Quoted 5 lines

If the class in skb->priority is not a leaf, apply filters from the selected class, not the qdisc. This lets netfilter or user space partially classify the packet.

Signed-off-by Harry Mason <[email protected]>

Can I please get some reviews of this patch?

Thanks.

#6 lore raw

Looks reasonable from my view.

cheers,
jamal

Quoted 12 lines · David Miller

From: Harry Mason <[email protected]>
Date: Fri, 17 Jan 2014 13:22:32 +0000

Quoted 5 lines

If the class in skb->priority is not a leaf, apply filters from the selected class, not the qdisc. This lets netfilter or user space partially classify the packet.

Signed-off-by Harry Mason <[email protected]>

Can I please get some reviews of this patch?

Thanks.

#9 lore raw
In reply to Harry Mason
Quoted 16 lines · Harry Mason

If the class in skb->priority is not a leaf, apply filters from the selected class, not the qdisc. This lets netfilter or user space partially classify the packet.

Signed-off-by Harry Mason <[email protected]>

---

Quoted 8 lines · Eric Dumazet
Quoted 5 lines · Harry Mason

+ /* Start with inner filter chain if a non-leaf class is selected */
+ if (cl)

net/sched/sch_htb.c +3 -0
+     tcf = cl->filter_list;
+ else
+     tcf = q->filter_list;
Quoted 7 lines
Quoted 1 line

Could this break some existing htb setups ?

I think it is unlikely. Setting skb->priority to a non-leaf class would be equivalent to setting it to the base qdisc. In theory an application might rely on this if it expects the classes to be dynamic, but adding a filter could restore the old behaviour.

Problem is : Your patch is one patch among thousands of patches, and people will install new kernels without knowing this could have an impact on their setup and might discover the problems too late (after some failure)

Quoted 3 lines

To me this is intuitively how it should behave, and reproduces what would happen if a tc filter instead of netfilter had first assigned the non-leaf class.

This is definitely a patch for net-next, not net tree.

Quoted 17 lines
Quoted 3 lines

Also we test cl being NULL at line 222, it would be nice to not test it again...

Updated below.

 net/sched/sch_htb.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
net/sched/sch_htb.c +0 -0
index 717b210..8073d92 100644
@@ -219,11 +219,15 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch,
 	if (skb->priority == sch->handle)
 		return HTB_DIRECT;	/* X:0 (direct flow) selected */
 	cl = htb_find(skb->priority, sch);

net/sched/sch_htb.c static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch, · line 229

Quoted 10 lines
net/sched/sch_htb.c +7 -2
-	if (cl && cl->level == 0)
-		return cl;
+	if (cl) {
+		if (cl->level == 0)
+			return cl;
+		/* Start with inner filter chain if a non-leaf class is selected */
+		tcf = cl->filter_list;
+	} else
+		tcf = q->filter_list;
 

	} else {
        	tcf = q->filter_list;
	}

(Documentation/CodingStyle line 169)

Quoted 5 lines
net/sched/sch_htb.c +0 -1
 	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
-	tcf = q->filter_list;
 	while (tcf && (result = tc_classify(skb, tcf, &res)) >= 0) {
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {