netdev - Re: [PATCH] hfsc: ensure class is added to eltree exactly once

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1464737614.5939.142.camel@edumazet-glaptop3.roam.corp.google.com>
Date:	Tue, 31 May 2016 16:33:34 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Florian Westphal <fw@...len.de>
Cc:	netdev@...r.kernel.org, Miroslav Kratochvil <exa.exa@...il.com>
Subject: Re: [PATCH] hfsc: ensure class is added to eltree exactly once

On Wed, 2016-06-01 at 01:00 +0200, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@...il.com> wrote:
> > > diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> > > index d783d7c..0854be3 100644
> > > --- a/net/sched/sch_hfsc.c
> > > +++ b/net/sched/sch_hfsc.c
> > > @@ -1583,6 +1583,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch)
> > >  {
> > >  	struct hfsc_class *cl;
> > >  	int uninitialized_var(err);
> > > +	unsigned int qlen;
> > >  
> > >  	cl = hfsc_classify(skb, sch, &err);
> > >  	if (cl == NULL) {
> > > @@ -1592,6 +1593,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch)
> > >  		return err;
> > >  	}
> > >  
> > > +	qlen = cl->qdisc->q.qlen;
> > >  	err = qdisc_enqueue(skb, cl->qdisc);
> > >  	if (unlikely(err != NET_XMIT_SUCCESS)) {
> > >  		if (net_xmit_drop_count(err)) {
> > > @@ -1601,7 +1603,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch)
> > >  		return err;
> > >  	}
> > >  
> > > -	if (cl->qdisc->q.qlen == 1)
> > > +	if (qlen == 0)
> > >  		set_active(cl, qdisc_pkt_len(skb));
> > >  
> > >  	sch->q.qlen++;
> > 
> > Well, I am not sure HFSC can deal with non work conserving qdisc
> > anyway ?
> 
> As long as child qdisc dequeue() returns an skb if qlen > 0 it should
> work.

Generally speaking, a non work conserving qdisc could return NULL if it
decides to drop all packet(s) that were sitting in the queue.

Say we add a 'max sojourn time' on skbs, as yet another anti-bloat
features, in a Codel variant/extension.

HFSC does check this and eventually complains ( qdisc_warn_nonwc()),
but all its precise computations are probably wrong.

> > Call to set_active(cl, qdisc_pkt_len(skb)); would tell you that HFSC
> > does not expect another packet than @skb being the next dequeued one.
> 
> Good point.  I'll look into it in more detail but AFAIU this "only"
> means we might overshoot a defined realtime/deadline criterion,
> I don't (yet) see how it could explain Miroslavs bug report.


> > If you want to make HFSC generic, you would need a lot more changes.
> 
> Could you elaborate?

HFSC is based on packet lengths and realtime/deadline constraints.

With lazy packet lengths (if the attached children qdisc reorder
packets), this all becomes fuzzy. Which might be OK or not.

We are not helping users by silently making such decisions.
Maybe they prefer to be warned so that they can fix their setup. 

> 
> I'm not interested in e.g. tbf leaves (makes no sense to me), but I
> think it should play nice with netem, sfq, codel, fq_codel, fq, ...
> 
> I don't see any problems with fq_codel + hfsc unless I deliberately
> misconfigure fq_codel (setting extremely low mem limit, e.g. 32kbyte).

That is pure luck, then. I would not be surprised that HTB + nonwc qdisc
would actually crash in some cases.