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: <b7e58a49-744b-b34a-9fbe-a58700ddd20a@lab.ntt.co.jp>
Date:   Tue, 16 Jan 2018 15:33:53 +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:57 PM, Cong Wang wrote:
> On Mon, Jan 15, 2018 at 9:47 PM, Prashant Bhole
> <bhole_prashant_q7@....ntt.co.jp> wrote:
>>
>>
>> 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/
> 
> Odd, I don't even see it in my inbox...
> 
> 
>>
>> Let me know if we still need v2 of my patch on -net-next (with your
>> suggested nit picks).
> 
> It is okay, but it doesn't have to forward commit d59f5ffa59d8 to
> net, reordering allocation before ->init() should be simpler.

Pardon my ignorance, I think it is necessary to forward d59f5ffa59d8 to 
-net. Because previously flags were set during init and checked after 
init to allocate memory. static_flags makes the flags available before 
init, hence we can allocate memory before init.
Sorry, but from your reply I couldn't understand whether we need v2 for 
net-next.

-Prashant