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

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- 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 --]