Re: RCU callback crashes

On Thu, Dec 21, 2017 at 8:26 AM, John Fastabend
[off-list ref] wrote:

On 12/20/2017 11:27 PM, Cong Wang wrote:

quoted

On Wed, Dec 20, 2017 at 4:50 PM, Jakub Kicinski [off-list ref] wrote:

quoted

On Wed, 20 Dec 2017 16:41:14 -0800, Jakub Kicinski wrote:

quoted

Just as I hit send... :)  but this looks unrelated, "Comm: sshd" -
so probably from the management interface.

[  154.604041] ==================================================================
[  154.612245] BUG: KASAN: slab-out-of-bounds in pfifo_fast_dequeue+0x140/0x2d0
[  154.620219] Read of size 8 at addr ffff88086bb64040 by task sshd/983
[  154.627403]
[  154.629161] CPU: 10 PID: 983 Comm: sshd Not tainted 4.15.0-rc3-perf-00984-g82d3fc87a4aa-dirty #13
[  154.639190] Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 2.3.4 11/08/2016
[  154.647665] Call Trace:
[  154.650494]  dump_stack+0xa6/0x118
[  154.654387]  ? _atomic_dec_and_lock+0xe8/0xe8
[  154.659355]  ? trace_event_raw_event_rcu_torture_read+0x190/0x190
[  154.666263]  ? rcu_segcblist_enqueue+0xe9/0x120
[  154.671422]  ? _raw_spin_unlock_bh+0x91/0xc0
[  154.676286]  ? pfifo_fast_dequeue+0x140/0x2d0
[  154.681251]  print_address_description+0x6a/0x270
[  154.686601]  ? pfifo_fast_dequeue+0x140/0x2d0
[  154.691565]  kasan_report+0x23f/0x350
[  154.695752]  pfifo_fast_dequeue+0x140/0x2d0

If we trust stack decode it's:

   615  static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
   616  {
   617          struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
   618          struct sk_buff *skb = NULL;
   619          int band;
   620
   621          for (band = 0; band < PFIFO_FAST_BANDS && !skb; band++) {
   622                  struct skb_array *q = band2list(priv, band);
   623

quoted

quoted

624                  if (__skb_array_empty(q))

   625                          continue;
   626
   627                  skb = skb_array_consume_bh(q);
   628          }
   629          if (likely(skb)) {
   630                  qdisc_qstats_cpu_backlog_dec(qdisc, skb);
   631                  qdisc_bstats_cpu_update(qdisc, skb);
   632                  qdisc_qstats_cpu_qlen_dec(qdisc);
   633          }
   634
   635          return skb;
   636  }

Yeah, this one is clearly a different one and it is introduced by John's
"lockless" patchset.

I will take a look tomorrow if John doesn't.

I guess this path

  dev_deactivate_many
    dev_deactivate_queue
      qdisc_reset

here we have the qdisc lock but no rcu call or sync before the reset
does a kfree_skb and cleans up list walks. So possible for xmit path to
also be pushing skbs onto the array/lists still. I don't think this is
the issue triggered above but needs to be fixed

No, we already have a synchronize_net() there. It is probably just
a race with BH.

I think you missed the spinlock in dev_qdisc_reset(), at least
the comment right above qdisc_reset() says so.

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 10aaa3b615ce..00ddb5f8f430 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c

@@ -1097,8 +1097,11 @@ static void dev_qdisc_reset(struct net_device *dev,
 {
        struct Qdisc *qdisc = dev_queue->qdisc_sleeping;

-       if (qdisc)
+       if (qdisc) {
+               spin_lock_bh(qdisc_lock(qdisc));
                qdisc_reset(qdisc);
+               spin_unlock_bh(qdisc_lock(qdisc));
+       }
 }

 /**