Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
On Wed, 6 Mar 2019 17:02:16 +, Stefan Hajnoczi wrote:
> On Wed, Mar 06, 2019 at 11:10:41AM +0200, Adalbert Lazăr wrote:
> > On Wed, 6 Mar 2019 08:41:04 +, Stefan Hajnoczi
> > 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(, 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
I've got it, a couple of minutes after I've replied :)
The second version[1] should be in your mailbox.
Thank you,
Adalbert
[1]: https://patchwork.kernel.org/patch/10840787/
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
On Wed, 6 Mar 2019 08:41:04 +, Stefan Hajnoczi wrote:
> 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.
Thank you for your kind words :)
> 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(, 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?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
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 0110
> [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: [#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:b42701ab7d40 EFLAGS: 00010282
> [Wed Feb 27 14:17:09 2019] RAX: RBX: 9d79637ee080 RCX:
> 0003
> [Wed Feb 27 14:17:09 2019] RDX: 0001 RSI: 0002 RDI:
> 9d79637ee080
> [Wed Feb 27 14:17:09 2019] RBP: b42701ab7d78 R08: 9d796fae70e0 R09:
> 9d796f403500
> [Wed Feb 27 14:17:09 2019] R10: b42701ab7d90 R11: R12:
> 9d7969d09240
> [Wed Feb 27 14:17:09 2019] R13: 9d79624e6840 R14: 9d7969d09318 R15:
> 9d796d48ff80
> [Wed Feb 27 14:17:09 2019] FS: ()
> GS:9d796fac() knlGS:
> [Wed Feb 27 14:17:09 2019] CS: 0010 DS: ES: CR0: 80050033
> [Wed Feb 27 14:17:09 2019] CR2: 0110 CR3: 000427f22000 CR4:
> 06e0
> [Wed Feb 27 14:17:09 2019] DR0: DR1: DR2:
>
> [Wed Feb 27 14:17:09 2019] DR3: DR6: fffe0ff0 DR7:
> 0400
> [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: 0110
> [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:b42701ab7d40 EFLAGS: 00010282
> [Wed Feb 27 14:17:09 2019] RAX: RBX: 9d79637ee080 RCX:
> 0003
> [Wed Feb 27 14:17:09 2019] RDX: 0001 RSI: 0002 RDI:
> 9d79637ee080
> [Wed Feb 27 14:17:09 2019] RBP: b42701ab7d78 R08: 9d796fae70e0 R09:
> 9d796f403500
> [Wed Feb 27 14:17:09 2019] R10: b42701ab7d90 R11: R12:
> 9d7969d09240
> [Wed Feb 27 14:17:09 2019] R13: 9d79624e6840 R14: 9d7969d09318 R15:
> 9d796d48ff80
> [Wed Feb 27 14:17:09 2019] FS: ()
> GS:9d796fac() knlGS:
> [Wed Feb 27 14:17:09 2019] CS: 0010 DS: ES: CR0: 80050033
> [Wed Feb 27 14:17:09 2019] CR2: 0110 CR3: 000427f22000 CR4:
> 06e0
> [Wed Feb 27 14:17:09 2019] DR0: DR1: DR2:
>
> [Wed Feb 27 14:17:09
Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
On Wed, 6 Mar 2019 09:12:36 +0100, Stefano Garzarella
wrote:
> > --- 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.
Right! :D
I will send a second version.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
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 0110
[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: [#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:b42701ab7d40 EFLAGS: 00010282
[Wed Feb 27 14:17:09 2019] RAX: RBX: 9d79637ee080 RCX:
0003
[Wed Feb 27 14:17:09 2019] RDX: 0001 RSI: 0002 RDI:
9d79637ee080
[Wed Feb 27 14:17:09 2019] RBP: b42701ab7d78 R08: 9d796fae70e0 R09:
9d796f403500
[Wed Feb 27 14:17:09 2019] R10: b42701ab7d90 R11: R12:
9d7969d09240
[Wed Feb 27 14:17:09 2019] R13: 9d79624e6840 R14: 9d7969d09318 R15:
9d796d48ff80
[Wed Feb 27 14:17:09 2019] FS: ()
GS:9d796fac() knlGS:
[Wed Feb 27 14:17:09 2019] CS: 0010 DS: ES: CR0: 80050033
[Wed Feb 27 14:17:09 2019] CR2: 0110 CR3: 000427f22000 CR4:
06e0
[Wed Feb 27 14:17:09 2019] DR0: DR1: DR2:
[Wed Feb 27 14:17:09 2019] DR3: DR6: fffe0ff0 DR7:
0400
[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: 0110
[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:b42701ab7d40 EFLAGS: 00010282
[Wed Feb 27 14:17:09 2019] RAX: RBX: 9d79637ee080 RCX:
0003
[Wed Feb 27 14:17:09 2019] RDX: 0001 RSI: 0002 RDI:
9d79637ee080
[Wed Feb 27 14:17:09 2019] RBP: b42701ab7d78 R08: 9d796fae70e0 R09:
9d796f403500
[Wed Feb 27 14:17:09 2019] R10: b42701ab7d90 R11: R12:
9d7969d09240
[Wed Feb 27 14:17:09 2019] R13: 9d79624e6840 R14: 9d7969d09318 R15:
9d796d48ff80
[Wed Feb 27 14:17:09 2019] FS: ()
GS:9d796fac() knlGS:
[Wed Feb 27 14:17:09 2019] CS: 0010 DS: ES: CR0: 80050033
[Wed Feb 27 14:17:09 2019] CR2: 0110 CR3: 000427f22000 CR4:
06e0
[Wed Feb 27 14:17:09 2019] DR0: DR1: DR2:
[Wed Feb 27 14:17:09 2019] DR3: DR6: fffe0ff0 DR7:
0400
---
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
Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
On Wed, Mar 06, 2019 at 11:10:41AM +0200, Adalbert Lazăr wrote:
> On Wed, 6 Mar 2019 08:41:04 +, Stefan Hajnoczi 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(, 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
signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
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(, 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
signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
