| Message ID | 20190712201749.28421-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net_sched: unset TCQ_F_CAN_BYPASS when adding filters | expand |
On 7/12/19 10:17 PM, Cong Wang wrote: > 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 easily reproduced 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. > > Fixes: 4b549a2ef4be ("fq_codel: Fair Queue Codel AQM") > Cc: Eric Dumazet <edumazet@google.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > net/sched/cls_api.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 638c1bc1ea1b..5c800b0c810b 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: > Strange, because sfq and fq_codel are roughly the same for TCQ_F_CAN_BYPASS handling. Why is fq_codel_bind() not effective ? If not effective, sfq had the same issue, so the Fixes: tag needs to be refined, maybe to commit 23624935e0c4 net_sched: TCQ_F_CAN_BYPASS generalization
On Sat, Jul 13, 2019 at 5:54 AM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 7/12/19 10:17 PM, Cong Wang wrote: > > 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 easily reproduced 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. > > > > Fixes: 4b549a2ef4be ("fq_codel: Fair Queue Codel AQM") > > Cc: Eric Dumazet <edumazet@google.com> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > --- > > net/sched/cls_api.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > > index 638c1bc1ea1b..5c800b0c810b 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: > > > > Strange, because sfq and fq_codel are roughly the same for TCQ_F_CAN_BYPASS handling. > > Why is fq_codel_bind() not effective ? Because I don't have class id set in the filter. > > If not effective, sfq had the same issue, so the Fixes: tag needs to be refined, > maybe to commit 23624935e0c4 net_sched: TCQ_F_CAN_BYPASS generalization > Yeah, I think it is probably a better commit here. Thanks.
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 638c1bc1ea1b..5c800b0c810b 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:
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 easily reproduced 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. Fixes: 4b549a2ef4be ("fq_codel: Fair Queue Codel AQM") Cc: Eric Dumazet <edumazet@google.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/sched/cls_api.c | 1 + 1 file changed, 1 insertion(+)