netdev - Re: [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty

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:   Wed, 16 Aug 2017 10:22:02 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jiri Kosina <jkosina@...e.cz>,
        Eric Dumazet <edumazet@...gle.com>,
        Jamal Hadi Salim <jhs@...atatu.com>
Subject: Re: [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty

On Tue, Aug 15, 2017 at 6:39 AM, Konstantin Khlebnikov
<khlebnikov@...dex-team.ru> wrote:
> This callback is used for deactivating class in parent qdisc.
> This is cheaper to test queue length right here.
>
> Also this allows to catch draining screwed backlog and prevent
> second deactivation of already inactive parent class which will
> crash kernel for sure. Kernel with print warning at destruction
> of child qdisc where no packets but backlog is not zero.

Good cleanup. Please explicitly mark patches like this as
for net-next.

Just one comment below.


> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index bd24a550e0f9..18da45c0769c 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -752,6 +752,7 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, unsigned int n,
>         const struct Qdisc_class_ops *cops;
>         unsigned long cl;
>         u32 parentid;
> +       bool notify;
>         int drops;
>
>         if (n == 0 && len == 0)
> @@ -764,6 +765,13 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, unsigned int n,
>
>                 if (sch->flags & TCQ_F_NOPARENT)
>                         break;
> +               /* Notify parent qdisc only if child qdisc becomes empty.
> +                *
> +                * If child was empty even before update then backlog
> +                * counter is screwed and we skip notification because
> +                * parent class is already passive.
> +                */
> +               notify = !sch->q.qlen && !WARN_ON_ONCE(!n);

Since 'n' never changes in this function, can we just move
this WARN_ON_ONCE() right before the loop??