netdev - Re: [PATCH net] net/sched: sch_taprio: reset child qdiscs before freeing them

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]

Date:   Thu, 17 Dec 2020 21:32:29 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net] net/sched: sch_taprio: reset child qdiscs before
 freeing them

hello Jakub, and thanks for checking!

On Thu, 2020-12-17 at 11:05 -0800, Jakub Kicinski wrote:
> On Wed, 16 Dec 2020 19:33:29 +0100 Davide Caratti wrote:
> > +	if (q->qdiscs) {
> > +		for (i = 0; i < dev->num_tx_queues && q->qdiscs[i]; i++)
> > +			qdisc_reset(q->qdiscs[i]);
> 
> Are you sure that we can't graft a NULL in the middle of the array?

that should not happen, because child qdiscs are checked for being non-
NULL when they are created:

https://elixir.bootlin.com/linux/v5.10/source/net/sched/sch_taprio.c#L1674

and then assigned to q->qdiscs[i]. So, there might be NULL elements of
q->qdiscs[] in the middle of the array when taprio_reset() is called,
but it should be ok to finish the loop when we encounter the first one:
subsequent ones should be NULL as well.

> Shouldn't this be:
> 
> 	for (i = 0; i < dev->num_tx_queues; i++)
> 		if (q->qdiscs[i])
> 			qdisc_reset(q->qdiscs[i]);
> 
> ?

probably the above code is more robust, but - like you noticed - then we
should also change the same 'for' loop in taprio_destroy(), otherwise it
leaks resources. If you and Vinicius agree, I can post a follow-up patch
that makes ->reset() and ->destroy()  more consistent with ->enqueue() 
and ->dequeue(), and send it for net-next when it reopens. Ok?

-- 
davide