Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <01ad7d00-9a53-445b-8916-3342047112a0@uipath.com>
Date:   Mon, 23 Oct 2023 18:36:21 +0300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] 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: <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>
Content-Language: en-US
From:   Alexandru Matei <alexandru.matei@uipath.com>
In-Reply-To: <dynlbzmgtr35byn5etbar33ufhweii6gk2pct5wpqxpqubchce@cltop4aar7r6>
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?S3JIemt0KzdUUVVkK29rWGpKV1BMT1BROHJhcWNsMDRVUHVrQ1AycDI0aGNi?=
 =?utf-8?B?YlNtbmI3eEFJMEpTNjRVM2lxc05rS1BmWXN6SG5sMXJpNDNLVTVJNXhKY1ZH?=
 =?utf-8?B?MCt5dHJlTDh5N0E5QXJjMFpUeFhQMWFad0tFV1Z1Z2psYXBkV21kVlR6eHlK?=
 =?utf-8?B?MTNwNEt1Q044RXAraEZBUDM1c0hEaGZ1cy92UlhBVENFNld1bHFRUGNqRXRo?=
 =?utf-8?B?aXkxNFFrUU5yb080RFVVRTNLZWl0cmVXMk5DaWVpbVhCcERKM3k4Sjk1N2Jr?=
 =?utf-8?B?cGdrY0tJYjgyekNqOGNucmtrcEhjYTBMeGNRL2JZM0NTSEJXQzN2cnNMYUdJ?=
 =?utf-8?B?dFpyNkZsRnpPK1VTVTFyRDVNQ1ZpaWZlK3lMQlJmR3FERGZPTDlDdFJDcnJN?=
 =?utf-8?B?ZHVPR3ExRWxFYVFxOENjM0p0bHptRE52WHZ6bGgycjlnWno1QUd0R3VKZXB6?=
 =?utf-8?B?N2MyWXFHK1h3YytpVk5Zd1NxVmNlam5IdnJyZUhyaGw2Wkc2L0pET0U5V3Ji?=
 =?utf-8?B?K1I1V25FTjNPVGJzWU5udWdQMFYwWmZIUUxrS3psS1JiNW5nb1QwbXpXdHE5?=
 =?utf-8?B?bWFaWUdzVzhNMWRpeit5N3hyWlhvT3B5RXBDZy9GTTlsQnV0NkVRY3NzUXNK?=
 =?utf-8?B?R3B0WGVsL3NjOE0xektXc05qeUhMeGNqR1dBUTIrYk1Cdy9vTjFsd2ZvU3dF?=
 =?utf-8?B?TldrSGtqUXZpVkdLY3cxWXE3eFVkZXl1aHVOb3FxRU9JREdkamliL0ZqNEgz?=
 =?utf-8?B?NWh2QkRPeTJ6Q3JDa0pDbWREaWg3Wk9uVkhCMENlQTNSN3Z5QnNMNFNhMGt3?=
 =?utf-8?B?RGVGekM4LzQ2UHlnbnpJNzg3SlNnWGxtT3BabDZ4Sk9iei95ZnAyTm9aSC80?=
 =?utf-8?B?WDErUmgxSzdNbzRadWlDaHQ1ampWNHZ0MlV4ZjZRdE1NM0FrUnA1SEJsREg3?=
 =?utf-8?B?MVJOZlB5RlgwUDIvVzFGaWlZSHhEakhKUE1KelY4MVRMK0RQUzRZb3JjTGZB?=
 =?utf-8?B?dEV4Wmx1dkFzR1QvTXRMRHNPQjFvc0QycUhLNzF0VmR0Qk5hRTVuTjhVa3R2?=
 =?utf-8?B?OXVDVDdndTZFdzZlckhhdXBXNlczemF1amlPNjhHRlVzcVZCcWU4dTZweXNm?=
 =?utf-8?B?TGNtbHZXalIzWFpxVGxLWkEwZk1Fak41Wng4S2RwN2RlYnE3aVZVTVA0KzVE?=
 =?utf-8?B?NmJoMTFQeFpibkZiVzdkb0dxbVN2aEVGaUNzRkU2NGVac0g0Tmo5WVZzTlVQ?=
 =?utf-8?B?Y0VuSlNaMDNEaGlNRVA0OEVWaWlmbENUalF2cVhSK2FEL2hXRTRTbkF3eFkw?=
 =?utf-8?B?U0l6elFSS3lXd0hvamJhMk5iKzdUNEFGOEgrTnNCUnVWVVhEZGM2Z3hNWUY1?=
 =?utf-8?B?UDAwdVZZbXJpY2RvSnpPMU1ycjY5UThTZTNzRmtPYWpVWFhoTW1XU3FjT2xS?=
 =?utf-8?B?V1BWODc5My85YXJ4aDYwdWFuTG15d1pJd3V2THhhQ2ljQU5mNUQyc3AzR2or?=
 =?utf-8?B?SWxxaTNaSW5CdzJESERnZG5TcFl3Q1luZ2NsTTU1SlVtdHgyNFc0dEhWWVNt?=
 =?utf-8?B?ZGtobXdhdFZsbUE1RDBLd1lFeFdOVUJpZ3hUWkc0MklVU2l4UXNoODJEdWtH?=
 =?utf-8?B?d3plRkVZcTZSaDdlSlFBRzJjYWJCZHAzanE0YmprL21vZkplejhjNnZNUmFR?=
 =?utf-8?B?bFlITnJrcEpNbVFjT1I0Uk5SNXZEQzFGckRxWGZUSHR1UUxzUmxCaUhMNUY5?=
 =?utf-8?B?NU9FSmd0dXB6MEUyWUJPNHpnR3pELzNNN1ZmblFJUUkxbk90TUh6K1laTWZM?=
 =?utf-8?B?dUhpY1ovelJhdG1hUlNuVVlKZWRFRVFrNkNGdko5MmM1Z0xvZndBT3FCdDhT?=
 =?utf-8?B?QUFpdHRmQ0hGZUc5b1lkN2p4czdDTXJqa1RVQlVJWW02LzIyNjIybDI1K3lZ?=
 =?utf-8?B?cnhBdUVCS1hUclRxV2hwbGxGSjUyVEloc3pTN3VqZUxCbkl1bVFoeEEzQkpi?=
 =?utf-8?B?VUtFZU43ODlmcWdYVnJ3aFY2TWg0MWFmajVRSFlkWE9URUpGZ3RnTWNSRGt3?=
 =?utf-8?B?ZWl3ZTRMTFkzSmRTaUFRVmtPTHRzcXV3ODFsdWxoODcweVRpQVgreUc1V2xO?=
 =?utf-8?B?dnhzbktNcWNkNmhHWk05N1ZUbk9pMEZhYiszUjlaVnJ1cUJaRXlkMkxGZVFX?=
 =?utf-8?B?K3c9PQ==?=
X-OriginatorOrg: uipath.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0bf37684-1fa0-498a-a07e-08dbd3ddd607
X-MS-Exchange-CrossTenant-AuthSource: VI1PR02MB4527.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Oct 2023 15:36:33.6746
 (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: cnQFj2ZzGE4+EAvHRlAsX6DVrVqL80MCizZAvlPR2+GnJS2fPJkYEWqiwVlt41dVgNEwk/uVS1Hsr4WyPpYUmgmwQuqUyodyWOS5ghHsuQQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PR3PR02MB6427
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 morse.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 (morse.vger.email [0.0.0.0]); Mon, 23 Oct 2023 08:36:53 -0700 (PDT)

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