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 20:32:22 +0300
From:   Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
To:     Cong Wang <xiyou.wangcong@...il.com>
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 16.08.2017 20:22, Cong Wang wrote:
> 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??
> 

Here warning evaluated only if qlen is zero.
'n' could be zero not together with non-zero 'len' and zero 'qlen'.
This means queue already was empty before reduce but caller somehow reduced it even further.