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

[Alexandru Matei <alexandru.matei@uipath.com>]

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
>>>>>
>>>>
>>
>