Question about TC filter

Question about TC filter

[satish dhote]

Hi Team,

I have a question regarding tc filter behavior. I tried to look
for the answer over the web and netdev FAQ but didn't get the
answer. Hence I'm looking for your help.

I added ingress qdisc for interface enp0s25 and then configured the
tc filter as shown below, but after adding filters I realize that
rule is reflected as a result of both ingress and egress filter
command?  Is this the expected behaviour? or a bug? Why should the
same filter be reflected in both ingress and egress path?

I understand that policy is always configured for ingress traffic,
so I believe that filters should not be reflected with egress.
Behaviour is same when I offloaded ovs flow to the tc software
datapath.

Please advise or redirect me to the right channel if this is not
the right place for this question. Below are the executed tc
commands:

tc qdisc add dev enp0s25 ingress

tc -g qdisc show dev enp0s25
qdisc fq_codel 0: root refcnt 2 limit 10240p flows 1024 quantum 1514
target 5.0ms interval 100.0ms memory_limit 32Mb ecn
qdisc ingress ffff: parent ffff:fff1 ----------------

tc filter add dev enp0s25 protocol ip parent ffff: prio 1 flower
dst_ip 192.168.1.1/0.0.0.0 ip_proto tcp skip_hw action drop

tc filter show dev enp0s25 ingress
filter parent ffff: protocol ip pref 1 flower chain 0
filter parent ffff: protocol ip pref 1 flower chain 0 handle 0x1
  eth_type ipv4
  ip_proto tcp
  skip_hw
  not_in_hw
action order 1: gact action drop
random type none pass val 0
index 1 ref 1 bind 1

tc filter show dev enp0s25 egress   (Shows duplicate flows as above)
filter parent ffff: protocol ip pref 1 flower chain 0
filter parent ffff: protocol ip pref 1 flower chain 0 handle 0x1
  eth_type ipv4
  ip_proto tcp
  skip_hw
  not_in_hw
action order 1: gact action drop
random type none pass val 0
index 1 ref 1 bind 1

Thanks
Satish

Re: Question about TC filter

[Jakub Kicinski]

On Wed, 5 Aug 2020 11:08:08 +0530 satish dhote wrote:
> Hi Team,
> 
> I have a question regarding tc filter behavior. I tried to look
> for the answer over the web and netdev FAQ but didn't get the
> answer. Hence I'm looking for your help.
> 
> I added ingress qdisc for interface enp0s25 and then configured the
> tc filter as shown below, but after adding filters I realize that
> rule is reflected as a result of both ingress and egress filter
> command?  Is this the expected behaviour? or a bug? Why should the
> same filter be reflected in both ingress and egress path?
> 
> I understand that policy is always configured for ingress traffic,
> so I believe that filters should not be reflected with egress.
> Behaviour is same when I offloaded ovs flow to the tc software
> datapath.

I feel like this was discussed and perhaps fixed by:

a7df4870d79b ("net_sched: fix tcm_parent in tc filter dump")

What's your kernel version?

Re: Question about TC filter

[satish dhote]

Hi Jakub,

Thanks for your response. Below are the OS and Kernel version I'm using.

OS: Ubuntu 20.04.1 LTS
Kernel Version: 5.4.0-42-generic

Thanks
Satish

On Wed, Aug 5, 2020 at 10:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 5 Aug 2020 11:08:08 +0530 satish dhote wrote:
> > Hi Team,
> >
> > I have a question regarding tc filter behavior. I tried to look
> > for the answer over the web and netdev FAQ but didn't get the
> > answer. Hence I'm looking for your help.
> >
> > I added ingress qdisc for interface enp0s25 and then configured the
> > tc filter as shown below, but after adding filters I realize that
> > rule is reflected as a result of both ingress and egress filter
> > command?  Is this the expected behaviour? or a bug? Why should the
> > same filter be reflected in both ingress and egress path?
> >
> > I understand that policy is always configured for ingress traffic,
> > so I believe that filters should not be reflected with egress.
> > Behaviour is same when I offloaded ovs flow to the tc software
> > datapath.
>
> I feel like this was discussed and perhaps fixed by:
>
> a7df4870d79b ("net_sched: fix tcm_parent in tc filter dump")
>
> What's your kernel version?

Re: Question about TC filter

[Cong Wang]

Hello,

On Tue, Aug 4, 2020 at 10:39 PM satish dhote <sdhote926@gmail.com> wrote:
>
> Hi Team,
>
> I have a question regarding tc filter behavior. I tried to look
> for the answer over the web and netdev FAQ but didn't get the
> answer. Hence I'm looking for your help.
>
> I added ingress qdisc for interface enp0s25 and then configured the
> tc filter as shown below, but after adding filters I realize that
> rule is reflected as a result of both ingress and egress filter
> command?  Is this the expected behaviour? or a bug? Why should the
> same filter be reflected in both ingress and egress path?

I am not very sure but I am feeling this is a bug. Let's Cc Daniel who
introduced this.

With the introduction of clsact qdisc, the keywords "ingress" and
"egress" were introduced too to match clsact qdisc. However, its
major/minor handle is kinda confusing:

TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_INGRESS);
TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_EGRESS);
#define TC_H_CLSACT TC_H_INGRESS

They could match the ingress qdisc (ffff:) too.


>
> I understand that policy is always configured for ingress traffic,
> so I believe that filters should not be reflected with egress.
> Behaviour is same when I offloaded ovs flow to the tc software
> datapath.

I believe so too, otherwise it would be too confusing to users.

If you are able to test kernel patch, does the following one-line
change fix this problem? If not, I will try it on my side.

diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index 84838128b9c5..4d9c1bb15545 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -49,7 +49,7 @@ static struct tcf_block *ingress_tcf_block(struct
Qdisc *sch, unsigned long cl,
 {
        struct ingress_sched_data *q = qdisc_priv(sch);

-       return q->block;
+       return cl == 0 ? q->block : NULL;
 }

 static void clsact_chain_head_change(struct tcf_proto *tp_head, void *priv)


Thanks.

Re: Question about TC filter

[satish dhote]

Hi Cong,

I tried adding below patch i.e. "return cl == 0 ? q->block : NULL;"
but after this I'm not able to see any output using "tc filter show... "
command. Looks like the filter is not getting configured.

If this is a bug, then do I need to file a new ticket for this?

Thanks
Satish

On Thu, Aug 6, 2020 at 5:36 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> Hello,
>
> On Tue, Aug 4, 2020 at 10:39 PM satish dhote <sdhote926@gmail.com> wrote:
> >
> > Hi Team,
> >
> > I have a question regarding tc filter behavior. I tried to look
> > for the answer over the web and netdev FAQ but didn't get the
> > answer. Hence I'm looking for your help.
> >
> > I added ingress qdisc for interface enp0s25 and then configured the
> > tc filter as shown below, but after adding filters I realize that
> > rule is reflected as a result of both ingress and egress filter
> > command?  Is this the expected behaviour? or a bug? Why should the
> > same filter be reflected in both ingress and egress path?
>
> I am not very sure but I am feeling this is a bug. Let's Cc Daniel who
> introduced this.
>
> With the introduction of clsact qdisc, the keywords "ingress" and
> "egress" were introduced too to match clsact qdisc. However, its
> major/minor handle is kinda confusing:
>
> TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_INGRESS);
> TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_EGRESS);
> #define TC_H_CLSACT TC_H_INGRESS
>
> They could match the ingress qdisc (ffff:) too.
>
>
> >
> > I understand that policy is always configured for ingress traffic,
> > so I believe that filters should not be reflected with egress.
> > Behaviour is same when I offloaded ovs flow to the tc software
> > datapath.
>
> I believe so too, otherwise it would be too confusing to users.
>
> If you are able to test kernel patch, does the following one-line
> change fix this problem? If not, I will try it on my side.
>
> diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
> index 84838128b9c5..4d9c1bb15545 100644
> --- a/net/sched/sch_ingress.c
> +++ b/net/sched/sch_ingress.c
> @@ -49,7 +49,7 @@ static struct tcf_block *ingress_tcf_block(struct
> Qdisc *sch, unsigned long cl,
>  {
>         struct ingress_sched_data *q = qdisc_priv(sch);
>
> -       return q->block;
> +       return cl == 0 ? q->block : NULL;
>  }
>
>  static void clsact_chain_head_change(struct tcf_proto *tp_head, void *priv)
>
>
> Thanks.

Re: Question about TC filter

[Cong Wang]

On Thu, Aug 6, 2020 at 10:21 AM satish dhote <sdhote926@gmail.com> wrote:
>
> Hi Cong,
>
> I tried adding below patch i.e. "return cl == 0 ? q->block : NULL;"
> but after this I'm not able to see any output using "tc filter show... "
> command. Looks like the filter is not getting configured.

What exact command did you use? If you specify "ingress" rather
than "ffff:", it is exactly _my_ goal, because prior to clsact, only
"ffff:" could match ingress qdisc, the keyword "ingress" did not
even exist.

Of course, it may not be Daniel's intention, he might expect
"ingress" to match ingress qdisc too for convenience.

Thanks.

Re: Question about TC filter

[satish dhote]

Sorry for responding late. I got engaged with other work.
I didn't replace ingress with ffff

To do more experiments, I will have to recompile everything
from fresh as I lost that setup where I was experimenting with
tc utility.

But if we can get inputs from Daniel's about this change,
it will help us to figure out whether this was really a bug or
expected behavior.

Thanks.

On Fri, Aug 7, 2020 at 12:05 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Thu, Aug 6, 2020 at 10:21 AM satish dhote <sdhote926@gmail.com> wrote:
> >
> > Hi Cong,
> >
> > I tried adding below patch i.e. "return cl == 0 ? q->block : NULL;"
> > but after this I'm not able to see any output using "tc filter show... "
> > command. Looks like the filter is not getting configured.
>
> What exact command did you use? If you specify "ingress" rather
> than "ffff:", it is exactly _my_ goal, because prior to clsact, only
> "ffff:" could match ingress qdisc, the keyword "ingress" did not
> even exist.
>
> Of course, it may not be Daniel's intention, he might expect
> "ingress" to match ingress qdisc too for convenience.
>
> Thanks.