Re: [dpdk-stable] [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup

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

On 11/11/20 8:57 AM, Xia, Chenbo wrote:
> Hi Xueming & Maxime,
> 
>> -----Original Message-----
>> From: Xueming(Steven) Li <xuemingl@nvidia.com>
>> Sent: Wednesday, November 11, 2020 2:02 PM
>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org; Ding, Xuan
>> <xuan.ding@intel.com>; stephen@networkplumber.org; NBU-Contact-Thomas
>> Monjalon <thomas@monjalon.net>; stable@dpdk.org; Xia, Chenbo
>> <chenbo.xia@intel.com>
>> 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 <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
>>> Sent: Monday, November 9, 2020 8:17 PM
>>> To: dev@dpdk.org; xuan.ding@intel.com; stephen@networkplumber.org;
>>> NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; stable@dpdk.org;
>>> chenbo.xia@intel.com
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> 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: stable@dpdk.org
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
>>> ---
>>> 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
>