[PATCH net] net: sched: sch_ingress: do not report ingress filter info in egress path

[PATCH net] net: sched: sch_ingress: do not report ingress filter info in egress path

[Lorenzo Bianconi]

Currently if we add a filter to the ingress qdisc (e.g matchall) the
filter data are reported even in the egress path. The issue can be
triggered with the following reproducer:

$tc qdisc add dev lo ingress
$tc filter add dev lo ingress matchall action ok
$tc filter show dev lo ingress
filter protocol all pref 49152 matchall chain 0
filter protocol all pref 49152 matchall chain 0 handle 0x1
  not_in_hw
	action order 1: gact action pass
		 random type none pass val 0
		 	 index 1 ref 1 bind 1

$tc filter show dev lo egress
filter protocol all pref 49152 matchall chain 0
filter protocol all pref 49152 matchall chain 0 handle 0x1
  not_in_hw
	action order 1: gact action pass
		 random type none pass val 0
		 	 index 1 ref 1 bind 1

Fix it reporting NULL for non-ingress filters in ingress_tcf_block
routine

Fixes: 6529eaba33f0 ("net: sched: introduce tcf block infrastructure")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 net/sched/sch_ingress.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index 0bac926b46c7..1825347fed3a 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -31,7 +31,7 @@ static struct Qdisc *ingress_leaf(struct Qdisc *sch, unsigned long arg)
 
 static unsigned long ingress_find(struct Qdisc *sch, u32 classid)
 {
-	return TC_H_MIN(classid) + 1;
+	return TC_H_MIN(classid);
 }
 
 static unsigned long ingress_bind_filter(struct Qdisc *sch,
@@ -53,7 +53,12 @@ static struct tcf_block *ingress_tcf_block(struct Qdisc *sch, unsigned long cl,
 {
 	struct ingress_sched_data *q = qdisc_priv(sch);
 
-	return q->block;
+	switch (cl) {
+	case TC_H_MIN(TC_H_MIN_INGRESS):
+		return q->block;
+	default:
+		return NULL;
+	}
 }
 
 static void clsact_chain_head_change(struct tcf_proto *tp_head, void *priv)
-- 
2.20.1

Re: [PATCH net] net: sched: sch_ingress: do not report ingress filter info in egress path

[Davide Caratti]

On Tue, 2019-05-21 at 14:59 +0200, Lorenzo Bianconi wrote:
> Currently if we add a filter to the ingress qdisc (e.g matchall) the
> filter data are reported even in the egress path. The issue can be
> triggered with the following reproducer:
> 
> $tc qdisc add dev lo ingress
> $tc filter add dev lo ingress matchall action ok
> $tc filter show dev lo ingress
> filter protocol all pref 49152 matchall chain 0
> filter protocol all pref 49152 matchall chain 0 handle 0x1
>   not_in_hw
> 	action order 1: gact action pass
> 		 random type none pass val 0
> 		 	 index 1 ref 1 bind 1
> 
> $tc filter show dev lo egress
> filter protocol all pref 49152 matchall chain 0
> filter protocol all pref 49152 matchall chain 0 handle 0x1
>   not_in_hw
> 	action order 1: gact action pass
> 		 random type none pass val 0
> 		 	 index 1 ref 1 bind 1
> 
> Fix it reporting NULL for non-ingress filters in ingress_tcf_block
> routine
> 
> Fixes: 6529eaba33f0 ("net: sched: introduce tcf block infrastructure")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>  net/sched/sch_ingress.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
> index 0bac926b46c7..1825347fed3a 100644
> --- a/net/sched/sch_ingress.c
> +++ b/net/sched/sch_ingress.c
> @@ -31,7 +31,7 @@ static struct Qdisc *ingress_leaf(struct Qdisc *sch, unsigned long arg)
>  
>  static unsigned long ingress_find(struct Qdisc *sch, u32 classid)
>  {
> -	return TC_H_MIN(classid) + 1;
> +	return TC_H_MIN(classid);

probably this breaks a command that was wrong before, but it's worth
mentioning. Because of the above hunk, the following command

# tc qdisc add dev test0 ingress
# tc filter add dev test0 parent ffff:fff1 matchall action drop
# tc filter add dev test0 parent ffff: matchall action continue

gave no errors, and dropped packets on unpatched kernel. With this patch,
the kernel refuses to add the 'matchall' rules (and because of that,
traffic passes).

running TDC, it seems that a patched kernel does not pass anymore
some of the test cases belonging to the 'filter' category:

# ./tdc.py -e 901f
Test 901f: Add fw filter with prio at 32-bit maxixum
exit: 2
exit: 0
RTNETLINK answers: Invalid argument
We have an error talking to the kernel, -1

All test results:
1..1
not ok 1 901f - Add fw filter with prio at 32-bit maxixum
        Command exited with 2, expected 0
RTNETLINK answers: Invalid argument
We have an error talking to the kernel, -1

(the same test is passing on a unpatched kernel)

Do you think it's worth fixing those test cases too?

thanks a lot!
-- 
davide

>  }
>  
>  static unsigned long ingress_bind_filter(struct Qdisc *sch,
> @@ -53,7 +53,12 @@ static struct tcf_block *ingress_tcf_block(struct Qdisc *sch, unsigned long cl,
>  {
>  	struct ingress_sched_data *q = qdisc_priv(sch);
>  
> -	return q->block;
> +	switch (cl) {
> +	case TC_H_MIN(TC_H_MIN_INGRESS):
> +		return q->block;
> +	default:
> +		return NULL;
> +	}
>  }
>  
>  static void clsact_chain_head_change(struct tcf_proto *tp_head, void *priv)

Re: [PATCH net] net: sched: sch_ingress: do not report ingress filter info in egress path

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3113 bytes --]

> On Tue, 2019-05-21 at 14:59 +0200, Lorenzo Bianconi wrote:
> > Currently if we add a filter to the ingress qdisc (e.g matchall) the
> > filter data are reported even in the egress path. The issue can be
> > triggered with the following reproducer:
> > 

[...]

> > diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
> > index 0bac926b46c7..1825347fed3a 100644
> > --- a/net/sched/sch_ingress.c
> > +++ b/net/sched/sch_ingress.c
> > @@ -31,7 +31,7 @@ static struct Qdisc *ingress_leaf(struct Qdisc *sch, unsigned long arg)
> >  
> >  static unsigned long ingress_find(struct Qdisc *sch, u32 classid)
> >  {
> > -	return TC_H_MIN(classid) + 1;
> > +	return TC_H_MIN(classid);
> 
> probably this breaks a command that was wrong before, but it's worth
> mentioning. Because of the above hunk, the following command
> 
> # tc qdisc add dev test0 ingress
> # tc filter add dev test0 parent ffff:fff1 matchall action drop
> # tc filter add dev test0 parent ffff: matchall action continue
> 
> gave no errors, and dropped packets on unpatched kernel. With this patch,
> the kernel refuses to add the 'matchall' rules (and because of that,
> traffic passes).
> 
> running TDC, it seems that a patched kernel does not pass anymore
> some of the test cases belonging to the 'filter' category:
> 
> # ./tdc.py -e 901f
> Test 901f: Add fw filter with prio at 32-bit maxixum
> exit: 2
> exit: 0
> RTNETLINK answers: Invalid argument
> We have an error talking to the kernel, -1
> 
> All test results:
> 1..1
> not ok 1 901f - Add fw filter with prio at 32-bit maxixum
>         Command exited with 2, expected 0
> RTNETLINK answers: Invalid argument
> We have an error talking to the kernel, -1
> 
> (the same test is passing on a unpatched kernel)
> 
> Do you think it's worth fixing those test cases too?
> 
> thanks a lot!
> -- 
> davide

Hi Davide,

thx to point this out. Applying this patch the ingress qdisc has the same
behaviour of clsact one.

$tc qdisc add dev lo clsact
$tc filter add dev lo parent ffff:fff1 matchall action drop
Error: Specified class doesn't exist.
We have an error talking to the kernel, -1
$tc filter add dev lo parent ffff:fff2 matchall action drop

$tc qdisc add dev lo ingress
$tc filter add dev lo parent ffff:fff2 matchall action drop

is it acceptable? If so I can fix the tests as well
If not, is there another way to verify the filter is for the ingress path if
parent identifier is not constant? (ingress_find() reports the TC_H_MIN of
parent identifier)

Regards,
Lorenzo

> 
> >  }
> >  
> >  static unsigned long ingress_bind_filter(struct Qdisc *sch,
> > @@ -53,7 +53,12 @@ static struct tcf_block *ingress_tcf_block(struct Qdisc *sch, unsigned long cl,
> >  {
> >  	struct ingress_sched_data *q = qdisc_priv(sch);
> >  
> > -	return q->block;
> > +	switch (cl) {
> > +	case TC_H_MIN(TC_H_MIN_INGRESS):
> > +		return q->block;
> > +	default:
> > +		return NULL;
> > +	}
> >  }
> >  
> >  static void clsact_chain_head_change(struct tcf_proto *tp_head, void *priv)
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net] net: sched: sch_ingress: do not report ingress filter info in egress path

[Daniel Borkmann]

On 05/22/2019 12:20 PM, Lorenzo Bianconi wrote:
>> On Tue, 2019-05-21 at 14:59 +0200, Lorenzo Bianconi wrote:
>>> Currently if we add a filter to the ingress qdisc (e.g matchall) the
>>> filter data are reported even in the egress path. The issue can be
>>> triggered with the following reproducer:
> 
> [...]
> 
>>> diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
>>> index 0bac926b46c7..1825347fed3a 100644
>>> --- a/net/sched/sch_ingress.c
>>> +++ b/net/sched/sch_ingress.c
>>> @@ -31,7 +31,7 @@ static struct Qdisc *ingress_leaf(struct Qdisc *sch, unsigned long arg)
>>>  
>>>  static unsigned long ingress_find(struct Qdisc *sch, u32 classid)
>>>  {
>>> -	return TC_H_MIN(classid) + 1;
>>> +	return TC_H_MIN(classid);
>>
>> probably this breaks a command that was wrong before, but it's worth
>> mentioning. Because of the above hunk, the following command
>>
>> # tc qdisc add dev test0 ingress
>> # tc filter add dev test0 parent ffff:fff1 matchall action drop
>> # tc filter add dev test0 parent ffff: matchall action continue
>>
>> gave no errors, and dropped packets on unpatched kernel. With this patch,
>> the kernel refuses to add the 'matchall' rules (and because of that,
>> traffic passes).
>>
>> running TDC, it seems that a patched kernel does not pass anymore
>> some of the test cases belonging to the 'filter' category:
>>
>> # ./tdc.py -e 901f
>> Test 901f: Add fw filter with prio at 32-bit maxixum
>> exit: 2
>> exit: 0
>> RTNETLINK answers: Invalid argument
>> We have an error talking to the kernel, -1
>>
>> All test results:
>> 1..1
>> not ok 1 901f - Add fw filter with prio at 32-bit maxixum
>>         Command exited with 2, expected 0
>> RTNETLINK answers: Invalid argument
>> We have an error talking to the kernel, -1
>>
>> (the same test is passing on a unpatched kernel)
>>
>> Do you think it's worth fixing those test cases too?
>>
>> thanks a lot!
>> -- 
>> davide
> 
> Hi Davide,
> 
> thx to point this out. Applying this patch the ingress qdisc has the same
> behaviour of clsact one.
> 
> $tc qdisc add dev lo clsact
> $tc filter add dev lo parent ffff:fff1 matchall action drop
> Error: Specified class doesn't exist.
> We have an error talking to the kernel, -1
> $tc filter add dev lo parent ffff:fff2 matchall action drop
> 
> $tc qdisc add dev lo ingress
> $tc filter add dev lo parent ffff:fff2 matchall action drop
> 
> is it acceptable? If so I can fix the tests as well
> If not, is there another way to verify the filter is for the ingress path if
> parent identifier is not constant? (ingress_find() reports the TC_H_MIN of
> parent identifier)

As far as I know this would break sch_ingress users ... For sch_ingress
any minor should be accepted. For sch_clsact, only 0xFFF2U and 0xFFF3U
are accepted, so it can be extended in future if needed. For old sch_ingress
that ship has sailed, which is why sch_clsact was needed in order to have
such selectors, see also 1f211a1b929c ("net, sched: add clsact qdisc").
Meaning, minors for sch_ingress are a superset of sch_clsact and not
compatible in that sense. If you adapt sch_ingress to the same behavior
as sch_clsact, things might break indeed as Davide pointed out.

> Regards,
> Lorenzo
> 
>>
>>>  }
>>>  
>>>  static unsigned long ingress_bind_filter(struct Qdisc *sch,
>>> @@ -53,7 +53,12 @@ static struct tcf_block *ingress_tcf_block(struct Qdisc *sch, unsigned long cl,
>>>  {
>>>  	struct ingress_sched_data *q = qdisc_priv(sch);
>>>  
>>> -	return q->block;
>>> +	switch (cl) {
>>> +	case TC_H_MIN(TC_H_MIN_INGRESS):
>>> +		return q->block;
>>> +	default:
>>> +		return NULL;
>>> +	}
>>>  }
>>>  
>>>  static void clsact_chain_head_change(struct tcf_proto *tp_head, void *priv)
>>
>>

Re: [PATCH net] net: sched: sch_ingress: do not report ingress filter info in egress path

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 4249 bytes --]

> On 05/22/2019 12:20 PM, Lorenzo Bianconi wrote:
> >> On Tue, 2019-05-21 at 14:59 +0200, Lorenzo Bianconi wrote:
> >>> Currently if we add a filter to the ingress qdisc (e.g matchall) the
> >>> filter data are reported even in the egress path. The issue can be
> >>> triggered with the following reproducer:
> > 
> > [...]
> > 
> >>> diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
> >>> index 0bac926b46c7..1825347fed3a 100644
> >>> --- a/net/sched/sch_ingress.c
> >>> +++ b/net/sched/sch_ingress.c
> >>> @@ -31,7 +31,7 @@ static struct Qdisc *ingress_leaf(struct Qdisc *sch, unsigned long arg)
> >>>  
> >>>  static unsigned long ingress_find(struct Qdisc *sch, u32 classid)
> >>>  {
> >>> -	return TC_H_MIN(classid) + 1;
> >>> +	return TC_H_MIN(classid);
> >>
> >> probably this breaks a command that was wrong before, but it's worth
> >> mentioning. Because of the above hunk, the following command
> >>
> >> # tc qdisc add dev test0 ingress
> >> # tc filter add dev test0 parent ffff:fff1 matchall action drop
> >> # tc filter add dev test0 parent ffff: matchall action continue
> >>
> >> gave no errors, and dropped packets on unpatched kernel. With this patch,
> >> the kernel refuses to add the 'matchall' rules (and because of that,
> >> traffic passes).
> >>
> >> running TDC, it seems that a patched kernel does not pass anymore
> >> some of the test cases belonging to the 'filter' category:
> >>
> >> # ./tdc.py -e 901f
> >> Test 901f: Add fw filter with prio at 32-bit maxixum
> >> exit: 2
> >> exit: 0
> >> RTNETLINK answers: Invalid argument
> >> We have an error talking to the kernel, -1
> >>
> >> All test results:
> >> 1..1
> >> not ok 1 901f - Add fw filter with prio at 32-bit maxixum
> >>         Command exited with 2, expected 0
> >> RTNETLINK answers: Invalid argument
> >> We have an error talking to the kernel, -1
> >>
> >> (the same test is passing on a unpatched kernel)
> >>
> >> Do you think it's worth fixing those test cases too?
> >>
> >> thanks a lot!
> >> -- 
> >> davide
> > 
> > Hi Davide,
> > 
> > thx to point this out. Applying this patch the ingress qdisc has the same
> > behaviour of clsact one.
> > 
> > $tc qdisc add dev lo clsact
> > $tc filter add dev lo parent ffff:fff1 matchall action drop
> > Error: Specified class doesn't exist.
> > We have an error talking to the kernel, -1
> > $tc filter add dev lo parent ffff:fff2 matchall action drop
> > 
> > $tc qdisc add dev lo ingress
> > $tc filter add dev lo parent ffff:fff2 matchall action drop
> > 
> > is it acceptable? If so I can fix the tests as well
> > If not, is there another way to verify the filter is for the ingress path if
> > parent identifier is not constant? (ingress_find() reports the TC_H_MIN of
> > parent identifier)
> 
> As far as I know this would break sch_ingress users ... For sch_ingress
> any minor should be accepted. For sch_clsact, only 0xFFF2U and 0xFFF3U
> are accepted, so it can be extended in future if needed. For old sch_ingress
> that ship has sailed, which is why sch_clsact was needed in order to have
> such selectors, see also 1f211a1b929c ("net, sched: add clsact qdisc").
> Meaning, minors for sch_ingress are a superset of sch_clsact and not
> compatible in that sense. If you adapt sch_ingress to the same behavior
> as sch_clsact, things might break indeed as Davide pointed out.

Hi Daniel,

right, thx the clarification. So for the moment let's just drop this patch
and I will investigate if it is possible to not dump ingress info on egress
path in a different way.

Regards,
Lorenzo

> 
> > Regards,
> > Lorenzo
> > 
> >>
> >>>  }
> >>>  
> >>>  static unsigned long ingress_bind_filter(struct Qdisc *sch,
> >>> @@ -53,7 +53,12 @@ static struct tcf_block *ingress_tcf_block(struct Qdisc *sch, unsigned long cl,
> >>>  {
> >>>  	struct ingress_sched_data *q = qdisc_priv(sch);
> >>>  
> >>> -	return q->block;
> >>> +	switch (cl) {
> >>> +	case TC_H_MIN(TC_H_MIN_INGRESS):
> >>> +		return q->block;
> >>> +	default:
> >>> +		return NULL;
> >>> +	}
> >>>  }
> >>>  
> >>>  static void clsact_chain_head_change(struct tcf_proto *tp_head, void *priv)
> >>
> >>
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]