netdev - Re: [PATCH net-next] net: sched: fix use before alloc of per cpu stats

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: <c6ea5328-98ca-568f-7282-858b575b78ea@lab.ntt.co.jp>
Date:   Tue, 16 Jan 2018 14:47:29 +0900
From:   Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Daniel Borkmann <daniel@...earbox.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: sched: fix use before alloc of per cpu
 stats



On 1/16/2018 2:08 PM, Cong Wang wrote:
> On Sun, Jan 14, 2018 at 9:52 PM, Prashant Bhole
> <bhole_prashant_q7@....ntt.co.jp> wrote:
>> About bug:
>> During init of clsact/ingress, it links qdisc's cpu_bstats,cpu_qstats
>> with mini qdisc. TCQ_F_CPUSTATS is set in qdisc->flags during init and
>> this flag is checked after init to allocate memory for stats.
>>
>> Hence mini qdisc points to null per-cpu-stats. The problem isn't caught
>> while updating stats via mini qdisc because per_cpu_ptr(NULL, cpu_num)
>> or this_cpu_ptr(NULL) gives a valid pointer.
>>
>> About fix:
>> Currently stats memory is allocated at two places.
>> - in qdisc_alloc() if TCQ_F_CPUSTATS is set in Qdisc_ops->static_flags
>> - in qdisc_create() if TCQ_F_CPUSTATS is set in Qdisc->flags
>>
>> Qdisc_ops->static_flags is propagated to Qdisc->flags. So to fix this bug,
>> we set TCQ_F_CPUSTATS in static flags and additional condition to avoid
>> allocation after init.
> 
> 
> Good catch! One nit below.
> 
> 
>>
>> Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath")
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
>> ---
>>   net/sched/sch_api.c     | 3 ++-
>>   net/sched/sch_ingress.c | 6 ++----
>>   2 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>> index 8a04c36e579f..de99a5e80944 100644
>> --- a/net/sched/sch_api.c
>> +++ b/net/sched/sch_api.c
>> @@ -1094,7 +1094,8 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
>>                          goto err_out5;
>>          }
>>
>> -       if (qdisc_is_percpu_stats(sch)) {
>> +       if (!(ops->static_flags & TCQ_F_CPUSTATS) &&
>> +           qdisc_is_percpu_stats(sch)) {
> 
> Instead of checking both flags, it is simpler to just check if sch->cpu_bstats
> and sch->cpu_qstats are NULL here?
> 
> Also, this patch should go to -net tree, not just net-next, but -net
> doesn't have ops->static_flags yet. ;) Given this, consider moving
> the sch->cpu_bstats allocation before ops->init(), which might be
> simpler.

Cong,
Thanks for reviewing. Based on this patch, Daniel submitted another 
patch for -net:
http://patchwork.ozlabs.org/patch/861098/

Let me know if we still need v2 of my patch on -net-next (with your
suggested nit picks).

-Prashant