Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Date:   Mon, 23 Oct 2023 18:10:23 +0200
From:   Stefano Garzarella <sgarzare@redhat.com>
To:     Alexandru Matei <alexandru.matei@uipath.com>
Cc:     Stefan Hajnoczi <stefanha@redhat.com>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, kvm@vger.kernel.org,
        virtualization@lists.linux-foundation.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Mihai Petrisor <mihai.petrisor@uipath.com>,
        Viorel Canja <viorel.canja@uipath.com>
Subject: Re: [PATCH v2] vsock/virtio: initialize the_virtio_vsock before
 using VQs
Message-ID: <jyjfsjvfmmr7ucf53v6p57scdxah64bgvd2lj7l4hbjwiyd2ct@lj3ejlseqvog>
References: <20231023140833.11206-1-alexandru.matei@uipath.com>
 <2tc56vwgs5xwqzfqbv5vud346uzagwtygdhkngdt3wjqaslbmh@zauky5czyfkg>
 <0624137c-85cf-4086-8256-af2b8405f434@uipath.com>
 <632465d0-e04c-4e10-abb9-a740d6e3dc30@uipath.com>
 <dynlbzmgtr35byn5etbar33ufhweii6gk2pct5wpqxpqubchce@cltop4aar7r6>
 <01ad7d00-9a53-445b-8916-3342047112a0@uipath.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <01ad7d00-9a53-445b-8916-3342047112a0@uipath.com>
Precedence: bulk

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