netdev - Re: [Patch net-next v3 3/3] net_sched: implement ->change_tx_queue_len() for pfifo

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: <20180126154618-mutt-send-email-mst@kernel.org>
Date:   Fri, 26 Jan 2018 15:48:15 +0200
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org,
        john.fastabend@...il.com
Subject: Re: [Patch net-next v3 3/3] net_sched: implement
 ->change_tx_queue_len() for pfifo_fast

On Fri, Jan 26, 2018 at 11:57:59AM +0800, Jason Wang wrote:
> 
> 
> On 2018年01月26日 10:26, Cong Wang wrote:
> > pfifo_fast used to drop based on qdisc_dev(qdisc)->tx_queue_len,
> > so we have to resize skb array when we change tx_queue_len.
> > 
> > Other qdiscs which read tx_queue_len are fine because they
> > all save it to sch->limit or somewhere else in qdisc during init.
> > They don't have to implement this, it is nicer if they do so
> > that users don't have to re-configure qdisc after changing
> > tx_queue_len.
> > 
> > Cc: John Fastabend <john.fastabend@...il.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> > ---
> >   net/sched/sch_generic.c | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> > 
> > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> > index 08f9fa27e06e..190570f21b20 100644
> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -763,6 +763,23 @@ static void pfifo_fast_destroy(struct Qdisc *sch)
> >   	}
> >   }
> > +static int pfifo_fast_change_tx_queue_len(struct Qdisc *sch,
> > +					  unsigned int new_len)
> > +{
> > +	struct pfifo_fast_priv *priv = qdisc_priv(sch);
> > +	struct skb_array *bands[PFIFO_FAST_BANDS];
> > +	int prio;
> > +
> > +	for (prio = 0; prio < PFIFO_FAST_BANDS; prio++) {
> > +		struct skb_array *q = band2list(priv, prio);
> > +
> > +		bands[prio] = q;
> > +	}
> > +
> > +	return skb_array_resize_multiple(bands, PFIFO_FAST_BANDS, new_len,
> > +					 GFP_KERNEL);
> > +}
> > +
> >   struct Qdisc_ops pfifo_fast_ops __read_mostly = {
> >   	.id		=	"pfifo_fast",
> >   	.priv_size	=	sizeof(struct pfifo_fast_priv),
> > @@ -773,6 +790,7 @@ struct Qdisc_ops pfifo_fast_ops __read_mostly = {
> >   	.destroy	=	pfifo_fast_destroy,
> >   	.reset		=	pfifo_fast_reset,
> >   	.dump		=	pfifo_fast_dump,
> > +	.change_tx_queue_len =  pfifo_fast_change_tx_queue_len,
> >   	.owner		=	THIS_MODULE,
> >   	.static_flags	=	TCQ_F_NOLOCK | TCQ_F_CPUSTATS,
> >   };
> 
> Is __skb_array_empty() in pfifo_fast_dequeue() still safe after this change?
> 
> Thanks

I think it isn't.  If you want to use resize *and* use unlocked variants,
you must lock all producers and consumers when resizing yourself.

-- 
MST