netdev - Re: [PATCH v3] vsock/virtio: initialize the_virtio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <674f7593-5421-4ecf-a0e0-2c46ab42a051@uipath.com>
Date: Tue, 24 Oct 2023 21:27:00 +0300
From: Alexandru Matei <alexandru.matei@...ath.com>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: Stefan Hajnoczi <stefanha@...hat.com>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, kvm@...r.kernel.org,
 virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, Mihai Petrisor <mihai.petrisor@...ath.com>,
 Viorel Canja <viorel.canja@...ath.com>,
 Alexandru Matei <alexandru.matei@...ath.com>
Subject: Re: [PATCH v3] vsock/virtio: initialize the_virtio_vsock before using
 VQs

On 10/24/2023 10:22 AM, Stefano Garzarella wrote:
> On Mon, Oct 23, 2023 at 10:22:07PM +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.
>>
>> virtio_transport_send_pkt() can queue packets once the_virtio_vsock is
>> set, but they won't be processed until vsock->tx_run is set to true. We
>> queue vsock->send_pkt_work when initialization finishes to send those
>> packets queued earlier.
>>
>> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
>> Signed-off-by: Alexandru Matei <alexandru.matei@...ath.com>
>> ---
>> v3:
>> - renamed vqs_fill to vqs_start and moved tx_run initialization to it
>> - queued send_pkt_work at the end of initialization to send packets queued earlier
>> 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 | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>> index e95df847176b..c0333f9a8002 100644
>> --- a/net/vmw_vsock/virtio_transport.c
>> +++ b/net/vmw_vsock/virtio_transport.c
>> @@ -555,6 +555,11 @@ static int virtio_vsock_vqs_init(struct virtio_vsock *vsock)
>>
>>     virtio_device_ready(vdev);
>>
>> +    return 0;
>> +}
>> +
>> +static void virtio_vsock_vqs_start(struct virtio_vsock *vsock)
>> +{
>>     mutex_lock(&vsock->tx_lock);
>>     vsock->tx_run = true;
>>     mutex_unlock(&vsock->tx_lock);
>> @@ -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,9 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
>>         goto out;
>>
>>     rcu_assign_pointer(the_virtio_vsock, vsock);
>> +    virtio_vsock_vqs_start(vsock);
>> +
>> +    queue_work(virtio_vsock_workqueue, &vsock->send_pkt_work);
> 
> I would move this call in virtio_vsock_vqs_start() adding also a comment on top, bringing back what you wrote in the commit. Something like this:
> 
>         /* virtio_transport_send_pkt() can queue packets once
>          * the_virtio_vsock is set, but they won't be processed until
>          * vsock->tx_run is set to true. We queue vsock->send_pkt_work
>          * when initialization finishes to send those packets queued
>          * earlier.
>          */
> 
> Just as a consideration, we don't need to queue the other workers (rx, event) because as long as we don't fill the queues with empty buffers, the host can't send us any notification. (We could add it in the comment if you want).
> 

Thanks. Sure, I added it in the comment.

> The rest LGTM!
> 
> Thanks,
> Stefano
> 
>>
>>     mutex_unlock(&the_virtio_vsock_mutex);
>>
>> @@ -736,6 +742,9 @@ static int virtio_vsock_restore(struct virtio_device *vdev)
>>         goto out;
>>
>>     rcu_assign_pointer(the_virtio_vsock, vsock);
>> +    virtio_vsock_vqs_start(vsock);
>> +
>> +    queue_work(virtio_vsock_workqueue, &vsock->send_pkt_work);
>>
>> out:
>>     mutex_unlock(&the_virtio_vsock_mutex);
>> -- 
>> 2.25.1
>>
>