Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <632465d0-e04c-4e10-abb9-a740d6e3dc30@uipath.com>
Date:   Mon, 23 Oct 2023 17:59:45 +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>
From:   Alexandru Matei <alexandru.matei@uipath.com>
In-Reply-To: <0624137c-85cf-4086-8256-af2b8405f434@uipath.com>
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?cTVxQlVYRkJSRXBuV09yVExJa0h2MVpBcGI5MnAwNEZtdkV4OEt6dzZBT00v?=
 =?utf-8?B?NG1weStBYVcwUkNjK0dPVEZUcStYUXJDZjUraTlobFZwR1FoaTFlTzVsYnk5?=
 =?utf-8?B?NUhPVzhFcTk0WGhMcE1Ra0FRMkZyQzdjcXB5VUl4UkZXZkpMcm5ob0o4UnlU?=
 =?utf-8?B?dHZYU1Z5R1VTbEJrSlNoM1Q5eXJCcGgzSDQ0Ri9Kc0FhcCt2allna3R4MDd1?=
 =?utf-8?B?VGd4ajRsTkNaK2VHbUZFV0FVa2lhVlVoY3NyZk5aVnNyMkE0VjZ6dkxrV2l5?=
 =?utf-8?B?UnNlekNxT1hSQkw4dEZnaGowa1NhV2RwM25UK282UmozUk5TZk5VOFFra3FV?=
 =?utf-8?B?SVJHOU5CcTlyTURZZ1RRd2svQk1vM3FJdnVyRGVCMWlVVTFhUFVqdlR3eURX?=
 =?utf-8?B?R0NTdkxmd0JYM1RBZStUclRnOVJpWW5HWWlzcXJtampoN0hKdE91SGoyZkx3?=
 =?utf-8?B?cGZNVS9tdmFIRVJscHBFSy81N0pUNjg1V0Z1N0RJNGtwYXV1YUQzOFFnSHlH?=
 =?utf-8?B?czhqT1RWVGhLYjduaWNFWlhoWGtCaEh6ZmUxM2JyZGtFSG1nQTZ2UFhMVjRF?=
 =?utf-8?B?bmY1dk81VDhXckFadHp0OGRhaHBTSmtHK212and2K2hYUHpiakJaWUlpT3Ax?=
 =?utf-8?B?dHpvQWVtbUhHVzZnSU9rZlNYdjRLejljdmNSdldhUnlzUmVkRnluV3FKQzVY?=
 =?utf-8?B?Y2Q3MkdKc2dqMG1RUkx6U2ZKYmdNcVJTYmU5cUtndi9EcUNPcTEwYnpneFBt?=
 =?utf-8?B?aHJtQjRxeXlRM1dxQkZlc2k0V2NId1lSTmZjM29aRVVWb0c1aTkycExKdTZs?=
 =?utf-8?B?cG1kbXdmRFhqTVpOSzFPS2lON1VwUTRvVWVUTjIyVHNqaHJabmMvSnpDTmk5?=
 =?utf-8?B?TXdhZWNhVXhYVFljcUZ0UXAxb1puMGpCcHIyMW1SdkQyMmw3Q1U2LzE4cWFM?=
 =?utf-8?B?ZStpOGFia25nR2I3NUdJMDdmS3VLb2tGS2lteU5GVFpZNGRLemZiYzdZNUo4?=
 =?utf-8?B?ZnUvR25qWmgvdzlrZzlYeUxhS1NRUTNJR0lFazZFcWZ3ZEREYWhDRklmOWlY?=
 =?utf-8?B?QTVtbTBFaU45MkFndHhSKzI2ajErYWZIRFVKZStwdld4MEdrQjA5V3VaN25B?=
 =?utf-8?B?M3k3WCtJNmNkaE1qcldnU1Rra2RvcjF6anB0TmpPRHFCUWlBYmlkK3J0YUhu?=
 =?utf-8?B?U29rRHYwdEQ2enhFL2trZ1JKbGxWV1BRc0hUZzVkaEhmckFFUW43RDllVjZ0?=
 =?utf-8?B?ZFBuSktyekxyK3I5enlBSEtZVC9oMy92aFA2SExzZjRXVlN6aTVQMnV6OTJH?=
 =?utf-8?B?Q1BIeHFCRHhpYkZNZ3JqUW9JT3RVVlF2TVBMek9Sb1hkd0IxYVJTc25NMjNv?=
 =?utf-8?B?aW5iSDZyd1AyRmo5azZKejJ2bHZobWpzRUZMWFp6V2tOQkNQZjJZVzM5Q2U3?=
 =?utf-8?B?K1ZsZHUrQkRqMjloY2FncU8xL3UwM0Rrcm9rMS9MQ0w5Z0QzaGwwbXcxU0th?=
 =?utf-8?B?ZlZWdTNJSko1ZzF5bjVTMCtScURTa0puQ0hYR3JUZDJ1ZCtCVUo0R1VhS0ZB?=
 =?utf-8?B?dEhiMDBsRXE5TWVnQ3lXUkZKRjdlenhjRWlvZ3hTc0RYUWwvZ2VDeHJKTU1M?=
 =?utf-8?B?QXF5dyswSmpyWDZzeHR3cU82SWlvd1VkS1VoeEhXT2pjQ290KzRyZVJaUWM0?=
 =?utf-8?B?R3d6bHZiUWI4Wmo5YVltTXczZzROYVdVSWdPbVN4dGYyZUNHblV2R2E4SnNI?=
 =?utf-8?B?Y3RHVHBuQTNrdCtjcUhvUU95NWZBTkJudUl5eUUySmpwNHFzdkZBd2cvaWJI?=
 =?utf-8?B?WHhRTE5UQXplRDQ2OGdJajUzaFNEVks2UzRScVNuR25wa1JWVkRtQkJtbHVI?=
 =?utf-8?B?QVMwcytCOWR3bzk2amFXd0dVMnZnTlR5MXhlYitra1J5ZThhU1NFUCt0MFlL?=
 =?utf-8?B?RnE1OFBFejgwQlQxYUZkSjl4ZElKY0ZzcHFDRDVQV2hyckFQTnBRdkg5Wk5W?=
 =?utf-8?B?QVVpQjY1T3ZpS3VyRUhHbzVocmp5ak5lNXdkSTBZRDJqTm9DM2JKamgzYWpn?=
 =?utf-8?B?Q3FpUFh5V091ZWdHY1p1cmdVTWcra2dQZlZHQmVhdFFLSFRwQUwrN250NFlS?=
 =?utf-8?B?YldhTHlzUGE1MTc5YkVUREFNMHo5Z0NTYzhYYnA3SkJhbVJEdExHS2pYNEhv?=
 =?utf-8?B?dlE9PQ==?=
X-OriginatorOrg: uipath.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a81bcb6f-d26b-4c17-4314-08dbd3d8b387
X-MS-Exchange-CrossTenant-AuthSource: VI1PR02MB4527.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Oct 2023 14:59:48.4235
 (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: JbFMuoGpGbhbtXZwEBxctlOcVYcWarTk5r/kzwQMtMwHaZzgdhE5R73nLRCrIVOXUNwrwDDIEYdgx2sTYA2ozvaviMiCTIJ1oooZzUW3+WE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS2PR02MB9606
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 groat.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 (groat.vger.email [0.0.0.0]); Mon, 23 Oct 2023 08:00:12 -0700 (PDT)

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

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.

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