netdev - Re: [Patch net] net_sched: fix a missing rcu barrier in mini_qdisc_pair

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: <CAM_iQpVfYUsJ6D5fpE+Ng8wGxK6uoN3PRntYO_Y0j+7YRKdSqg@mail.gmail.com>
Date:   Thu, 21 Dec 2017 11:01:35 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        John Fastabend <john.fastabend@...il.com>
Subject: Re: [Patch net] net_sched: fix a missing rcu barrier in mini_qdisc_pair_swap()

On Thu, Dec 21, 2017 at 1:03 AM, Jiri Pirko <jiri@...nulli.us> wrote:
> Thu, Dec 21, 2017 at 08:26:24AM CET, xiyou.wangcong@...il.com wrote:
>>The rcu_barrier_bh() in mini_qdisc_pair_swap() is to wait for
>>flying RCU callback installed by a previous mini_qdisc_pair_swap(),
>>however we miss it on the tp_head==NULL path, which leads to that
>>the RCU callback still uses miniq_old->rcu after it is freed together
>>with qdisc in qdisc_graft(). So just add it on that path too.
>>
>>Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath ")
>
> This fixes:
> 752fbcc33405 ("net_sched: no need to free qdisc in RCU callback")
>
> Before that, the issue was not there as the qdisc struct got removed
> after a grace period.


This is non-sense. You have to read the stack trace from Jakub again
and tell me why you keep believing any RCU reader involved.

I am pretty sure no one reported any crash between commit
752fbcc33405 and 46209401f8f6.


>
>
>>Reported-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
>>Tested-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
>>Cc: Jiri Pirko <jiri@...lanox.com>
>>Cc: John Fastabend <john.fastabend@...il.com>
>>Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
>>---
>> net/sched/sch_generic.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>>diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>>index cd1b200acae7..661c7144b53a 100644
>>--- a/net/sched/sch_generic.c
>>+++ b/net/sched/sch_generic.c
>>@@ -1040,6 +1040,8 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
>>
>>       if (!tp_head) {
>>               RCU_INIT_POINTER(*miniqp->p_miniq, NULL);
>>+              /* Wait for flying RCU callback before it is freed. */
>>+              rcu_barrier_bh();
>
>
>>               return;
>>       }
>>
>>@@ -1055,7 +1057,7 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
>>       rcu_assign_pointer(*miniqp->p_miniq, miniq);
>>
>>       if (miniq_old)
>>-              /* This is counterpart of the rcu barrier above. We need to
>>+              /* This is counterpart of the rcu barriers above. We need to
>
> This is incorrect. Here we block in order to not use the same miniq
> again in scenario
>
> miniq1 (X)
> miniq2
> miniq1 (yet there are reader using X)
>
> This call_rcu has 0 relation to the barrier you are adding.


Seriously? It is this call_rcu still flying after we free the qdisc.
Did you seriously look into the stack trace from Jakub?


>
>
> But again, we don't we just free qdisc in call_rcu and avoid the
> barrier?


Non-sense again. Why qdisc code should be adjusted for your
miniq code? It is your own responsibility to take care of this shit.
Don't spread it out of minq.