[Patch net] net_sched: fix a missing rcu barrier in mini_qdisc_pair_swap()

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 ")
Reported-by: Jakub Kicinski <redacted>
Tested-by: Jakub Kicinski <redacted>
Cc: Jiri Pirko <redacted>
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <redacted>
---
 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
 		 * block potential new user of miniq_old until all readers
 		 * are not seeing it.
 		 */

-- 
2.13.6

Thu, Dec 21, 2017 at 08:26:24AM CET, xiyou.wangcong@gmail.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 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.


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

On Thu, Dec 21, 2017 at 1:03 AM, Jiri Pirko [off-list ref] wrote:

Thu, Dec 21, 2017 at 08:26:24AM CET, xiyou.wangcong@gmail.com wrote:

quoted

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.

quoted

Reported-by: Jakub Kicinski <redacted>
Tested-by: Jakub Kicinski <redacted>
Cc: Jiri Pirko <redacted>
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <redacted>
---
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();

quoted

              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.

On Thu, Dec 21, 2017 at 11:01 AM, Cong Wang [off-list ref] wrote:

On Thu, Dec 21, 2017 at 1:03 AM, Jiri Pirko [off-list ref] wrote:

quoted

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.

Also, in case you believe call_rcu to free qdisc is queued after
the call_rcu in miniq, you are wrong again:

https://www.kernel.org/pub/linux/kernel/people/paulmck/Answers/RCU/RCUCBordering.html

The rcu callbacks don't guarantee FIFO ordering.

Thu, Dec 21, 2017 at 09:54:06PM CET, xiyou.wangcong@gmail.com wrote:

On Thu, Dec 21, 2017 at 11:01 AM, Cong Wang [off-list ref] wrote:

quoted

On Thu, Dec 21, 2017 at 1:03 AM, Jiri Pirko [off-list ref] wrote:

quoted

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.

Also, in case you believe call_rcu to free qdisc is queued after
the call_rcu in miniq, you are wrong again:

I certainly don't :) Not sure why you think I do.

https://www.kernel.org/pub/linux/kernel/people/paulmck/Answers/RCU/RCUCBordering.html

The rcu callbacks don't guarantee FIFO ordering.

Thu, Dec 21, 2017 at 08:01:35PM CET, xiyou.wangcong@gmail.com wrote:

On Thu, Dec 21, 2017 at 1:03 AM, Jiri Pirko [off-list ref] wrote:

quoted

Thu, Dec 21, 2017 at 08:26:24AM CET, xiyou.wangcong@gmail.com wrote:

[...]

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

Got it now. You are right. Thank you for politely helping me to get out
of my confusion.

Thu, Dec 21, 2017 at 08:26:24AM CET, xiyou.wangcong@gmail.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 ")
Reported-by: Jakub Kicinski <redacted>
Tested-by: Jakub Kicinski <redacted>
Cc: Jiri Pirko <redacted>
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <redacted>

Acked-by: Jiri Pirko <redacted>

From: Cong Wang <redacted>
Date: Wed, 20 Dec 2017 23:26:24 -0800

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 ")
Reported-by: Jakub Kicinski <redacted>
Tested-by: Jakub Kicinski <redacted>
Cc: Jiri Pirko <redacted>
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <redacted>

Applied.