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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1389969340.31367.494.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Fri, 17 Jan 2014 06:35:40 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Harry Mason <harry.mason@...othwall.net>
Cc:	Jamal Hadi Salim <hadi@...atatu.com>,
	linux-netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v2] sch_htb: let skb->priority refer to non-leaf class

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@...othwall.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) {


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html