[v2] vsock/virtio: initialize the_virtio_vsock before using VQs

[PATCH v2] vsock/virtio: initialize the_virtio_vsock before using VQs

Posted by Alexandru Matei 2 years, 2 months ago

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>
---
v2: 
- split virtio_vsock_vqs_init in vqs_init and vqs_fill and moved 
  the_virtio_vsock initialization after vqs_init

 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 e95df847176b..92738d1697c1 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -559,6 +559,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;
@@ -568,8 +573,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)
@@ -664,6 +667,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);
 
@@ -736,6 +740,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);
-- 
2.34.1

Re: [PATCH v2] vsock/virtio: initialize the_virtio_vsock before using VQs

Posted by Stefano Garzarella 2 years, 2 months ago

On Mon, Oct 23, 2023 at 05:08:33PM +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.
>
>Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
>Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
>---
>v2:
>- split virtio_vsock_vqs_init in vqs_init and vqs_fill and moved
>  the_virtio_vsock initialization after vqs_init
>
> 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 e95df847176b..92738d1697c1 100644
>--- a/net/vmw_vsock/virtio_transport.c
>+++ b/net/vmw_vsock/virtio_transport.c
>@@ -559,6 +559,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)

What about renaming this function in virtio_vsock_vqs_start() and move 
also the setting of `tx_run` here?

Thanks,
Stefano

>+{
> 	mutex_lock(&vsock->rx_lock);
> 	virtio_vsock_rx_fill(vsock);
> 	vsock->rx_run = true;
>@@ -568,8 +573,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)
>@@ -664,6 +667,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);
>
>@@ -736,6 +740,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);
>-- 
>2.34.1
>

Re: [PATCH v2] vsock/virtio: initialize the_virtio_vsock before using VQs

Posted by Alexandru Matei 2 years, 2 months ago

On 10/23/2023 5:29 PM, Stefano Garzarella wrote:
> On Mon, Oct 23, 2023 at 05:08:33PM +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.
>>
>> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
>> Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
>> ---
>> v2:
>> - split virtio_vsock_vqs_init in vqs_init and vqs_fill and moved
>>  the_virtio_vsock initialization after vqs_init
>>
>> 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 e95df847176b..92738d1697c1 100644
>> --- a/net/vmw_vsock/virtio_transport.c
>> +++ b/net/vmw_vsock/virtio_transport.c
>> @@ -559,6 +559,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)
> 
> What about renaming this function in virtio_vsock_vqs_start() and move also the setting of `tx_run` here?

It works but in this case we also need to move rcu_assign_pointer in virtio_vsock_vqs_start(), 
the assignment needs to be right after setting tx_run to true and before filling the VQs.

> 
> Thanks,
> Stefano
> 
>> +{
>>     mutex_lock(&vsock->rx_lock);
>>     virtio_vsock_rx_fill(vsock);
>>     vsock->rx_run = true;
>> @@ -568,8 +573,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)
>> @@ -664,6 +667,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);
>>
>> @@ -736,6 +740,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);
>> -- 
>> 2.34.1
>>
>

Re: [PATCH v2] vsock/virtio: initialize the_virtio_vsock before using VQs

Posted by Alexandru Matei 2 years, 2 months ago

On 10/23/2023 5:52 PM, Alexandru Matei wrote:
> On 10/23/2023 5:29 PM, Stefano Garzarella wrote:
>> On Mon, Oct 23, 2023 at 05:08:33PM +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.
>>>
>>> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
>>> Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
>>> ---
>>> v2:
>>> - split virtio_vsock_vqs_init in vqs_init and vqs_fill and moved
>>>  the_virtio_vsock initialization after vqs_init
>>>
>>> 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 e95df847176b..92738d1697c1 100644
>>> --- a/net/vmw_vsock/virtio_transport.c
>>> +++ b/net/vmw_vsock/virtio_transport.c
>>> @@ -559,6 +559,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)
>>
>> What about renaming this function in virtio_vsock_vqs_start() and move also the setting of `tx_run` here?
> 
> It works but in this case we also need to move rcu_assign_pointer in virtio_vsock_vqs_start(), 
> the assignment needs to be right after setting tx_run to true and before filling the VQs.
> 

And if we move rcu_assign_pointer then there is no need to split the function in two,
We can move rcu_assign_pointer() directly inside virtio_vsock_vqs_init() after setting tx_run.

>>
>> Thanks,
>> Stefano
>>
>>> +{
>>>     mutex_lock(&vsock->rx_lock);
>>>     virtio_vsock_rx_fill(vsock);
>>>     vsock->rx_run = true;
>>> @@ -568,8 +573,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)
>>> @@ -664,6 +667,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);
>>>
>>> @@ -736,6 +740,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);
>>> -- 
>>> 2.34.1
>>>
>>

Re: [PATCH v2] vsock/virtio: initialize the_virtio_vsock before using VQs

Posted by Stefano Garzarella 2 years, 2 months ago

On Mon, Oct 23, 2023 at 05:59:45PM +0300, Alexandru Matei wrote:
>On 10/23/2023 5:52 PM, Alexandru Matei wrote:
>> On 10/23/2023 5:29 PM, Stefano Garzarella wrote:
>>> On Mon, Oct 23, 2023 at 05:08:33PM +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.
>>>>
>>>> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
>>>> Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
>>>> ---
>>>> v2:
>>>> - split virtio_vsock_vqs_init in vqs_init and vqs_fill and moved
>>>>  the_virtio_vsock initialization after vqs_init
>>>>
>>>> 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 e95df847176b..92738d1697c1 100644
>>>> --- a/net/vmw_vsock/virtio_transport.c
>>>> +++ b/net/vmw_vsock/virtio_transport.c
>>>> @@ -559,6 +559,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)
>>>
>>> What about renaming this function in virtio_vsock_vqs_start() and move also the setting of `tx_run` here?
>>
>> It works but in this case we also need to move rcu_assign_pointer in virtio_vsock_vqs_start(),
>> the assignment needs to be right after setting tx_run to true and before filling the VQs.

Why?

If `rx_run` is false, we shouldn't need to send replies to the host 
IIUC.

If we need this instead, please add a comment in the code, but also in 
the commit, because it's not clear why.

>>
>
>And if we move rcu_assign_pointer then there is no need to split the function in two,
>We can move rcu_assign_pointer() directly inside virtio_vsock_vqs_init() after setting tx_run.

Yep, this could be another option, but we need to change the name of 
that function in this case.

Stefano

>
>>>
>>> Thanks,
>>> Stefano
>>>
>>>> +{
>>>>     mutex_lock(&vsock->rx_lock);
>>>>     virtio_vsock_rx_fill(vsock);
>>>>     vsock->rx_run = true;
>>>> @@ -568,8 +573,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)
>>>> @@ -664,6 +667,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);
>>>>
>>>> @@ -736,6 +740,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);
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>

Re: [PATCH v2] vsock/virtio: initialize the_virtio_vsock before using VQs

Posted by Alexandru Matei 2 years, 2 months ago

On 10/23/2023 6:13 PM, Stefano Garzarella wrote:
> On Mon, Oct 23, 2023 at 05:59:45PM +0300, Alexandru Matei wrote:
>> On 10/23/2023 5:52 PM, Alexandru Matei wrote:
>>> On 10/23/2023 5:29 PM, Stefano Garzarella wrote:
>>>> On Mon, Oct 23, 2023 at 05:08:33PM +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.
>>>>>
>>>>> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
>>>>> Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
>>>>> ---
>>>>> v2:
>>>>> - split virtio_vsock_vqs_init in vqs_init and vqs_fill and moved
>>>>>  the_virtio_vsock initialization after vqs_init
>>>>>
>>>>> 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 e95df847176b..92738d1697c1 100644
>>>>> --- a/net/vmw_vsock/virtio_transport.c
>>>>> +++ b/net/vmw_vsock/virtio_transport.c
>>>>> @@ -559,6 +559,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)
>>>>
>>>> What about renaming this function in virtio_vsock_vqs_start() and move also the setting of `tx_run` here?
>>>
>>> It works but in this case we also need to move rcu_assign_pointer in virtio_vsock_vqs_start(),
>>> the assignment needs to be right after setting tx_run to true and before filling the VQs.
> 
> Why?
> 
> If `rx_run` is false, we shouldn't need to send replies to the host IIUC.
> 
> If we need this instead, please add a comment in the code, but also in the commit, because it's not clear why.
> 

We need rcu_assign_pointer after setting tx_run to true for connections that are initiated from the guest -> host. 
virtio_transport_connect() calls virtio_transport_send_pkt(). Once 'the_virtio_vsock' is initialized, virtio_transport_send_pkt() will queue the packet,
but virtio_transport_send_pkt_work() will exit if tx_run is false. 

>>>
>>
>> And if we move rcu_assign_pointer then there is no need to split the function in two,
>> We can move rcu_assign_pointer() directly inside virtio_vsock_vqs_init() after setting tx_run.
> 
> Yep, this could be another option, but we need to change the name of that function in this case.
> 

OK, how does virtio_vsock_vqs_setup() sound?

> Stefano
> 
>>
>>>>
>>>> Thanks,
>>>> Stefano
>>>>
>>>>> +{
>>>>>     mutex_lock(&vsock->rx_lock);
>>>>>     virtio_vsock_rx_fill(vsock);
>>>>>     vsock->rx_run = true;
>>>>> @@ -568,8 +573,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)
>>>>> @@ -664,6 +667,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);
>>>>>
>>>>> @@ -736,6 +740,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);
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>>
>>
>

Re: [PATCH v2] vsock/virtio: initialize the_virtio_vsock before using VQs

Posted by Stefano Garzarella 2 years, 2 months ago

On Mon, Oct 23, 2023 at 06:36:21PM +0300, Alexandru Matei wrote:
>On 10/23/2023 6:13 PM, Stefano Garzarella wrote:
>> On Mon, Oct 23, 2023 at 05:59:45PM +0300, Alexandru Matei wrote:
>>> On 10/23/2023 5:52 PM, Alexandru Matei wrote:
>>>> On 10/23/2023 5:29 PM, Stefano Garzarella wrote:
>>>>> On Mon, Oct 23, 2023 at 05:08:33PM +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.
>>>>>>
>>>>>> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
>>>>>> Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
>>>>>> ---
>>>>>> v2:
>>>>>> - split virtio_vsock_vqs_init in vqs_init and vqs_fill and moved
>>>>>>  the_virtio_vsock initialization after vqs_init
>>>>>>
>>>>>> 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 e95df847176b..92738d1697c1 100644
>>>>>> --- a/net/vmw_vsock/virtio_transport.c
>>>>>> +++ b/net/vmw_vsock/virtio_transport.c
>>>>>> @@ -559,6 +559,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)
>>>>>
>>>>> What about renaming this function in virtio_vsock_vqs_start() and move also the setting of `tx_run` here?
>>>>
>>>> It works but in this case we also need to move rcu_assign_pointer in virtio_vsock_vqs_start(),
>>>> the assignment needs to be right after setting tx_run to true and before filling the VQs.
>>
>> Why?
>>
>> If `rx_run` is false, we shouldn't need to send replies to the host IIUC.
>>
>> If we need this instead, please add a comment in the code, but also in the commit, because it's not clear why.
>>
>
>We need rcu_assign_pointer after setting tx_run to true for connections 
>that are initiated from the guest -> host.
>virtio_transport_connect() calls virtio_transport_send_pkt().  Once 
>'the_virtio_vsock' is initialized, virtio_transport_send_pkt() will 
>queue the packet,
>but virtio_transport_send_pkt_work() will exit if tx_run is false.

Okay, but in this case we could safely queue &vsock->send_pkt_work after 
finishing initialization to send those packets queued earlier.

In the meantime I'll try to see if we can leave the initialization of 
`the_virtio_vsock` as the ulitmate step and maybe go out first in the 
workers if it's not set.

That way just queue all the workers after everything is done and we 
should be fine.

>
>>>>
>>>
>>> And if we move rcu_assign_pointer then there is no need to split the function in two,
>>> We can move rcu_assign_pointer() directly inside virtio_vsock_vqs_init() after setting tx_run.
>>
>> Yep, this could be another option, but we need to change the name of that function in this case.
>>
>
>OK, how does virtio_vsock_vqs_setup() sound?

Or virtio_vsock_start() (without vqs)

Stefano

>
>> Stefano
>>
>>>
>>>>>
>>>>> Thanks,
>>>>> Stefano
>>>>>
>>>>>> +{
>>>>>>     mutex_lock(&vsock->rx_lock);
>>>>>>     virtio_vsock_rx_fill(vsock);
>>>>>>     vsock->rx_run = true;
>>>>>> @@ -568,8 +573,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)
>>>>>> @@ -664,6 +667,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);
>>>>>>
>>>>>> @@ -736,6 +740,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);
>>>>>> -- 
>>>>>> 2.34.1
>>>>>>
>>>>>
>>>
>>
>

Re: [PATCH v2] vsock/virtio: initialize the_virtio_vsock before using VQs

Posted by Alexandru Matei 2 years, 2 months ago

On 10/23/2023 7:10 PM, Stefano Garzarella wrote:
> On Mon, Oct 23, 2023 at 06:36:21PM +0300, Alexandru Matei wrote:
>> On 10/23/2023 6:13 PM, Stefano Garzarella wrote:
>>> On Mon, Oct 23, 2023 at 05:59:45PM +0300, Alexandru Matei wrote:
>>>> On 10/23/2023 5:52 PM, Alexandru Matei wrote:
>>>>> On 10/23/2023 5:29 PM, Stefano Garzarella wrote:
>>>>>> On Mon, Oct 23, 2023 at 05:08:33PM +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.
>>>>>>>
>>>>>>> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
>>>>>>> Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> - split virtio_vsock_vqs_init in vqs_init and vqs_fill and moved
>>>>>>>  the_virtio_vsock initialization after vqs_init
>>>>>>>
>>>>>>> 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 e95df847176b..92738d1697c1 100644
>>>>>>> --- a/net/vmw_vsock/virtio_transport.c
>>>>>>> +++ b/net/vmw_vsock/virtio_transport.c
>>>>>>> @@ -559,6 +559,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)
>>>>>>
>>>>>> What about renaming this function in virtio_vsock_vqs_start() and move also the setting of `tx_run` here?
>>>>>
>>>>> It works but in this case we also need to move rcu_assign_pointer in virtio_vsock_vqs_start(),
>>>>> the assignment needs to be right after setting tx_run to true and before filling the VQs.
>>>
>>> Why?
>>>
>>> If `rx_run` is false, we shouldn't need to send replies to the host IIUC.
>>>
>>> If we need this instead, please add a comment in the code, but also in the commit, because it's not clear why.
>>>
>>
>> We need rcu_assign_pointer after setting tx_run to true for connections that are initiated from the guest -> host.
>> virtio_transport_connect() calls virtio_transport_send_pkt().  Once 'the_virtio_vsock' is initialized, virtio_transport_send_pkt() will queue the packet,
>> but virtio_transport_send_pkt_work() will exit if tx_run is false.
> 
> Okay, but in this case we could safely queue &vsock->send_pkt_work after finishing initialization to send those packets queued earlier.
> 
> In the meantime I'll try to see if we can leave the initialization of `the_virtio_vsock` as the ulitmate step and maybe go out first in the workers if it's not set.
> 
> That way just queue all the workers after everything is done and we should be fine.
> 

Yep, Thanks for input, I'll send another patch with this approach.
I'll keep virtio_vsock_vqs_init() split in virtio_vsock_vqs_init() and virtio_vsock_vqs_start(), 
move tx_run setting in virtio_vsock_vqs_start() and queue &vsock->send_pkt_work after virtio_vsock_vqs_start() is called. 

>>
>>>>>
>>>>
>>>> And if we move rcu_assign_pointer then there is no need to split the function in two,
>>>> We can move rcu_assign_pointer() directly inside virtio_vsock_vqs_init() after setting tx_run.
>>>
>>> Yep, this could be another option, but we need to change the name of that function in this case.
>>>
>>
>> OK, how does virtio_vsock_vqs_setup() sound?
> 
> Or virtio_vsock_start() (without vqs)
> 
> Stefano
> 
>>
>>> Stefano
>>>
>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Stefano
>>>>>>
>>>>>>> +{
>>>>>>>     mutex_lock(&vsock->rx_lock);
>>>>>>>     virtio_vsock_rx_fill(vsock);
>>>>>>>     vsock->rx_run = true;
>>>>>>> @@ -568,8 +573,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)
>>>>>>> @@ -664,6 +667,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);
>>>>>>>
>>>>>>> @@ -736,6 +740,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);
>>>>>>> -- 
>>>>>>> 2.34.1
>>>>>>>
>>>>>>
>>>>
>>>
>>
>