* [PATCH] vsock: initialize the_virtio_vsock before using VQs
@ 2023-10-18 18:32 Alexandru Matei
2023-10-19 0:12 ` Vadim Fedorenko
2023-10-19 8:54 ` Stefano Garzarella
0 siblings, 2 replies; 6+ messages in thread
From: Alexandru Matei @ 2023-10-18 18:32 UTC (permalink / raw)
To: Stefan Hajnoczi, Stefano Garzarella, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: kvm, virtualization, netdev, linux-kernel, Alexandru Matei,
Mihai Petrisor, Viorel Canja
Once VQs are filled with empty buffers and we kick the host, it can send
connection requests. If 'the_virtio_vsock' is not initialized before,
replies are silently dropped and do not reach the host.
Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
---
net/vmw_vsock/virtio_transport.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index e95df847176b..eae0867133f8 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -658,12 +658,13 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
vsock->seqpacket_allow = true;
vdev->priv = vsock;
+ rcu_assign_pointer(the_virtio_vsock, vsock);
ret = virtio_vsock_vqs_init(vsock);
- if (ret < 0)
+ if (ret < 0) {
+ rcu_assign_pointer(the_virtio_vsock, NULL);
goto out;
-
- rcu_assign_pointer(the_virtio_vsock, vsock);
+ }
mutex_unlock(&the_virtio_vsock_mutex);
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] vsock: initialize the_virtio_vsock before using VQs
2023-10-18 18:32 [PATCH] vsock: initialize the_virtio_vsock before using VQs Alexandru Matei
@ 2023-10-19 0:12 ` Vadim Fedorenko
2023-10-19 21:12 ` Alexandru Matei
2023-10-19 8:54 ` Stefano Garzarella
1 sibling, 1 reply; 6+ messages in thread
From: Vadim Fedorenko @ 2023-10-19 0:12 UTC (permalink / raw)
To: Alexandru Matei, Stefan Hajnoczi, Stefano Garzarella,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: kvm, virtualization, netdev, linux-kernel, Mihai Petrisor,
Viorel Canja
On 18/10/2023 19:32, Alexandru Matei wrote:
> Once VQs are filled with empty buffers and we kick the host, it can send
> connection requests. If 'the_virtio_vsock' is not initialized before,
> replies are silently dropped and do not reach the host.
>
> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
> Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
> ---
> net/vmw_vsock/virtio_transport.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index e95df847176b..eae0867133f8 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -658,12 +658,13 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> vsock->seqpacket_allow = true;
>
> vdev->priv = vsock;
> + rcu_assign_pointer(the_virtio_vsock, vsock);
>
> ret = virtio_vsock_vqs_init(vsock);
> - if (ret < 0)
> + if (ret < 0) {
> + rcu_assign_pointer(the_virtio_vsock, NULL);
> goto out;
> -
> - rcu_assign_pointer(the_virtio_vsock, vsock);
> + }
>
> mutex_unlock(&the_virtio_vsock_mutex);
>
Looks like virtio_vsock_restore() needs the same changes. But
virtio_vsock_vqs_init() can fail only in virtio_find_vqs(). Maybe it can
be split into 2 functions to avoid second rcu_assign_pointer() in case
of error?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vsock: initialize the_virtio_vsock before using VQs
2023-10-18 18:32 [PATCH] vsock: initialize the_virtio_vsock before using VQs Alexandru Matei
2023-10-19 0:12 ` Vadim Fedorenko
@ 2023-10-19 8:54 ` Stefano Garzarella
2023-10-19 21:12 ` Alexandru Matei
1 sibling, 1 reply; 6+ messages in thread
From: Stefano Garzarella @ 2023-10-19 8:54 UTC (permalink / raw)
To: Alexandru Matei
Cc: Stefan Hajnoczi, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, kvm, virtualization, netdev, linux-kernel,
Mihai Petrisor, Viorel Canja
On Wed, Oct 18, 2023 at 09:32:47PM +0300, Alexandru Matei wrote:
>Once VQs are filled with empty buffers and we kick the host, it can send
>connection requests. If 'the_virtio_vsock' is not initialized before,
>replies are silently dropped and do not reach the host.
Are replies really dropped or we just miss the notification?
Could the reverse now happen, i.e., the guest wants to send a connection
request, finds the pointer assigned but can't use virtqueues because
they haven't been initialized yet?
Perhaps to avoid your problem, we could just queue vsock->rx_work at the
bottom of the probe to see if anything was queued in the meantime.
Nit: please use "vsock/virtio" to point out that this problem is of the
virtio transport.
Thanks,
Stefano
>
>Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
>Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
>---
> net/vmw_vsock/virtio_transport.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>index e95df847176b..eae0867133f8 100644
>--- a/net/vmw_vsock/virtio_transport.c
>+++ b/net/vmw_vsock/virtio_transport.c
>@@ -658,12 +658,13 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> vsock->seqpacket_allow = true;
>
> vdev->priv = vsock;
>+ rcu_assign_pointer(the_virtio_vsock, vsock);
>
> ret = virtio_vsock_vqs_init(vsock);
>- if (ret < 0)
>+ if (ret < 0) {
>+ rcu_assign_pointer(the_virtio_vsock, NULL);
> goto out;
>-
>- rcu_assign_pointer(the_virtio_vsock, vsock);
>+ }
>
> mutex_unlock(&the_virtio_vsock_mutex);
>
>--
>2.34.1
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vsock: initialize the_virtio_vsock before using VQs
2023-10-19 8:54 ` Stefano Garzarella
@ 2023-10-19 21:12 ` Alexandru Matei
2023-10-20 7:23 ` Stefano Garzarella
0 siblings, 1 reply; 6+ messages in thread
From: Alexandru Matei @ 2023-10-19 21:12 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Stefan Hajnoczi, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, kvm, virtualization, netdev, linux-kernel,
Mihai Petrisor, Viorel Canja
On 10/19/2023 11:54 AM, Stefano Garzarella wrote:
> On Wed, Oct 18, 2023 at 09:32:47PM +0300, Alexandru Matei wrote:
>> Once VQs are filled with empty buffers and we kick the host, it can send
>> connection requests. If 'the_virtio_vsock' is not initialized before,
>> replies are silently dropped and do not reach the host.
>
> Are replies really dropped or we just miss the notification?
>
> Could the reverse now happen, i.e., the guest wants to send a connection request, finds the pointer assigned but can't use virtqueues because they haven't been initialized yet?
>
> Perhaps to avoid your problem, we could just queue vsock->rx_work at the bottom of the probe to see if anything was queued in the meantime.
>
> Nit: please use "vsock/virtio" to point out that this problem is of the virtio transport.
>
> Thanks,
> Stefano
The replies are dropped , the scenario goes like this:
Once rx_run is set to true and rx queue is filled with empty buffers, the host sends a connection request.
The request is processed in virtio_transport_recv_pkt(), and since there is no bound socket, it calls virtio_transport_reset_no_sock() which tries to send a reset packet.
In virtio_transport_send_pkt() it checks 'the_virtio_vsock' and because it is null it exits with -ENODEV, basically dropping the packet.
I looked on your scenario and there is an issue from the moment we set the_virtio_vsock (in this patch) up until vsock->tx_run is set to TRUE.
virtio_transport_send_pkt() will queue the packet, but virtio_transport_send_pkt_work() will exit because tx_run is FALSE. This could be fixed by moving rcu_assign_pointer() after tx_run is set to TRUE.
virtio_transport_cancel_pkt() uses the rx virtqueue once the_virtio_vsock is set, so rcu_assign_pointer() should be moved after virtio_find_vqs() is called.
I think the way to go is to split virtio_vsock_vqs_init() in two: virtio_vsock_vqs_init() and virtio_vsock_vqs_fill(), as Vadim suggested. This should fix all the cases:
---
net/vmw_vsock/virtio_transport.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index ad64f403536a..1f95f98ddd3f 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -594,6 +594,11 @@ static int virtio_vsock_vqs_init(struct virtio_vsock *vsock)
vsock->tx_run = true;
mutex_unlock(&vsock->tx_lock);
+ return 0;
+}
+
+static void virtio_vsock_vqs_fill(struct virtio_vsock *vsock)
+{
mutex_lock(&vsock->rx_lock);
virtio_vsock_rx_fill(vsock);
vsock->rx_run = true;
@@ -603,8 +608,6 @@ static int virtio_vsock_vqs_init(struct virtio_vsock *vsock)
virtio_vsock_event_fill(vsock);
vsock->event_run = true;
mutex_unlock(&vsock->event_lock);
-
- return 0;
}
static void virtio_vsock_vqs_del(struct virtio_vsock *vsock)
@@ -707,6 +710,7 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
goto out;
rcu_assign_pointer(the_virtio_vsock, vsock);
+ virtio_vsock_vqs_fill(vsock);
mutex_unlock(&the_virtio_vsock_mutex);
@@ -779,6 +783,7 @@ static int virtio_vsock_restore(struct virtio_device *vdev)
goto out;
rcu_assign_pointer(the_virtio_vsock, vsock);
+ virtio_vsock_vqs_fill(vsock);
out:
mutex_unlock(&the_virtio_vsock_mutex);
--
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] vsock: initialize the_virtio_vsock before using VQs
2023-10-19 0:12 ` Vadim Fedorenko
@ 2023-10-19 21:12 ` Alexandru Matei
0 siblings, 0 replies; 6+ messages in thread
From: Alexandru Matei @ 2023-10-19 21:12 UTC (permalink / raw)
To: Vadim Fedorenko, Stefan Hajnoczi, Stefano Garzarella,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: kvm, virtualization, netdev, linux-kernel, Mihai Petrisor,
Viorel Canja
On 10/19/2023 3:12 AM, Vadim Fedorenko wrote:
> On 18/10/2023 19:32, Alexandru Matei wrote:
>> Once VQs are filled with empty buffers and we kick the host, it can send
>> connection requests. If 'the_virtio_vsock' is not initialized before,
>> replies are silently dropped and do not reach the host.
>>
>> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
>> Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
>> ---
>> net/vmw_vsock/virtio_transport.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>> index e95df847176b..eae0867133f8 100644
>> --- a/net/vmw_vsock/virtio_transport.c
>> +++ b/net/vmw_vsock/virtio_transport.c
>> @@ -658,12 +658,13 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
>> vsock->seqpacket_allow = true;
>> vdev->priv = vsock;
>> + rcu_assign_pointer(the_virtio_vsock, vsock);
>> ret = virtio_vsock_vqs_init(vsock);
>> - if (ret < 0)
>> + if (ret < 0) {
>> + rcu_assign_pointer(the_virtio_vsock, NULL);
>> goto out;
>> -
>> - rcu_assign_pointer(the_virtio_vsock, vsock);
>> + }
>> mutex_unlock(&the_virtio_vsock_mutex);
>>
>
> Looks like virtio_vsock_restore() needs the same changes. But
> virtio_vsock_vqs_init() can fail only in virtio_find_vqs(). Maybe it can be split into 2 functions to avoid second rcu_assign_pointer() in case
> of error?
>
Thanks, it definitely looks better and it solves a few other issues by splitting the function in two.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vsock: initialize the_virtio_vsock before using VQs
2023-10-19 21:12 ` Alexandru Matei
@ 2023-10-20 7:23 ` Stefano Garzarella
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2023-10-20 7:23 UTC (permalink / raw)
To: Alexandru Matei
Cc: Stefan Hajnoczi, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, kvm, virtualization, netdev, linux-kernel,
Mihai Petrisor, Viorel Canja
On Fri, Oct 20, 2023 at 12:12:04AM +0300, Alexandru Matei wrote:
>On 10/19/2023 11:54 AM, Stefano Garzarella wrote:
>> On Wed, Oct 18, 2023 at 09:32:47PM +0300, Alexandru Matei wrote:
>>> Once VQs are filled with empty buffers and we kick the host, it can send
>>> connection requests. If 'the_virtio_vsock' is not initialized before,
>>> replies are silently dropped and do not reach the host.
>>
>> Are replies really dropped or we just miss the notification?
>>
>> Could the reverse now happen, i.e., the guest wants to send a connection request, finds the pointer assigned but can't use virtqueues because they haven't been initialized yet?
>>
>> Perhaps to avoid your problem, we could just queue vsock->rx_work at the bottom of the probe to see if anything was queued in the meantime.
>>
>> Nit: please use "vsock/virtio" to point out that this problem is of the virtio transport.
>>
>> Thanks,
>> Stefano
>
>The replies are dropped , the scenario goes like this:
>
> Once rx_run is set to true and rx queue is filled with empty buffers, the host sends a connection request.
Oh, I see now, I thought virtio_transport_rx_work() returned early if
'the_virtio_vsock' was not set.
> The request is processed in virtio_transport_recv_pkt(), and since there is no bound socket, it calls virtio_transport_reset_no_sock() which tries to send a reset packet.
> In virtio_transport_send_pkt() it checks 'the_virtio_vsock' and because it is null it exits with -ENODEV, basically dropping the packet.
>
>I looked on your scenario and there is an issue from the moment we set the_virtio_vsock (in this patch) up until vsock->tx_run is set to TRUE.
>virtio_transport_send_pkt() will queue the packet, but virtio_transport_send_pkt_work() will exit because tx_run is FALSE. This could be fixed by moving rcu_assign_pointer() after tx_run is set to TRUE.
>virtio_transport_cancel_pkt() uses the rx virtqueue once the_virtio_vsock is set, so rcu_assign_pointer() should be moved after virtio_find_vqs() is called.
>
>I think the way to go is to split virtio_vsock_vqs_init() in two:
>virtio_vsock_vqs_init() and virtio_vsock_vqs_fill(), as Vadim
>suggested. This should fix all the cases:
Yep, LGTM!
Thank you both for the fix, please send a v2 with this approach!
Stefano
>
>---
> net/vmw_vsock/virtio_transport.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>index ad64f403536a..1f95f98ddd3f 100644
>--- a/net/vmw_vsock/virtio_transport.c
>+++ b/net/vmw_vsock/virtio_transport.c
>@@ -594,6 +594,11 @@ static int virtio_vsock_vqs_init(struct virtio_vsock *vsock)
> vsock->tx_run = true;
> mutex_unlock(&vsock->tx_lock);
>
>+ return 0;
>+}
>+
>+static void virtio_vsock_vqs_fill(struct virtio_vsock *vsock)
>+{
> mutex_lock(&vsock->rx_lock);
> virtio_vsock_rx_fill(vsock);
> vsock->rx_run = true;
>@@ -603,8 +608,6 @@ static int virtio_vsock_vqs_init(struct virtio_vsock *vsock)
> virtio_vsock_event_fill(vsock);
> vsock->event_run = true;
> mutex_unlock(&vsock->event_lock);
>-
>- return 0;
> }
>
> static void virtio_vsock_vqs_del(struct virtio_vsock *vsock)
>@@ -707,6 +710,7 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> goto out;
>
> rcu_assign_pointer(the_virtio_vsock, vsock);
>+ virtio_vsock_vqs_fill(vsock);
>
> mutex_unlock(&the_virtio_vsock_mutex);
>
>@@ -779,6 +783,7 @@ static int virtio_vsock_restore(struct virtio_device *vdev)
> goto out;
>
> rcu_assign_pointer(the_virtio_vsock, vsock);
>+ virtio_vsock_vqs_fill(vsock);
>
> out:
> mutex_unlock(&the_virtio_vsock_mutex);
>--
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-10-20 7:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-18 18:32 [PATCH] vsock: initialize the_virtio_vsock before using VQs Alexandru Matei
2023-10-19 0:12 ` Vadim Fedorenko
2023-10-19 21:12 ` Alexandru Matei
2023-10-19 8:54 ` Stefano Garzarella
2023-10-19 21:12 ` Alexandru Matei
2023-10-20 7:23 ` Stefano Garzarella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).