> 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)
>
>
signature.asc
Description: PGP signature
