Re: [PATCH v1] vhost: fix crash caused by accessing a freed vsocket

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Gongming,

On 5/10/24 09:28, Gongming Chen wrote:
> Hi Maxime and Chenbo,
> 
> Do you have any suggestions for how to address this?
> 
> Looking forward to hearing from you!

Could you please have a try with latest DPDK main branch,
and if it reproduces, rebase your series on top of it.

I don't think it has been fixed, but we've done significant changes in
fdman in this release so we need a rebase anyways.

Thanks in advance,
Maxime

> 
> Thanks,
> Gongming
> 
>> On Apr 3, 2024, at 11:52 PM, Gongming Chen <chengongming1900@outlook.com> wrote:
>>
>> Hi Maxime,
>> Thanks for review.
>>
>>> On Apr 3, 2024, at 5:39 PM, Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>>>
>>> Hi Gongming,
>>>
>>> It's the 9th time the patch has been sent.
>>> I'm not sure whether there are changes between them or these are just
>>> re-sends, but that's something to avoid.
>>>
>>
>> Sorry, there's something wrong with my mailbox.
>> I will send a v1 version as the latest patch, but they are actually the same.
>>
>>> If there are differences, you should use versionning to highlight it.
>>> If unsure, please check the contributions guidelines first.
>>>
>>> Regarding the patch itself, I don't know if this is avoidable, but I
>>> would prefer we do not introduce yet another lock in there.
>>>
>>> Thanks,
>>> Maxime
>>>
>>
>> I totally agree with your.
>> Therefore, initially I hoped to solve this problem without introducing
>> new lock. However, the result was not expected.
>>
>> 1. The vsocket is shared between the event and reconnect threads by
>> transmitting the vsocket pointer. Therefore, there is no way to protect
>> vsocket through a simple vsocket lock.
>>
>> 2. The event and reconnect threads can transmit vsocket pointers to
>> each other, so there is no way to ensure that vsocket will not be
>> accessed by locking the two threads separately.
>>
>> 3. Therefore, on the vsocket resource, event and reconnect are in the
>> same critical section. Only by locking two threads at the same time
>> can the vsocket be ensured that it will not be accessed and can be
>> freed safely.
>>
>> Currently, app config, event, and reconnect threads respectively have
>> locks corresponding to their own maintenance resources,
>> vhost_user.mutex, pfdset->fd_mutex, and reconn_list.mutex.
>>
>> I think there is a thread-level lock missing here to protect the
>> critical section between threads, just like the rcu scene protection.
>>
>> After app config acquires the write lock, it ensures that the event and
>> reconnect threads are outside the critical section.
>> This is to completely clean up the resources associated with vsocket
>> and safely free vsocket.
>>
>> Therefore, considering future expansion, if there may be more
>> resources like vsocket, this thread lock can also be used to ensure
>> that resources are safely released after complete cleanup.
>>
>> In this way, the threads will be clearer, and the complicated try lock
>> method is no longer needed.
>>
>> Thanks,
>> Gongming
>