[PATCH net] gro_cell: add napi_disable in gro_cells_destroy

[PATCH net] gro_cell: add napi_disable in gro_cells_destroy

[Lorenzo Bianconi]

Add napi_disable routine in gro_cells_destroy since starting from
commit c42858eaf492 ("gro_cells: remove spinlock protecting receive
queues") gro_cell_poll and gro_cells_destroy can run concurrently on
napi_skbs list producing a kernel Oops if the tunnel interface is
removed while gro_cell_poll is running. The following Oops has been
triggered removing a vxlan device while the interface is receiving
traffic

[ 5628.948853] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
[ 5628.949981] PGD 0 P4D 0
[ 5628.950308] Oops: 0002 [#1] SMP PTI
[ 5628.950748] CPU: 0 PID: 9 Comm: ksoftirqd/0 Not tainted 4.20.0-rc6+ #41
[ 5628.952940] RIP: 0010:gro_cell_poll+0x49/0x80
[ 5628.955615] RSP: 0018:ffffc9000004fdd8 EFLAGS: 00010202
[ 5628.956250] RAX: 0000000000000000 RBX: ffffe8ffffc08150 RCX: 0000000000000000
[ 5628.957102] RDX: 0000000000000000 RSI: ffff88802356bf00 RDI: ffffe8ffffc08150
[ 5628.957940] RBP: 0000000000000026 R08: 0000000000000000 R09: 0000000000000000
[ 5628.958803] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000040
[ 5628.959661] R13: ffffe8ffffc08100 R14: 0000000000000000 R15: 0000000000000040
[ 5628.960682] FS:  0000000000000000(0000) GS:ffff88803ea00000(0000) knlGS:0000000000000000
[ 5628.961616] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 5628.962359] CR2: 0000000000000008 CR3: 000000000221c000 CR4: 00000000000006b0
[ 5628.963188] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 5628.964034] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 5628.964871] Call Trace:
[ 5628.965179]  net_rx_action+0xf0/0x380
[ 5628.965637]  __do_softirq+0xc7/0x431
[ 5628.966510]  run_ksoftirqd+0x24/0x30
[ 5628.966957]  smpboot_thread_fn+0xc5/0x160
[ 5628.967436]  kthread+0x113/0x130
[ 5628.968283]  ret_from_fork+0x3a/0x50
[ 5628.968721] Modules linked in:
[ 5628.969099] CR2: 0000000000000008
[ 5628.969510] ---[ end trace 9d9dedc7181661fe ]---
[ 5628.970073] RIP: 0010:gro_cell_poll+0x49/0x80
[ 5628.972965] RSP: 0018:ffffc9000004fdd8 EFLAGS: 00010202
[ 5628.973611] RAX: 0000000000000000 RBX: ffffe8ffffc08150 RCX: 0000000000000000
[ 5628.974504] RDX: 0000000000000000 RSI: ffff88802356bf00 RDI: ffffe8ffffc08150
[ 5628.975462] RBP: 0000000000000026 R08: 0000000000000000 R09: 0000000000000000
[ 5628.976413] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000040
[ 5628.977375] R13: ffffe8ffffc08100 R14: 0000000000000000 R15: 0000000000000040
[ 5628.978296] FS:  0000000000000000(0000) GS:ffff88803ea00000(0000) knlGS:0000000000000000
[ 5628.979327] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 5628.980044] CR2: 0000000000000008 CR3: 000000000221c000 CR4: 00000000000006b0
[ 5628.980929] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 5628.981736] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 5628.982409] Kernel panic - not syncing: Fatal exception in interrupt
[ 5628.983307] Kernel Offset: disabled

Fixes: c42858eaf492 ("gro_cells: remove spinlock protecting receive queues")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 net/core/gro_cells.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c
index 4b54e5f107c6..acf45ddbe924 100644
--- a/net/core/gro_cells.c
+++ b/net/core/gro_cells.c
@@ -84,6 +84,7 @@ void gro_cells_destroy(struct gro_cells *gcells)
 	for_each_possible_cpu(i) {
 		struct gro_cell *cell = per_cpu_ptr(gcells->cells, i);
 
+		napi_disable(&cell->napi);
 		netif_napi_del(&cell->napi);
 		__skb_queue_purge(&cell->napi_skbs);
 	}
-- 
2.19.2

Re: [PATCH net] gro_cell: add napi_disable in gro_cells_destroy

[Eric Dumazet]

On Wed, Dec 19, 2018 at 2:23 PM Lorenzo Bianconi
<lorenzo.bianconi@redhat.com> wrote:
>
> Add napi_disable routine in gro_cells_destroy since starting from
> commit c42858eaf492 ("gro_cells: remove spinlock protecting receive
> queues") gro_cell_poll and gro_cells_destroy can run concurrently on
> napi_skbs list producing a kernel Oops if the tunnel interface is
> removed while gro_cell_poll is running. The following Oops has been
> triggered removing a vxlan device while the interface is receiving
> traffic
>

This seems reasonable, I wonder why this bug has been hiding for so long...

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net] gro_cell: add napi_disable in gro_cells_destroy

[David Miller]

From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Date: Wed, 19 Dec 2018 23:23:00 +0100

> Add napi_disable routine in gro_cells_destroy since starting from
> commit c42858eaf492 ("gro_cells: remove spinlock protecting receive
> queues") gro_cell_poll and gro_cells_destroy can run concurrently on
> napi_skbs list producing a kernel Oops if the tunnel interface is
> removed while gro_cell_poll is running. The following Oops has been
> triggered removing a vxlan device while the interface is receiving
> traffic
 ...
> Fixes: c42858eaf492 ("gro_cells: remove spinlock protecting receive queues")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Applied and queued up for -stable, thanks.

Re: [PATCH net] gro_cell: add napi_disable in gro_cells_destroy

[Eric Dumazet]

On 12/19/2018 02:46 PM, Eric Dumazet wrote:
> On Wed, Dec 19, 2018 at 2:23 PM Lorenzo Bianconi
> <lorenzo.bianconi@redhat.com> wrote:
>>
>> Add napi_disable routine in gro_cells_destroy since starting from
>> commit c42858eaf492 ("gro_cells: remove spinlock protecting receive
>> queues") gro_cell_poll and gro_cells_destroy can run concurrently on
>> napi_skbs list producing a kernel Oops if the tunnel interface is
>> removed while gro_cell_poll is running. The following Oops has been
>> triggered removing a vxlan device while the interface is receiving
>> traffic
>>
> 
> This seems reasonable, I wonder why this bug has been hiding for so long...
> 
> Acked-by: Eric Dumazet <edumazet@google.com>
> 

Since we had another syzbot report involving this stuff [1], I took another look.

Apparently vxlan code does not look at (dev->flags & IFF_UP) before injecting
a packet (calling gro_cells_receive())

IP tunnels do this check properly.

[1]
HEAD commit:    b71acb0e3721 Merge branch 'linus' of git://git.kernel.org/..
git tree:       net-next
console output: https://syzkaller.appspot.com/x/log.txt?x=14314bab400000
kernel config:  https://syzkaller.appspot.com/x/.config?x=b03c5892bb940c76
dashboard link: https://syzkaller.appspot.com/bug?extid=6fe674089f9deb9f7726
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)

I suspect the following fix is needed for vxlan

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 5209ee9aac47846367d7f469a7e69d08c030087e..7a443c251e604c41005d7d0f73832c22aed51768 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1657,6 +1657,10 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
                goto drop;
        }
 
+       if (unlikely(!(vxlan->dev->flags & IFF_UP))) {
+               atomic_long_inc(&vxlan->dev->rx_dropped);
+               goto drop;
+       }
        stats = this_cpu_ptr(vxlan->dev->tstats);
        u64_stats_update_begin(&stats->syncp);
        stats->rx_packets++;