netdev - Re: [PATCH net] net: sched: replaced invalid qdisc tree flush helper in qdisc

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] [day] [month] [year] [list]

Message-ID: <20210128133235.0c75b81e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 28 Jan 2021 13:32:35 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Alexander Ovechkin <ovov@...dex-team.ru>
Cc:     netdev@...r.kernel.org, zeil@...dex-team.ru,
        dmtrmonakhov@...dex-team.ru
Subject: Re: [PATCH net] net: sched: replaced invalid qdisc tree flush
 helper in qdisc_replace

On Wed, 27 Jan 2021 00:56:41 +0300 Alexander Ovechkin wrote:
> Commit e5f0e8f8e456 ("net: sched: introduce and use qdisc tree flush/purge helpers")
> introduced qdisc tree flush/purge helpers, but erroneously used flush helper
> instead of purge helper in qdisc_replace function.
> This issue was found in our CI, that tests various qdisc setups by configuring
> qdisc and sending data through it. Call of invalid helper sporadically leads
> to corruption of vt_tree/cf_tree of hfsc_class that causes kernel oops:

The patch in question indeed does change the code, but I wonder if the
problem isn't in HFSC. Why would the caller depend on the old qdisc
being purged by qdisc_replace()?

Please add some more info on this..

> Fixes: e5f0e8f8e456 ("net: sched: introduce and use qdisc tree flush/purge
> helpers")

... fix the tag (it shouldn't be wrapped), and CC the right people
(especially the author of the patch you're pointing the tag at -
scripts/get_maintainer is your friend), and repost.

> Signed-off-by: Alexander Ovechkin <ovov@...dex-team.ru>
> Reported-by: Alexander Kuznetsov <wwfq@...dex-team.ru>
> Acked-by: Dmitry Monakhov <dmtrmonakhov@...dex-team.ru>
> Acked-by: Dmitry Yakunin <zeil@...dex-team.ru>
> ---
>  include/net/sch_generic.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 639e465a108f..5b490b5591df 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -1143,7 +1143,7 @@ static inline struct Qdisc *qdisc_replace(struct Qdisc *sch, struct Qdisc *new,
>  	old = *pold;
>  	*pold = new;
>  	if (old != NULL)
> -		qdisc_tree_flush_backlog(old);
> +		qdisc_purge_queue(old);
>  	sch_tree_unlock(sch);
>  
>  	return old;