Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <e7929cb0-ccb1-46c9-99bf-2c6035eb7c01@uipath.com>
Date:   Mon, 23 Oct 2023 19:28:30 +0300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] vsock/virtio: initialize the_virtio_vsock before using
 VQs
Content-Language: en-US
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>
 <01ad7d00-9a53-445b-8916-3342047112a0@uipath.com>
 <jyjfsjvfmmr7ucf53v6p57scdxah64bgvd2lj7l4hbjwiyd2ct@lj3ejlseqvog>
From:   Alexandru Matei <alexandru.matei@uipath.com>
In-Reply-To: <jyjfsjvfmmr7ucf53v6p57scdxah64bgvd2lj7l4hbjwiyd2ct@lj3ejlseqvog>
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?Y3NIWFdyTFlmd05JaSsxSFoyYmxLdG9LUCt6a014RGVBZE13QStPTlpWNzRY?=
 =?utf-8?B?SVlHakVCeWdYSk5jUGtCZ0ZlYllHdjU3NlkzL0dHZVg5eWQyWVJnVjNxTUVZ?=
 =?utf-8?B?VklEYXNROTQxQ2dlR1B6amhVNTZyZEZMRjNWdlRuU24rRDdZdW53L2szTFNi?=
 =?utf-8?B?MW9EOHI3amIzVEZxTHY0K2tlOUU5U0oyRXI3M01QNWVWWWlGUm94czRJajRG?=
 =?utf-8?B?MXpCODZaaTV0OWc3VGlmVkJXRldTVmFUVnhrNit2c1BGYXV3UHA5dFJsL0ty?=
 =?utf-8?B?U0ZSdUREa2t4cDJNS0dUMGNjQmZZdWZ0VjlLTDlQRTlhb1BkeFYvc0ViYVE3?=
 =?utf-8?B?OXFWdWJJWXVvWDJjemowck5MclQ3SlllSThrenpWdVNVK2EwWXhlVW9wTzhH?=
 =?utf-8?B?a0Y4eWZPTkFLZ2o2MWVhVXB0TWw2QzByY3QrdnZMWWVWZnM3dkhjSDRteExF?=
 =?utf-8?B?S2JYK2tMMWp3YVdsMFFTODY0Vm5sU1Q2VDdXK000Zi9jVXZQWmdtVVkreS9y?=
 =?utf-8?B?Y3hQTGxPejUxMmo4MVdINkpTZEl0YzczeEZQbjRULytISnYxNXFoZnNwVVBk?=
 =?utf-8?B?aDNZYUZWbVpSVnhpVkpxb2lrLzlHaHlwVTlEcXBBWE5tRENUYXd2Qlk4TjNC?=
 =?utf-8?B?WGlwbFI3UmRWcDBhWVNjTmhsdHJvbEVDc01HVHc2MWlab09Yc2dudnBjSG9o?=
 =?utf-8?B?WGNBeVY3Q29mZVYyY2F6RUoyUjlyOGJUMHBkTEQxRWo5ZHJxdE1iVU01a1N6?=
 =?utf-8?B?M0dUb0Fiem5IQm9Zb1p4UXMvRmNsRE1tUXQ1VzNSZTBFWmJ1N3Vja3FndFpp?=
 =?utf-8?B?dmdhb3ZTYURCTk1NdlZMUVdrY2ZBaEgxSE50WUdKL3Y5MkNRaHVSckk3Tmtv?=
 =?utf-8?B?V3g1M1RPTFA5SldmYzVnUTQ2UHFvaU81L1JFeDRjRWxSSFUzUXcrdEE5TWVC?=
 =?utf-8?B?VEdGYTFSdjhYSWRqVVZmMjNaZDBsejd3ZnRyOFFBbFl3SlVRREdBY3V3VzFp?=
 =?utf-8?B?RjdMc1c2Ujc5SFRVTXF6bngveVFKZHJzNmJXRFJINmczcEMwUlNIYmVuWUJN?=
 =?utf-8?B?R0N5TnVrTmRUN1NBQlp3cVp3TnBiRUJ0ZEpCVmIxTHA2UXBtVVU3QTREUFdh?=
 =?utf-8?B?WFM4VDNyL21wZEVvRTQweklNYW03OUtzVU9NdTZ3T3Y2WmVnTUxkdUpuQjdT?=
 =?utf-8?B?cHdPSkpZOWU0UTgxUmNMZW1hKy95dmMyS1hmTndNR1B0aWl0ZTB5RFZYMFI5?=
 =?utf-8?B?MDB6U0p2MVlqTytHT25YT29XVTI4VEJmQkFLbXQzTmhJdjl2OGI1RVlvWWl2?=
 =?utf-8?B?MUlYWDA3cFdaYnBpQ3RkQkhSZDRPaHlxbTVHUTZvUGROcWx0QzVwVkFHUlNQ?=
 =?utf-8?B?WmMzTS9zOTdyM2RuelRlQ1RCMnBNYjFVMEVVTVF3UnVHRXc2TWxndERWUmFQ?=
 =?utf-8?B?QkI2SnU4bEhTS0pLcTVsR1ZDK2VkdnFzWkNibFI1WFRwUThOdVNCMXlpUzRm?=
 =?utf-8?B?ME5pdWdIT0FTVGhXdDZXUWJ6SmY4eHBDL0xTd25MUnFUektVTTIrRFRmV0M2?=
 =?utf-8?B?UFlteFVndFlydHdFUXJRQkdiNExLNk5kSGN1MWdwemZtcG5XMTdyeG1Yb0dr?=
 =?utf-8?B?MElKZGRtNENSbEIxUExBQk9adjlGK3V3VnlUZ2dSSjhGeWxOSktRS21rL3dm?=
 =?utf-8?B?UzlHWU5PcDRTZWZ1RUs0ZnhvVWErSEtFT2dqYjVtMm9nR1pwT0VsRUw3UEgw?=
 =?utf-8?B?WG5sbXM5Z2FKT0xGSHZ6ZmV3ZWt2SkQ5eCtlV0xmbUhMOXFqUHpmdk5sYzZJ?=
 =?utf-8?B?VWM3eDZPSzdnMTJwYjc4UkJsWUhxNkpmWFNGT1hKM1JCaXFka2F5aDc0ZUlt?=
 =?utf-8?B?R2hDVVk0T1NsOVAvYXBNbk5ReHdKZ1RkbEE4RDNjYnQrcmdzVEY0Y3QyZTZS?=
 =?utf-8?B?YUt5RmZCdStaa0pod1RBZERHa2d1bzBEc2ZPZmhMVktMamJSNUtzVDhEb3Q5?=
 =?utf-8?B?dHkxVVM2VzU3TDltMlZJdGZlRk0rQk5tbm91YmFYdXIvR3lLTkoxSDFtOVVS?=
 =?utf-8?B?WXZyUDAvWGx1Skx1SzJsSjBXbmxwYW5rTlQrcnJQV3pWRkhsWHkxTVJPUnNE?=
 =?utf-8?B?MUkvZDNiQWZ6T1Bkb0lNUmFDbWNjT1JPeFZieDJxRDA2TkFQZEV2NGdnbGRN?=
 =?utf-8?B?NVE9PQ==?=
X-OriginatorOrg: uipath.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 039ca1dc-65b0-4484-519b-08dbd3e51922
X-MS-Exchange-CrossTenant-AuthSource: VI1PR02MB4527.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Oct 2023 16:28:32.7868
 (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: DoPk4wt6xZLiKdpWJz/NJ0GVmMbMHdJi1BbvmIOV4LKgdlZCrNc1f+k9RpvcXYmyFSxIG+sn/bclvjbl4In5k5nqw9VdSL4q36L4d/5iG2k=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA4PR02MB6669
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]); Mon, 23 Oct 2023 09:28:48 -0700 (PDT)

On 10/23/2023 7:10 PM, Stefano Garzarella wrote:
> 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.
> 

Yep, Thanks for input, I'll send another patch with this approach.
I'll keep virtio_vsock_vqs_init() split in virtio_vsock_vqs_init() and virtio_vsock_vqs_start(), 
move tx_run setting in virtio_vsock_vqs_start() and queue &vsock->send_pkt_work after virtio_vsock_vqs_start() is called. 

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