On 11/11/20 8:57 AM, Xia, Chenbo wrote:
> Hi Xueming & Maxime,
>
>> -----Original Message-----
>> From: Xueming(Steven) Li <[email protected]>
>> Sent: Wednesday, November 11, 2020 2:02 PM
>> To: Maxime Coquelin <[email protected]>; [email protected]; Ding, Xuan
>> <[email protected]>; [email protected]; NBU-Contact-Thomas
>> Monjalon <[email protected]>; [email protected]; Xia, Chenbo
>> <[email protected]>
>> Subject: RE: [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup
>>
>> Hi Maxime,
>>
>> Near end of this function, if vhost_check_queue_inflights_packed() and
>> vhost_check_queue_inflights_split() return with error, is the fd expected
>> to be
>> closed by closing vq?
>
> I thought about this before. In theory, it will not cause fd leak because the
> fd
> is saved in vq. It will be closed upon next kick msg or vhost device destroy.
> But
> thinking it again, maybe it's better to close it now since anyway it's
> useless now😊
>
> What do you think?
I did it on purpose, as indeed it is saved in the vq metadata at that
stage.
The goal of the series being to avoid leaks, I think the patch does what
is necessary.
There is a function to cleanup the FDs and memory saved in the metadata,
so let it be done there.
Thanks,
Maxime
> Thanks,
> Chenbo
>
>>
>>> -----Original Message-----
>>> From: dev <[email protected]> On Behalf Of Maxime Coquelin
>>> Sent: Monday, November 9, 2020 8:17 PM
>>> To: [email protected]; [email protected]; [email protected];
>>> NBU-Contact-Thomas Monjalon <[email protected]>; [email protected];
>>> [email protected]
>>> Cc: Maxime Coquelin <[email protected]>
>>> Subject: [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup
>>>
>>> This patch fixes a file descriptor leak which happens in the error path
>> of
>>> vhost_user_set_vring_kick().
>>>
>>> Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")
>>> Cc: [email protected]
>>>
>>> Signed-off-by: Maxime Coquelin <[email protected]>
>>> Reviewed-by: Chenbo Xia <[email protected]>
>>> ---
>>> lib/librte_vhost/vhost_user.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c
>> b/lib/librte_vhost/vhost_user.c index
>>> 94b066f0b9..f3b2adabac 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -1855,8 +1855,12 @@ vhost_user_set_vring_kick(struct virtio_net **pdev,
>>> struct VhostUserMsg *msg,
>>>
>>> /* Interpret ring addresses only when ring is started. */
>>> dev = translate_ring_addresses(dev, file.index);
>>> - if (!dev)
>>> + if (!dev) {
>>> + if (file.fd != VIRTIO_INVALID_EVENTFD)
>>> + close(file.fd);
>>> +
>>> return RTE_VHOST_MSG_RESULT_ERR;
>>> + }
>>>
>>> *pdev = dev;
>>>
>>> --
>>> 2.26.2
>
