diff mbox series

[v3,3/3] vhost: fix fd leak in kick setup

Message ID 20201109121630.251603-4-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: fix fd an memory leaks |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Maxime Coquelin Nov. 9, 2020, 12:16 p.m. UTC 
  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(-)

Comments

Xueming Li Nov. 11, 2020, 6:01 a.m. UTC | #1 
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?

>-----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
Chenbo Xia Nov. 11, 2020, 7:57 a.m. UTC | #2 
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?

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
Maxime Coquelin Nov. 12, 2020, 5:06 p.m. UTC | #3 
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
>
diff mbox series

Patch

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;