[net,v2] net_sched: unset TCQ_F_CAN_BYPASS when adding filters
diff mbox series

Message ID 20190716205730.19675-1-xiyou.wangcong@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • [net,v2] net_sched: unset TCQ_F_CAN_BYPASS when adding filters
Related show

Commit Message

Cong Wang July 16, 2019, 8:57 p.m. UTC
For qdisc's that support TC filters and set TCQ_F_CAN_BYPASS,
notably fq_codel, it makes no sense to let packets bypass the TC
filters we setup in any scenario, otherwise our packets steering
policy could not be enforced.

This can be reproduced easily with the following script:

 ip li add dev dummy0 type dummy
 ifconfig dummy0 up
 tc qd add dev dummy0 root fq_codel
 tc filter add dev dummy0 parent 8001: protocol arp basic action mirred egress redirect dev lo
 tc filter add dev dummy0 parent 8001: protocol ip basic action mirred egress redirect dev lo
 ping -I dummy0 192.168.112.1

Without this patch, packets are sent directly to dummy0 without
hitting any of the filters. With this patch, packets are redirected
to loopback as expected.

This fix is not perfect, it only unsets the flag but does not set it back
because we have to save the information somewhere in the qdisc if we
really want that. Note, both fq_codel and sfq clear this flag in their
->bind_tcf() but this is clearly not sufficient when we don't use any
class ID.

Fixes: 23624935e0c4 ("net_sched: TCQ_F_CAN_BYPASS generalization")
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_api.c      | 1 +
 net/sched/sch_fq_codel.c | 2 --
 net/sched/sch_sfq.c      | 2 --
 3 files changed, 1 insertion(+), 4 deletions(-)

Comments

David Miller July 17, 2019, 7:04 p.m. UTC | #1
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 16 Jul 2019 13:57:30 -0700

> For qdisc's that support TC filters and set TCQ_F_CAN_BYPASS,
> notably fq_codel, it makes no sense to let packets bypass the TC
> filters we setup in any scenario, otherwise our packets steering
> policy could not be enforced.
 ...

Eric I think your feedback was addressed, please review to confirm.

Thank you.
Eric Dumazet July 17, 2019, 7:19 p.m. UTC | #2
On Wed, Jul 17, 2019 at 9:04 PM David Miller <davem@davemloft.net> wrote:
>
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Tue, 16 Jul 2019 13:57:30 -0700
>
> > For qdisc's that support TC filters and set TCQ_F_CAN_BYPASS,
> > notably fq_codel, it makes no sense to let packets bypass the TC
> > filters we setup in any scenario, otherwise our packets steering
> > policy could not be enforced.
>  ...
>
> Eric I think your feedback was addressed, please review to confirm.

Yes, this seems good to me, thanks.

Reviewed-by: Eric Dumazet <edumazet@google.com>
David Miller July 17, 2019, 8:34 p.m. UTC | #3
From: Eric Dumazet <edumazet@google.com>
Date: Wed, 17 Jul 2019 21:19:51 +0200

> On Wed, Jul 17, 2019 at 9:04 PM David Miller <davem@davemloft.net> wrote:
>>
>> From: Cong Wang <xiyou.wangcong@gmail.com>
>> Date: Tue, 16 Jul 2019 13:57:30 -0700
>>
>> > For qdisc's that support TC filters and set TCQ_F_CAN_BYPASS,
>> > notably fq_codel, it makes no sense to let packets bypass the TC
>> > filters we setup in any scenario, otherwise our packets steering
>> > policy could not be enforced.
>>  ...
>>
>> Eric I think your feedback was addressed, please review to confirm.
> 
> Yes, this seems good to me, thanks.
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Great, applied and queued up for -stable.

Patch
diff mbox series

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 278014e26aec..d144233423c5 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2152,6 +2152,7 @@  static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 		tfilter_notify(net, skb, n, tp, block, q, parent, fh,
 			       RTM_NEWTFILTER, false, rtnl_held);
 		tfilter_put(tp, fh);
+		q->flags &= ~TCQ_F_CAN_BYPASS;
 	}
 
 errout:
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index e2faf33d282b..d59fbcc745d1 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -596,8 +596,6 @@  static unsigned long fq_codel_find(struct Qdisc *sch, u32 classid)
 static unsigned long fq_codel_bind(struct Qdisc *sch, unsigned long parent,
 			      u32 classid)
 {
-	/* we cannot bypass queue discipline anymore */
-	sch->flags &= ~TCQ_F_CAN_BYPASS;
 	return 0;
 }
 
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 420bd8411677..68404a9d2ce4 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -824,8 +824,6 @@  static unsigned long sfq_find(struct Qdisc *sch, u32 classid)
 static unsigned long sfq_bind(struct Qdisc *sch, unsigned long parent,
 			      u32 classid)
 {
-	/* we cannot bypass queue discipline anymore */
-	sch->flags &= ~TCQ_F_CAN_BYPASS;
 	return 0;
 }