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

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

[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 <harry.mason@smoothwall.net>
---
 net/sched/sch_htb.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 717b210..e50ab65 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -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) {
-- 
1.7.10.4

Re: [PATCH] sch_htb: let skb->priority refer to non-leaf class

[Eric Dumazet]

On Thu, 2014-01-16 at 14:45 +0000, Harry Mason wrote:
> 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 <harry.mason@smoothwall.net>
> ---
>  net/sched/sch_htb.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
> index 717b210..e50ab65 100644
> --- a/net/sched/sch_htb.c
> +++ b/net/sched/sch_htb.c
> @@ -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;
> +

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...

[PATCH v2] sch_htb: let skb->priority refer to non-leaf class

[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 <harry.mason@smoothwall.net>
---

On Thu, 2014-01-16 at 08:25 -0800, Eric Dumazet wrote:
> On Thu, 2014-01-16 at 14:45 +0000, Harry Mason wrote:
> 
>> + /* Start with inner filter chain if a non-leaf class is selected */
>> + if (cl)
>> +     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.

> 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(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 717b210..8073d92 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -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) {
-- 
1.7.10.4

Re: [PATCH v2] sch_htb: let skb->priority refer to non-leaf class

[Sergei Shtylyov]

Hello.

On 17-01-2014 14:19, Harry Mason wrote:

> 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 <harry.mason@smoothwall.net>
> ---

> On Thu, 2014-01-16 at 08:25 -0800, Eric Dumazet wrote:
>> On Thu, 2014-01-16 at 14:45 +0000, Harry Mason wrote:

>>> + /* Start with inner filter chain if a non-leaf class is selected */
>>> + if (cl)
>>> +     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.

>> 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(-)

> diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
> index 717b210..8073d92 100644
> --- a/net/sched/sch_htb.c
> +++ b/net/sched/sch_htb.c
> @@ -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;

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

WBR, Sergei

[PATCH v3] sch_htb: let skb->priority refer to non-leaf class

[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 <harry.mason@smoothwall.net>
---

On Fri, 2014-01-17 at 16:53 +0400, Sergei Shtylyov wrote:
> On 17-01-2014 14:19, Harry Mason wrote:
> > +	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(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 717b210..e9c8c08 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -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) {
-- 
1.7.10.4

Re: [PATCH v2] sch_htb: let skb->priority refer to non-leaf class

[Eric Dumazet]

On Fri, 2014-01-17 at 10:19 +0000, Harry Mason wrote:
> 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 <harry.mason@smoothwall.net>
> ---
> 
> On Thu, 2014-01-16 at 08:25 -0800, Eric Dumazet wrote:
> > On Thu, 2014-01-16 at 14:45 +0000, Harry Mason wrote:
> > 
> >> + /* Start with inner filter chain if a non-leaf class is selected */
> >> + if (cl)
> >> +     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.
> 

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)

> 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.

> 
> > 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(-)
> 
> diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
> index 717b210..8073d92 100644
> --- a/net/sched/sch_htb.c
> +++ b/net/sched/sch_htb.c
> @@ -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;
>  

	} else {
        	tcf = q->filter_list;
	}

(Documentation/CodingStyle line 169)

>  	*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) {

Re: [PATCH v3] sch_htb: let skb->priority refer to non-leaf class

[David Miller]

From: Harry Mason <harry.mason@smoothwall.net>
Date: Fri, 17 Jan 2014 13:22:32 +0000

> 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 <harry.mason@smoothwall.net>

Can I please get some reviews of this patch?

Thanks.

Re: [PATCH v3] sch_htb: let skb->priority refer to non-leaf class

[Jamal Hadi Salim]

Looks reasonable from my view.

cheers,
jamal

On Tue, Jan 21, 2014 at 5:36 PM, David Miller <davem@davemloft.net> wrote:
> From: Harry Mason <harry.mason@smoothwall.net>
> Date: Fri, 17 Jan 2014 13:22:32 +0000
>
>> 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 <harry.mason@smoothwall.net>
>
> Can I please get some reviews of this patch?
>
> Thanks.

Re: [PATCH v3] sch_htb: let skb->priority refer to non-leaf class

[Eric Dumazet]

On Fri, 2014-01-17 at 13:22 +0000, Harry Mason wrote:
> 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 <harry.mason@smoothwall.net>
> ---

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v3] sch_htb: let skb->priority refer to non-leaf class

[David Miller]

From: Harry Mason <harry.mason@smoothwall.net>
Date: Fri, 17 Jan 2014 13:22:32 +0000

> 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 <harry.mason@smoothwall.net>

Applied, thank you.