| Message ID | 738244fd5863e6228275ee8f71e81d6baafca243.1558442828.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Rejected |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: sched: sch_ingress: do not report ingress filter info in egress path | expand |
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!
> 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) > >
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) >> >>
> 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) > >> > >> >
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)
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(-)