From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [Patch net] net_sched: refetch skb protocol for each filter Date: Mon, 14 Jan 2019 10:23:15 +0100 Message-ID: References: <20190112025542.397-1-xiyou.wangcong@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Martin Olsson , Jamal Hadi Salim , Jiri Pirko To: Cong Wang , netdev@vger.kernel.org Return-path: Received: from www62.your-server.de ([213.133.104.62]:33308 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726449AbfANJXS (ORCPT ); Mon, 14 Jan 2019 04:23:18 -0500 In-Reply-To: <20190112025542.397-1-xiyou.wangcong@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 01/12/2019 03:55 AM, Cong Wang wrote: > Martin reported a set of filters don't work after changing > from reclassify to continue. Looking into the code, it > looks like skb protocol is not always fetched for each > iteration of the filters. But, as demonstrated by Martin, > TC actions could modify skb->protocol, for example act_vlan, > this means we have to refetch skb protocol in each iteration, > rather than using the one we fetch in the beginning of the loop. > > This bug is _not_ introduced by commit 3b3ae880266d > ("net: sched: consolidate tc_classify{,_compat}"), technically, > if act_vlan is the only action that modifies skb protocol, then > it is commit c7e2b9689ef8 ("sched: introduce vlan action") which > introduced this bug. > > Reported-by: Martin Olsson > Cc: Jamal Hadi Salim > Cc: Jiri Pirko > Signed-off-by: Cong Wang > --- > net/sched/cls_api.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 8ce2a0507970..e2b5cb2eb34e 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -1277,7 +1277,6 @@ EXPORT_SYMBOL(tcf_block_cb_unregister); > int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp, > struct tcf_result *res, bool compat_mode) > { > - __be16 protocol = tc_skb_protocol(skb); > #ifdef CONFIG_NET_CLS_ACT > const int max_reclassify_loop = 4; > const struct tcf_proto *orig_tp = tp; > @@ -1287,6 +1286,7 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp, > reclassify: > #endif > for (; tp; tp = rcu_dereference_bh(tp->next)) { > + __be16 protocol = tc_skb_protocol(skb); > int err; > > if (tp->protocol != protocol && Can't we do something like the below instead? Otherwise we'll needlessly refetch protocol every time there is a mismatch in above tp->protocol check as well. diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 8ce2a05..dc725a1 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -1305,6 +1305,11 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp, #endif if (err >= 0) return err; + + /* We also need to refetch protocol from here as e.g. + * act_vlan could have changed it. + */ + protocol = tc_skb_protocol(skb); } return TC_ACT_UNSPEC; /* signal: continue lookup */ Thanks, Daniel