* [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
@ 2019-03-05 18:01 Adalbert Lazăr
2019-03-06 8:12 ` Stefano Garzarella
2019-03-06 8:41 ` Stefan Hajnoczi
0 siblings, 2 replies; 4+ messages in thread
From: Adalbert Lazăr @ 2019-03-05 18:01 UTC (permalink / raw)
To: Stefan Hajnoczi, David S . Miller, Stefano Garzarella
Cc: virtualization, kvm, netdev, linux-kernel, Adalbert Lazăr
Previous to commit 22b5c0b63f32 ("vsock/virtio: fix kernel panic after device hot-unplug"),
vsock_core_init() was called from virtio_vsock_probe(). Now,
virtio_transport_reset_no_sock() can be called before vsock_core_init()
has the chance to run.
[Wed Feb 27 14:17:09 2019] BUG: unable to handle kernel NULL pointer dereference at 0000000000000110
[Wed Feb 27 14:17:09 2019] #PF error: [normal kernel read fault]
[Wed Feb 27 14:17:09 2019] PGD 0 P4D 0
[Wed Feb 27 14:17:09 2019] Oops: 0000 [#1] SMP PTI
[Wed Feb 27 14:17:09 2019] CPU: 3 PID: 59 Comm: kworker/3:1 Not tainted 5.0.0-rc7-390-generic-hvi #390
[Wed Feb 27 14:17:09 2019] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[Wed Feb 27 14:17:09 2019] Workqueue: virtio_vsock virtio_transport_rx_work [vmw_vsock_virtio_transport]
[Wed Feb 27 14:17:09 2019] RIP: 0010:virtio_transport_reset_no_sock+0x8c/0xc0 [vmw_vsock_virtio_transport_common]
[Wed Feb 27 14:17:09 2019] Code: 35 8b 4f 14 48 8b 57 08 31 f6 44 8b 4f 10 44 8b 07 48 8d 7d c8 e8 84 f8 ff ff 48 85 c0 48 89 c3 74 2a e8 f7 31 03 00 48 89 df <48> 8b 80 10 01 00 00 e8 68 fb 69 ed 48 8b 75 f0 65 48 33 34 25 28
[Wed Feb 27 14:17:09 2019] RSP: 0018:ffffb42701ab7d40 EFLAGS: 00010282
[Wed Feb 27 14:17:09 2019] RAX: 0000000000000000 RBX: ffff9d79637ee080 RCX: 0000000000000003
[Wed Feb 27 14:17:09 2019] RDX: 0000000000000001 RSI: 0000000000000002 RDI: ffff9d79637ee080
[Wed Feb 27 14:17:09 2019] RBP: ffffb42701ab7d78 R08: ffff9d796fae70e0 R09: ffff9d796f403500
[Wed Feb 27 14:17:09 2019] R10: ffffb42701ab7d90 R11: 0000000000000000 R12: ffff9d7969d09240
[Wed Feb 27 14:17:09 2019] R13: ffff9d79624e6840 R14: ffff9d7969d09318 R15: ffff9d796d48ff80
[Wed Feb 27 14:17:09 2019] FS: 0000000000000000(0000) GS:ffff9d796fac0000(0000) knlGS:0000000000000000
[Wed Feb 27 14:17:09 2019] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[Wed Feb 27 14:17:09 2019] CR2: 0000000000000110 CR3: 0000000427f22000 CR4: 00000000000006e0
[Wed Feb 27 14:17:09 2019] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[Wed Feb 27 14:17:09 2019] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[Wed Feb 27 14:17:09 2019] Call Trace:
[Wed Feb 27 14:17:09 2019] virtio_transport_recv_pkt+0x63/0x820 [vmw_vsock_virtio_transport_common]
[Wed Feb 27 14:17:09 2019] ? kfree+0x17e/0x190
[Wed Feb 27 14:17:09 2019] ? detach_buf_split+0x145/0x160
[Wed Feb 27 14:17:09 2019] ? __switch_to_asm+0x40/0x70
[Wed Feb 27 14:17:09 2019] virtio_transport_rx_work+0xa0/0x106 [vmw_vsock_virtio_transport]
[Wed Feb 27 14:17:09 2019] NET: Registered protocol family 40
[Wed Feb 27 14:17:09 2019] process_one_work+0x167/0x410
[Wed Feb 27 14:17:09 2019] worker_thread+0x4d/0x460
[Wed Feb 27 14:17:09 2019] kthread+0x105/0x140
[Wed Feb 27 14:17:09 2019] ? rescuer_thread+0x360/0x360
[Wed Feb 27 14:17:09 2019] ? kthread_destroy_worker+0x50/0x50
[Wed Feb 27 14:17:09 2019] ret_from_fork+0x35/0x40
[Wed Feb 27 14:17:09 2019] Modules linked in: vmw_vsock_virtio_transport vmw_vsock_virtio_transport_common input_leds vsock serio_raw i2c_piix4 mac_hid qemu_fw_cfg autofs4 cirrus ttm drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops virtio_net psmouse drm net_failover pata_acpi virtio_blk failover floppy
[Wed Feb 27 14:17:09 2019] CR2: 0000000000000110
[Wed Feb 27 14:17:09 2019] ---[ end trace baa35abd2e040fe5 ]---
[Wed Feb 27 14:17:09 2019] RIP: 0010:virtio_transport_reset_no_sock+0x8c/0xc0 [vmw_vsock_virtio_transport_common]
[Wed Feb 27 14:17:09 2019] Code: 35 8b 4f 14 48 8b 57 08 31 f6 44 8b 4f 10 44 8b 07 48 8d 7d c8 e8 84 f8 ff ff 48 85 c0 48 89 c3 74 2a e8 f7 31 03 00 48 89 df <48> 8b 80 10 01 00 00 e8 68 fb 69 ed 48 8b 75 f0 65 48 33 34 25 28
[Wed Feb 27 14:17:09 2019] RSP: 0018:ffffb42701ab7d40 EFLAGS: 00010282
[Wed Feb 27 14:17:09 2019] RAX: 0000000000000000 RBX: ffff9d79637ee080 RCX: 0000000000000003
[Wed Feb 27 14:17:09 2019] RDX: 0000000000000001 RSI: 0000000000000002 RDI: ffff9d79637ee080
[Wed Feb 27 14:17:09 2019] RBP: ffffb42701ab7d78 R08: ffff9d796fae70e0 R09: ffff9d796f403500
[Wed Feb 27 14:17:09 2019] R10: ffffb42701ab7d90 R11: 0000000000000000 R12: ffff9d7969d09240
[Wed Feb 27 14:17:09 2019] R13: ffff9d79624e6840 R14: ffff9d7969d09318 R15: ffff9d796d48ff80
[Wed Feb 27 14:17:09 2019] FS: 0000000000000000(0000) GS:ffff9d796fac0000(0000) knlGS:0000000000000000
[Wed Feb 27 14:17:09 2019] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[Wed Feb 27 14:17:09 2019] CR2: 0000000000000110 CR3: 0000000427f22000 CR4: 00000000000006e0
[Wed Feb 27 14:17:09 2019] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[Wed Feb 27 14:17:09 2019] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
---
net/vmw_vsock/virtio_transport_common.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 3ae3a33da70b..502201aaff2a 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -662,6 +662,7 @@ static int virtio_transport_reset(struct vsock_sock *vsk,
*/
static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
{
+ const struct virtio_transport *t;
struct virtio_vsock_pkt_info info = {
.op = VIRTIO_VSOCK_OP_RST,
.type = le16_to_cpu(pkt->hdr.type),
@@ -680,7 +681,11 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
if (!pkt)
return -ENOMEM;
- return virtio_transport_get_ops()->send_pkt(pkt);
+ t = virtio_transport_get_ops();
+ if (!t)
+ return -ENOTCONN;
+
+ return t->send_pkt(pkt);
}
static void virtio_transport_wait_close(struct sock *sk, long timeout)
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
2019-03-05 18:01 [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock Adalbert Lazăr
@ 2019-03-06 8:12 ` Stefano Garzarella
2019-03-06 8:41 ` Stefan Hajnoczi
1 sibling, 0 replies; 4+ messages in thread
From: Stefano Garzarella @ 2019-03-06 8:12 UTC (permalink / raw)
To: Adalbert Lazăr
Cc: Stefan Hajnoczi, David S . Miller, virtualization, kvm, netdev,
linux-kernel
Hi Adalbert,
thanks for catching this issue, I have a comment below.
On Tue, Mar 05, 2019 at 08:01:45PM +0200, Adalbert Lazăr wrote:
> Previous to commit 22b5c0b63f32 ("vsock/virtio: fix kernel panic after device hot-unplug"),
> vsock_core_init() was called from virtio_vsock_probe(). Now,
> virtio_transport_reset_no_sock() can be called before vsock_core_init()
> has the chance to run.
>
> [Wed Feb 27 14:17:09 2019] BUG: unable to handle kernel NULL pointer dereference at 0000000000000110
> [Wed Feb 27 14:17:09 2019] #PF error: [normal kernel read fault]
> [Wed Feb 27 14:17:09 2019] PGD 0 P4D 0
> [Wed Feb 27 14:17:09 2019] Oops: 0000 [#1] SMP PTI
> [Wed Feb 27 14:17:09 2019] CPU: 3 PID: 59 Comm: kworker/3:1 Not tainted 5.0.0-rc7-390-generic-hvi #390
> [Wed Feb 27 14:17:09 2019] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
> [Wed Feb 27 14:17:09 2019] Workqueue: virtio_vsock virtio_transport_rx_work [vmw_vsock_virtio_transport]
> [Wed Feb 27 14:17:09 2019] RIP: 0010:virtio_transport_reset_no_sock+0x8c/0xc0 [vmw_vsock_virtio_transport_common]
> [Wed Feb 27 14:17:09 2019] Code: 35 8b 4f 14 48 8b 57 08 31 f6 44 8b 4f 10 44 8b 07 48 8d 7d c8 e8 84 f8 ff ff 48 85 c0 48 89 c3 74 2a e8 f7 31 03 00 48 89 df <48> 8b 80 10 01 00 00 e8 68 fb 69 ed 48 8b 75 f0 65 48 33 34 25 28
> [Wed Feb 27 14:17:09 2019] RSP: 0018:ffffb42701ab7d40 EFLAGS: 00010282
> [Wed Feb 27 14:17:09 2019] RAX: 0000000000000000 RBX: ffff9d79637ee080 RCX: 0000000000000003
> [Wed Feb 27 14:17:09 2019] RDX: 0000000000000001 RSI: 0000000000000002 RDI: ffff9d79637ee080
> [Wed Feb 27 14:17:09 2019] RBP: ffffb42701ab7d78 R08: ffff9d796fae70e0 R09: ffff9d796f403500
> [Wed Feb 27 14:17:09 2019] R10: ffffb42701ab7d90 R11: 0000000000000000 R12: ffff9d7969d09240
> [Wed Feb 27 14:17:09 2019] R13: ffff9d79624e6840 R14: ffff9d7969d09318 R15: ffff9d796d48ff80
> [Wed Feb 27 14:17:09 2019] FS: 0000000000000000(0000) GS:ffff9d796fac0000(0000) knlGS:0000000000000000
> [Wed Feb 27 14:17:09 2019] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [Wed Feb 27 14:17:09 2019] CR2: 0000000000000110 CR3: 0000000427f22000 CR4: 00000000000006e0
> [Wed Feb 27 14:17:09 2019] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [Wed Feb 27 14:17:09 2019] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [Wed Feb 27 14:17:09 2019] Call Trace:
> [Wed Feb 27 14:17:09 2019] virtio_transport_recv_pkt+0x63/0x820 [vmw_vsock_virtio_transport_common]
> [Wed Feb 27 14:17:09 2019] ? kfree+0x17e/0x190
> [Wed Feb 27 14:17:09 2019] ? detach_buf_split+0x145/0x160
> [Wed Feb 27 14:17:09 2019] ? __switch_to_asm+0x40/0x70
> [Wed Feb 27 14:17:09 2019] virtio_transport_rx_work+0xa0/0x106 [vmw_vsock_virtio_transport]
> [Wed Feb 27 14:17:09 2019] NET: Registered protocol family 40
> [Wed Feb 27 14:17:09 2019] process_one_work+0x167/0x410
> [Wed Feb 27 14:17:09 2019] worker_thread+0x4d/0x460
> [Wed Feb 27 14:17:09 2019] kthread+0x105/0x140
> [Wed Feb 27 14:17:09 2019] ? rescuer_thread+0x360/0x360
> [Wed Feb 27 14:17:09 2019] ? kthread_destroy_worker+0x50/0x50
> [Wed Feb 27 14:17:09 2019] ret_from_fork+0x35/0x40
> [Wed Feb 27 14:17:09 2019] Modules linked in: vmw_vsock_virtio_transport vmw_vsock_virtio_transport_common input_leds vsock serio_raw i2c_piix4 mac_hid qemu_fw_cfg autofs4 cirrus ttm drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops virtio_net psmouse drm net_failover pata_acpi virtio_blk failover floppy
> [Wed Feb 27 14:17:09 2019] CR2: 0000000000000110
> [Wed Feb 27 14:17:09 2019] ---[ end trace baa35abd2e040fe5 ]---
> [Wed Feb 27 14:17:09 2019] RIP: 0010:virtio_transport_reset_no_sock+0x8c/0xc0 [vmw_vsock_virtio_transport_common]
> [Wed Feb 27 14:17:09 2019] Code: 35 8b 4f 14 48 8b 57 08 31 f6 44 8b 4f 10 44 8b 07 48 8d 7d c8 e8 84 f8 ff ff 48 85 c0 48 89 c3 74 2a e8 f7 31 03 00 48 89 df <48> 8b 80 10 01 00 00 e8 68 fb 69 ed 48 8b 75 f0 65 48 33 34 25 28
> [Wed Feb 27 14:17:09 2019] RSP: 0018:ffffb42701ab7d40 EFLAGS: 00010282
> [Wed Feb 27 14:17:09 2019] RAX: 0000000000000000 RBX: ffff9d79637ee080 RCX: 0000000000000003
> [Wed Feb 27 14:17:09 2019] RDX: 0000000000000001 RSI: 0000000000000002 RDI: ffff9d79637ee080
> [Wed Feb 27 14:17:09 2019] RBP: ffffb42701ab7d78 R08: ffff9d796fae70e0 R09: ffff9d796f403500
> [Wed Feb 27 14:17:09 2019] R10: ffffb42701ab7d90 R11: 0000000000000000 R12: ffff9d7969d09240
> [Wed Feb 27 14:17:09 2019] R13: ffff9d79624e6840 R14: ffff9d7969d09318 R15: ffff9d796d48ff80
> [Wed Feb 27 14:17:09 2019] FS: 0000000000000000(0000) GS:ffff9d796fac0000(0000) knlGS:0000000000000000
> [Wed Feb 27 14:17:09 2019] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [Wed Feb 27 14:17:09 2019] CR2: 0000000000000110 CR3: 0000000427f22000 CR4: 00000000000006e0
> [Wed Feb 27 14:17:09 2019] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [Wed Feb 27 14:17:09 2019] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> ---
> net/vmw_vsock/virtio_transport_common.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 3ae3a33da70b..502201aaff2a 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -662,6 +662,7 @@ static int virtio_transport_reset(struct vsock_sock *vsk,
> */
> static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> {
> + const struct virtio_transport *t;
> struct virtio_vsock_pkt_info info = {
> .op = VIRTIO_VSOCK_OP_RST,
> .type = le16_to_cpu(pkt->hdr.type),
> @@ -680,7 +681,11 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> if (!pkt)
> return -ENOMEM;
>
> - return virtio_transport_get_ops()->send_pkt(pkt);
> + t = virtio_transport_get_ops();
> + if (!t)
> + return -ENOTCONN;
Should be better to do this check before the virtio_transport_alloc_pkt?
Otherwise, I think we should free that packet before to return -ENOTCONN.
Thanks,
Stefano
> +
> + return t->send_pkt(pkt);
> }
>
> static void virtio_transport_wait_close(struct sock *sk, long timeout)
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
2019-03-05 18:01 [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock Adalbert Lazăr
2019-03-06 8:12 ` Stefano Garzarella
@ 2019-03-06 8:41 ` Stefan Hajnoczi
[not found] ` <1551863441.5559.19509.@c1753101230bd75c4bdbfe8f0947046bcaf69c6c>
1 sibling, 1 reply; 4+ messages in thread
From: Stefan Hajnoczi @ 2019-03-06 8:41 UTC (permalink / raw)
To: Adalbert Lazăr
Cc: Stefan Hajnoczi, David S . Miller, Stefano Garzarella,
virtualization, kvm, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]
On Tue, Mar 05, 2019 at 08:01:45PM +0200, Adalbert Lazăr wrote:
Thanks for the patch, Adalbert! Please add a Signed-off-by tag so your
patch can be merged (see Documentation/process/submitting-patches.rst
Chapter 11 for details on the Developer's Certificate of Origin).
> static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> {
> + const struct virtio_transport *t;
> struct virtio_vsock_pkt_info info = {
> .op = VIRTIO_VSOCK_OP_RST,
> .type = le16_to_cpu(pkt->hdr.type),
> @@ -680,7 +681,11 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> if (!pkt)
> return -ENOMEM;
>
> - return virtio_transport_get_ops()->send_pkt(pkt);
> + t = virtio_transport_get_ops();
> + if (!t)
> + return -ENOTCONN;
pkt is leaked here. This is an easy mistake to make because the code is
unclear. The pkt argument is the received packet that we must reply to.
The reply packet is allocated just before line 680 and must be free
explicitly for return -ENOTCONN.
You can avoid the leak and make the code easier to read like this:
struct virtio_vsock_pkt *reply;
...
------ avoid reusing 'pkt'
v
reply = virtio_transport_alloc_pkt(&info, 0, ...);
if (!reply)
return -ENOMEM;
t = virtio_transport_get_ops();
if (!t) {
virtio_transport_free_pkt(reply); <-- prevent memory leak
return -ENOTCONN;
}
return t->send_pkt(reply);
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
[not found] ` <1551863441.5559.19509.@c1753101230bd75c4bdbfe8f0947046bcaf69c6c>
@ 2019-03-06 17:02 ` Stefan Hajnoczi
0 siblings, 0 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2019-03-06 17:02 UTC (permalink / raw)
To: Adalbert Lazăr
Cc: Stefan Hajnoczi, David S . Miller, Stefano Garzarella,
virtualization, kvm, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]
On Wed, Mar 06, 2019 at 11:10:41AM +0200, Adalbert Lazăr wrote:
> On Wed, 6 Mar 2019 08:41:04 +0000, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Tue, Mar 05, 2019 at 08:01:45PM +0200, Adalbert Lazăr wrote:
> > The pkt argument is the received packet that we must reply to.
> > The reply packet is allocated just before line 680 and must be free
> > explicitly for return -ENOTCONN.
> >
> > You can avoid the leak and make the code easier to read like this:
> >
> > struct virtio_vsock_pkt *reply;
> >
> > ...
> >
> > ------ avoid reusing 'pkt'
> > v
> > reply = virtio_transport_alloc_pkt(&info, 0, ...);
> > if (!reply)
> > return -ENOMEM;
> >
> > t = virtio_transport_get_ops();
> > if (!t) {
> > virtio_transport_free_pkt(reply); <-- prevent memory leak
> > return -ENOTCONN;
> > }
> > return t->send_pkt(reply);
>
> What do you think about Stefano's suggestion, to move the check above
> the line were the reply is allocated?
That's fine too.
However a follow up patch to eliminate the confusing way that 'pkt' is
reused is still warranted. If you are busy I'd be happy to send that
cleanup.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-03-06 17:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-05 18:01 [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock Adalbert Lazăr
2019-03-06 8:12 ` Stefano Garzarella
2019-03-06 8:41 ` Stefan Hajnoczi
[not found] ` <1551863441.5559.19509.@c1753101230bd75c4bdbfe8f0947046bcaf69c6c>
2019-03-06 17:02 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox