[Patch net] net_sched: refetch skb protocol for each filter

[Patch net] net_sched: refetch skb protocol for each filter

[Cong Wang]

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 <martin.olsson+netdev@sentorsecurity.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 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 &&
@@ -1319,7 +1319,6 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 	}
 
 	tp = first_tp;
-	protocol = tc_skb_protocol(skb);
 	goto reclassify;
 #endif
 }
-- 
2.20.1

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Jamal Hadi Salim]

On 2019-01-11 9:55 p.m., 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.
> 

+Cc Lucas
Do we have a test case for a setup like this in tdc?
i.e incoming tagged and then vlan popped by action?
Otherwise a test with IFE which resets the ethertype
would be sufficient i.e just something that will messup
with skb->protocol.
Cong,
I am a little worried about the impact of this change. Smells
more like it has to do with Vlan action or related issues
than with reclassifying.
Martin,
did this setup use to work before? If yes, rough idea of
which kernels?

cheers,
jamal

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Jamal Hadi Salim]

On 2019-01-12 7:23 a.m., Jamal Hadi Salim wrote:

> Do we have a test case for a setup like this in tdc?
> i.e incoming tagged and then vlan popped by action?
> Otherwise a test with IFE which resets the ethertype
> would be sufficient i.e just something that will messup
> with skb->protocol.

And here is a slightly complex test with IFE.
Wanted to show both reclassify and continue in play
as well as something that change skb->protocol.

I used VMs to test, but two hosts or two containers
with veth between them will do.
Martin, if you have the cycles please test this out.

TLDR: Suspect is the vlan part.

-----------
#Sender side egress setup. We are going to encode
#all outgoing pings(icmp) to carry an skbmark 17 on the wire
#and set the ethertype  to 0xDEAD
sudo $TC qdisc del dev $ETH root
sudo $TC qdisc add dev $ETH root handle 1: prio
sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 u32 \
match ip protocol 1 0xff \
flowid 1:2 \
action skbedit mark 17 \
action ife encode type 0xDEAD allow mark

#receiver side ingress setup

sudo $TC qdisc del dev $ETH ingress
sudo $TC qdisc add dev $ETH ingress
#decapsulate based on skb proto and then
#reclassify
sudo $TC filter add dev $ETH parent ffff: prio 2 protocol 0xdead u32 \
match u32 0 0 flowid 1:1 action ife decode reclassify
#match icmp
sudo $TC filter add dev $ETH parent ffff: prio 4 protocol ip u32 \
match ip protocol 1 0xff flowid 1:1 action continue
#
#now classify based on mark 17 (hex 11).
#
sudo $TC filter add dev $ETH parent ffff: prio 5 protocol ip \
handle 0x11 fw flowid 1:1 \
action ok

On sender side, send 10 pings to receiver. Now lets see
the receiver stats:

---------------
root@jhs-foobar:# uname -a
Linux jhs-foobar 4.20.0-rc7+ #2 SMP Fri Jan 12 9:29:35 EST 2019 x86_64 
x86_64 x86_64 GNU/Linux

root@jhs-foobar:# $TC -s filter ls dev $ETH parent ffff:
filter protocol [57005] pref 2 u32
filter protocol [57005] pref 2 u32 fh 800: ht divisor 1
filter protocol [57005] pref 2 u32 fh 800::800 order 2048 key ht 800 bkt 
0 flowid 1:1  (rule hit 10 success 10)
   match 00000000/00000000 at 0 (success 10 )
	action order 1: ife decode action reclassify type 0x0
	 allow mark allow prio
	 index 1 ref 1 bind 1 installed 40 sec used 19 sec
	Action statistics:
	Sent 1080 bytes 10 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0

filter protocol ip pref 4 u32
filter protocol ip pref 4 u32 fh 801: ht divisor 1
filter protocol ip pref 4 u32 fh 801::800 order 2048 key ht 801 bkt 0 
flowid 1:1  (rule hit 33 success 10)
   match 00010000/00ff0000 at 8 (success 10 )
	action order 1: gact action continue
	 random type none pass val 0
	 index 1 ref 1 bind 1 installed 40 sec used 19 sec
  	Action statistics:
	Sent 1080 bytes 10 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0

filter protocol ip pref 5 fw
filter protocol ip pref 5 fw handle 0x11 classid 1:1

	action order 1: gact action pass
	 random type none pass val 0
	 index 2 ref 1 bind 1 installed 39 sec used 19 sec
  	Action statistics:
	Sent 1080 bytes 10 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0
---------------


As can be observed all the rules are hit as expected.

cheers,
jamal

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Cong Wang]

On Sat, Jan 12, 2019 at 7:41 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On 2019-01-12 7:23 a.m., Jamal Hadi Salim wrote:
>
> > Do we have a test case for a setup like this in tdc?
> > i.e incoming tagged and then vlan popped by action?
> > Otherwise a test with IFE which resets the ethertype
> > would be sufficient i.e just something that will messup
> > with skb->protocol.
>
> And here is a slightly complex test with IFE.
> Wanted to show both reclassify and continue in play
> as well as something that change skb->protocol.
>

I don't know why you need a complex one, Martin's
test case is pretty simple (as I already sent to you).

Also, you can add two printk()'s around the skb_vlan_pop()
in tcf_vlan_act() to see the difference of tc_skb_protocol()
return values before and after. I tried, it clearly shows
ETH_P_8021Q and ETH_P_IP.

Of course, it could be tc_skb_protocol() which is wrong,
as skb->protocol stays same.

This patch is always correct despite of tc_skb_protocol():

1. If tc_skb_protocol() is wrong, this patch fixes nothing,
and harms nothing.

2. If tc_skb_protocol() is correct, this patch fixes a bug.

Changing tc_skb_protocol() is much more risky than this
patch. You fixed a very similar bug before:

commit 619fe32640b4b01f370574d50344ae0f62689816
Author: Jamal Hadi Salim <jhs@mojatatu.com>
Date:   Thu Feb 18 07:38:04 2016 -0500

    net_sched fix: reclassification needs to consider ether protocol changes

which also implies tc_skb_protocol() is correct.

Thanks.

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Jamal Hadi Salim]

On 2019-01-13 4:08 p.m., Cong Wang wrote:
> On Sat, Jan 12, 2019 at 7:41 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>> On 2019-01-12 7:23 a.m., Jamal Hadi Salim wrote:
>>
>>> Do we have a test case for a setup like this in tdc?
>>> i.e incoming tagged and then vlan popped by action?
>>> Otherwise a test with IFE which resets the ethertype
>>> would be sufficient i.e just something that will messup
>>> with skb->protocol.
>>
>> And here is a slightly complex test with IFE.
>> Wanted to show both reclassify and continue in play
>> as well as something that change skb->protocol.
>>
> 
> I don't know why you need a complex one, Martin's
> test case is pretty simple (as I already sent to you).
> 

Martin example had a "reclassify" with the vlan pop.
Your example was showing a "continue"
Note: "Reclassify" is typical how we have explained
to users to deal with skb->protocol changes.

I wanted to demonstrate that if i have a rule that followed that
rule  (as Martin did) with something other than vlan it would work:

match u32 protocol someethertype .... action change vl reclassify
match u32 protocol ip ... action continue
match u32 protocol ip ... action blah

infact that works before your patch. Basically it was an exercise
to reduce _the variables_ from knowing that reclassify/continue
do/nt work in presence of changing skb->protocol.
IMO that example demonstrated that the current skb->protocol
logic works and the vlan action maybe the guilty party.

> Also, you can add two printk()'s around the skb_vlan_pop()
> in tcf_vlan_act() to see the difference of tc_skb_protocol()
> return values before and after. I tried, it clearly shows
> ETH_P_8021Q and ETH_P_IP.
> 
> Of course, it could be tc_skb_protocol() which is wrong,
> as skb->protocol stays same.
> 
> This patch is always correct despite of tc_skb_protocol():
> 
> 1. If tc_skb_protocol() is wrong, this patch fixes nothing,
> and harms nothing.
> 
> 2. If tc_skb_protocol() is correct, this patch fixes a bug.
> 
> Changing tc_skb_protocol() is much more risky than this
> patch. You fixed a very similar bug before:
> 
> commit 619fe32640b4b01f370574d50344ae0f62689816
> Author: Jamal Hadi Salim <jhs@mojatatu.com>
> Date:   Thu Feb 18 07:38:04 2016 -0500
> 
>      net_sched fix: reclassification needs to consider ether protocol changes
> 
> which also implies tc_skb_protocol() is correct.

I agree with your rationale.
Lets have Lucas give this a run and we can move
forward.

cheers,
jamal

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Jamal Hadi Salim]

On 2019-01-15 10:59 a.m., Jamal Hadi Salim wrote:
> On 2019-01-13 4:08 p.m., Cong Wang wrote:
>> On Sat, Jan 12, 2019 at 7:41 AM Jamal Hadi Salim <jhs@mojatatu.com> 

[..]
> I agree with your rationale.
> Lets have Lucas give this a run and we can move
> forward.

And as an addendum - Martin corrected me, he did have a "continue"
after the vlan pop (not illegal to do). So i think your patch
is fine; lets not mix it up with vlan not correctly matching
(two separate issues).

Lets have Lucas create the tdc test, then add my Ack.

cheers,
jamal

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Cong Wang]

On Tue, Jan 15, 2019 at 8:21 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On 2019-01-15 10:59 a.m., Jamal Hadi Salim wrote:
> > On 2019-01-13 4:08 p.m., Cong Wang wrote:
> >> On Sat, Jan 12, 2019 at 7:41 AM Jamal Hadi Salim <jhs@mojatatu.com>
>
> [..]
> > I agree with your rationale.
> > Lets have Lucas give this a run and we can move
> > forward.
>
> And as an addendum - Martin corrected me, he did have a "continue"
> after the vlan pop (not illegal to do). So i think your patch
> is fine; lets not mix it up with vlan not correctly matching
> (two separate issues).
>
> Lets have Lucas create the tdc test, then add my Ack.

As I said, this case is a bit hard to add to tdc test, because:

1. It requires a vlan device on top. This is not hard to do, but still
needs some extra work.

2. It requires a run-time test, like ping. This is different from other
test cases which test setup-time config's. This is a bit hard because
we have to parse tc filter stats to learn if some filter is hit or not.

I am sure Lucas can do it, just it is not as easy as other cases.

Thanks.

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Jamal Hadi Salim]

On 2019-01-15 12:57 p.m., Cong Wang wrote:
> On Tue, Jan 15, 2019 at 8:21 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>

>>
>> Lets have Lucas create the tdc test, then add my Ack.
> 
> As I said, this case is a bit hard to add to tdc test, because:
> 
> 1. It requires a vlan device on top. This is not hard to do, but still
> needs some extra work.
> 
> 2. It requires a run-time test, like ping. This is different from other
> test cases which test setup-time config's. This is a bit hard because
> we have to parse tc filter stats to learn if some filter is hit or not.
> 
> I am sure Lucas can do it, just it is not as easy as other cases.

Lucas has confirmed to me he manually tested it (I just Acked).
Yes, it is tricky to add this test case but he will spend time
to build the infrastructure and submit this test as first user.
We discussed and the stats scrapping is doable with python and
the -j tc option.

cheers,
jamal

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Cong Wang]

On Tue, Jan 15, 2019 at 7:59 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On 2019-01-13 4:08 p.m., Cong Wang wrote:
> > On Sat, Jan 12, 2019 at 7:41 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >>
> >> On 2019-01-12 7:23 a.m., Jamal Hadi Salim wrote:
> >>
> >>> Do we have a test case for a setup like this in tdc?
> >>> i.e incoming tagged and then vlan popped by action?
> >>> Otherwise a test with IFE which resets the ethertype
> >>> would be sufficient i.e just something that will messup
> >>> with skb->protocol.
> >>
> >> And here is a slightly complex test with IFE.
> >> Wanted to show both reclassify and continue in play
> >> as well as something that change skb->protocol.
> >>
> >
> > I don't know why you need a complex one, Martin's
> > test case is pretty simple (as I already sent to you).
> >
>
> Martin example had a "reclassify" with the vlan pop.
> Your example was showing a "continue"
> Note: "Reclassify" is typical how we have explained
> to users to deal with skb->protocol changes.


You have to re-read Martin's example, he clearly showed
"reclassify" works but not "continue". This is exactly why
I have "continue" in my script, as I need to test what I fix.


> Lets have Lucas give this a run and we can move
> forward.

Sure.

Thanks.

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Cong Wang]

On Sat, Jan 12, 2019 at 4:23 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> Cong,
> I am a little worried about the impact of this change. Smells
> more like it has to do with Vlan action or related issues
> than with reclassifying.

You can verify this patch with Martin's test case. I verified it
with the script below, it works as expected.

Not sure if it is feasible to add it to tc-tests as it needs a vlan
device on top of veth.


ip netns add test1
ip netns add test2
ip link add dev veth1 type veth peer name veth2
ip link set dev veth1 netns test1 up
ip link set dev veth2 netns test2 up
ip netns exec test1 ifconfig veth1 192.168.1.1 netmask 255.255.255.0
ip netns exec test2 ifconfig veth2 192.168.1.2 netmask 255.255.255.0
ip netns exec test1 ip link add link veth1 name veth1.100 type vlan id 100
ip netns exec test2 ip link add link veth2 name veth2.100 type vlan id 100
ip netns exec test1 ip link set link dev veth1.100 up
ip netns exec test2 ip link set link dev veth2.100 up
ip netns exec test1 ifconfig veth1.100 192.168.100.1 netmask 255.255.255.0
ip netns exec test2 ifconfig veth2.100 192.168.100.2 netmask 255.255.255.0
ip netns exec test2 ip link set dev veth2 address 00:c0:7b:7d:00:c8
ip netns exec test1 ip neighbor add 192.168.100.2 lladdr
00:c0:7b:7d:00:c8 dev veth1.100 nud permanent

ip netns exec test1 tc qdisc add dev veth1 clsact
ip netns exec test1 tc filter add dev veth1 egress prio 100  protocol
802.1Q  matchall action vlan pop continue #reclassify
ip netns exec test1 tc filter add dev veth1 egress prio 200  protocol
ip      u32 match ip src 192.168.1.0/24  action drop
ip netns exec test1 tc filter add dev veth1 egress prio 201  protocol
ip      u32 match ip dst 192.168.100.0/24  action drop



Thanks.

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Jamal Hadi Salim]

On 2019-01-13 3:58 p.m., Cong Wang wrote:
> On Sat, Jan 12, 2019 at 4:23 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> Cong,
>> I am a little worried about the impact of this change. Smells
>> more like it has to do with Vlan action or related issues
>> than with reclassifying.
> 
> You can verify this patch with Martin's test case. I verified it
> with the script below, it works as expected.
> 

Your script is slightly different from his. Probably doesnt matter
in the bigger picture - but lets give Lucas a chance to test.
So a ping from test1->test2 should be sufficient here, yes?

cheers,
jamal

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Martin Olsson]

> "did this setup use to work before? If yes, rough idea of which 
> kernels?"

Unknown. I haven't tried it before.

/Martin


On Sat, 12 Jan 2019, Jamal Hadi Salim wrote:

> On 2019-01-11 9:55 p.m., 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.
>> 
>
> +Cc Lucas
> Do we have a test case for a setup like this in tdc?
> i.e incoming tagged and then vlan popped by action?
> Otherwise a test with IFE which resets the ethertype
> would be sufficient i.e just something that will messup
> with skb->protocol.
> Cong,
> I am a little worried about the impact of this change. Smells
> more like it has to do with Vlan action or related issues
> than with reclassifying.
> Martin,
> did this setup use to work before? If yes, rough idea of
> which kernels?
>
> cheers,
> jamal
>
>

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Jamal Hadi Salim]

On 2019-01-14 4:15 a.m., Martin Olsson wrote:
> 
>> "did this setup use to work before? If yes, rough idea of which kernels?"
> 
> Unknown. I haven't tried it before.
> 

Ok, IMO we are dealing with vlan breakage.

cheers,
jamal

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Lucas Bates]

On Sat, Jan 12, 2019 at 7:23 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On 2019-01-11 9:55 p.m., 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.
> >
>
> +Cc Lucas
> Do we have a test case for a setup like this in tdc?
> i.e incoming tagged and then vlan popped by action?
> Otherwise a test with IFE which resets the ethertype
> would be sufficient i.e just something that will messup
> with skb->protocol.

Sorry, I've been a little caught up in things around here.

There isn't currently a test case like this, but I can look at the
example Cong provided and see if I can make it tdc-friendly.

- Lucas

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Daniel Borkmann]

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 <martin.olsson+netdev@sentorsecurity.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  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

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Cong Wang]

On Mon, Jan 14, 2019 at 1:23 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> 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.

Does this save anything given the fact we simply return if err>=0?

The protocol must be fetched, either before the loop or inside the loop, before
this "if err>=0". If we return, it won't be refetched even with my
patch. If we don't,
it has to be refetched again. So, I don't see any difference here.

Re: [Patch net] net_sched: refetch skb protocol for each filter

[Jamal Hadi Salim]

On 2019-01-11 9:55 p.m., 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 <martin.olsson+netdev@sentorsecurity.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal