[Patch net] net_sched: unset TCQ_F_CAN_BYPASS when adding filters

[Patch net] net_sched: unset TCQ_F_CAN_BYPASS when adding filters

[Cong Wang]

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:
-- 
2.21.0

Re: [Patch net] net_sched: unset TCQ_F_CAN_BYPASS when adding filters

[Eric Dumazet]

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

Re: [Patch net] net_sched: unset TCQ_F_CAN_BYPASS when adding filters

[Cong Wang]

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.