Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <674f7593-5421-4ecf-a0e0-2c46ab42a051@uipath.com>
Date:   Tue, 24 Oct 2023 21:27:00 +0300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3] vsock/virtio: initialize the_virtio_vsock before using
 VQs
To:     Stefano Garzarella <sgarzare@redhat.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>,
        Alexandru Matei <alexandru.matei@uipath.com>
References: <20231023192207.1804-1-alexandru.matei@uipath.com>
 <iqjmblf2n42w7afw42udxvju3znupmwrixfsbwcn247u7bayoc@zrbken7ls6m7>
Content-Language: en-US
From:   Alexandru Matei <alexandru.matei@uipath.com>
In-Reply-To: <iqjmblf2n42w7afw42udxvju3znupmwrixfsbwcn247u7bayoc@zrbken7ls6m7>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SU1TMm5ZSlRWRjVSdHNtcmR3MkRCbU9tZ0tXSmtlS0s1VzBFaDUvVlZPRDJq?=
 =?utf-8?B?cDVmbnNZQzU5aGhxcHdnSDdRNDk4MTJrL0k3c1ZXOWtMa3dVT2xjYUpLbnBp?=
 =?utf-8?B?STlLNVAybVhqRlNvTmlTblpGU0VIMEJyOTZLSWVBbUhyaW1WMEZTMUgvNFFH?=
 =?utf-8?B?eWZBQUY5amhHcHBya214MTFIbkU2SzU5QWw3SGhSZThqb3VvQjhuS2R3ZGJM?=
 =?utf-8?B?TVJwWUZ4VWJCSU9iWFJTbFN6L3Rjdys1VUtBSWw1S1BoOHhqc3lVVUJhbE1x?=
 =?utf-8?B?TDA5OHRvb1pCREMvNG11RU5lWW1sRktkR3RiellnazZHWWlOeGpIanBEdkhJ?=
 =?utf-8?B?bk5pTllzN0R4Wk0weEs1TWU2VldFUEQrc1R2eCtXNnozMmRZL3N4OEJHQlJG?=
 =?utf-8?B?TzlYeHJaMjI5NlVkTWw5RmJuQ25qNXVNS2VHOVZObnZiaXJCTVZKSEt5NXVH?=
 =?utf-8?B?QjJRRHRPdkZPUFowRVpQTDNyTEhuWENELzVqK3RWcEs5OTZRTm5EZE1LT1R5?=
 =?utf-8?B?ak9vMmN1SmE3TUFtUzhsRkFTNnBNQXBsRE5JN2x6R1Ura0dmbWZ4RDVCb0s0?=
 =?utf-8?B?aFY4ZGpBNUVBdEpQemtaTStFbVA4SVZ5eXptUzQweUxCLy9WMFdac0FsQk1M?=
 =?utf-8?B?UkFCWXcrWFduSDl3YkJEMWxpQ2ZOK29YbGV0TmgxSHJuYmo5QUNsMnZpN0lz?=
 =?utf-8?B?R0FmeFpBaURqbnV6V0F1bTlucUZtdFBlQmxTNHBkRm9IZk5SWXF3RjNaNDc0?=
 =?utf-8?B?ZEo1VG82NHBUaXdXcDBmRUpPSkNUNzVBUzlhamRucjdmNDh1MUJROWtyNGFH?=
 =?utf-8?B?NGpxWkprZmM4Z2c5d2UvZzFRMGZLaVZwUjFvQ1dKSloxNzRHODFicUx2Wk5C?=
 =?utf-8?B?aElCNDBndjZ4SUphNjlwM2pCWkl6MGZQVVpaeGdteG9xU1pRMlpwR2dFRWly?=
 =?utf-8?B?QjFRdGxURkNKdSs3elJyUWtSYjV4QXJyU2pweWxtQUtiT3k2UzRadFo1cXBT?=
 =?utf-8?B?ZWV0WHlLa0pKTGw2RGtkVTlkWnNxUGpBdnlEckR0SHNJczZiWU9OM053VndB?=
 =?utf-8?B?VXpaUmxzQTNzS1M3N2pFdlU1cU1Tcy9MQWorWmNUbkNUR2JVUzFpcjlDaGZ0?=
 =?utf-8?B?cjBUY3hCb25kbmhCdm4rRHJQWVVmcXZ2YUFvMENNeXd0a1BiUWJoRDlkU1lN?=
 =?utf-8?B?eGZOSjVXOFNOMmZnUFo0S21sYXk4Q3FkSDZBUUlxbEE4SzdmODFQTUFDaHBy?=
 =?utf-8?B?c2xhbGxib2s5aHJZOFYvbGtpb2hSTDhtZ0hBVUdjK0YrUGV1N2tKSzVRRG5w?=
 =?utf-8?B?SWFzanA4dVJoVE0zTnkvS3V0cXBFcHRGWkJsUTJMUHNyY2ZuUFB1c3I2dml5?=
 =?utf-8?B?aEJTTU9TUFhLa2ZuMnJOd20zYTh3VmlacTQ2ZkJWY3ZqZi85V0JxaC82bkVO?=
 =?utf-8?B?YVFaK2NhSjlSNHlYZTh3WkQ2U2VGaGg2aTlkb1BKbm9hQS8vY056NndNT01w?=
 =?utf-8?B?cmVQZ3A3UWd0TGtvbmxOdkRZTC8xNlNWbHFyL3l1LzE2aGNSU0NiVFhMcjFz?=
 =?utf-8?B?SlF4OWRtNHAvdmw3aEFWdnpaOHRnVDBxbTljWHlRbTF3b25mQ3ZOT1FmSE9B?=
 =?utf-8?B?cFVKSW9yaW12cjlUNkkzRHF3TVBLZjRWQXpoaThjRHVYWnRQUEZSYUFER2dW?=
 =?utf-8?B?c3grcDNGSlc5dXhGRWFIcllWMEdIa0s4bzNjZ3Y3K0VXZVhWTUdweVhJVHBU?=
 =?utf-8?B?cXBlUUxwUStqUGtLQXE4M2RxUUtTSkdXak4wZWNDNTlsdnNLU3Y0TFY1VmlW?=
 =?utf-8?B?K3duWThEbU5yaVhjSk16MWNLV2ZMOEpkNmFJTVhsZ3ZJVm4wbEt0TmFOSzFO?=
 =?utf-8?B?cE9xUUcxaWZjWG11OGZLbjJiRml1a3ZCbmVuS3VYR3BvVy9IZGdsajJmdHc4?=
 =?utf-8?B?MUpxVEUvSVgvS3ZFVmJncUxPRkc3M2t6NC9CcUdLbklNK3Z4NDk0UU5iaHpm?=
 =?utf-8?B?NXBmVWQzS2p5aXZQZTZWTzZZeDl2Y3lGa1dRNURJR1h3VG04M0FzdExYVlVp?=
 =?utf-8?B?ZG85bVJ6aWJpZm83anBRTnN3aVB4ZTkrbWQ4TDF1MlJTMmhvVHVOYlEyRUIz?=
 =?utf-8?B?Q3AwTG0zQTlaYzlQR0x3bXgweGNuMml6cEV4VWdHRHNpcDgwVUl3Ym5mMGRL?=
 =?utf-8?B?ZEE9PQ==?=
X-OriginatorOrg: uipath.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c1131d7f-a67c-4a9c-0a19-08dbd4bed40a
X-MS-Exchange-CrossTenant-AuthSource: VI1PR02MB4527.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Oct 2023 18:27:07.1456
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: d8353d2a-b153-4d17-8827-902c51f72357
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: SJxRuZJXhOAxQXZHI2eFetMt3PRPEnREK+uZKhffYlqRHyiIPVMatBDRxpOeizo6QhefGoMxb1XeHVbkGWaA5J6PJte16q7B32OYh0Hb/KA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS2PR02MB9488
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Tue, 24 Oct 2023 11:27:28 -0700 (PDT)

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@uipath.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
>>
>