[PATCH 1/3] bus/vdev: revert fix devargs in secondary process

[PATCH 1/3] bus/vdev: revert fix devargs in secondary process

[Mingjin Ye]

The asan tool detected a memory leak in the vdev driver
alloc_devargs. The previous commit was that when inserting
a vdev device, the primary process alloc devargs and the
secondary process looks for devargs. This causes the
device to not be created if the secondary process does
not initialise the vdev device. And, this is not the
root cause.

Therefore the following commit was reverted accordingly.

Fixes: 6666628362c9 ("bus/vdev: fix devargs in secondary process")

After restoring this commit, the memory leak still exists.
A new patch has since fixed this issue.

Fixes: 6666628362c9 ("bus/vdev: fix devargs in secondary process")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 drivers/bus/vdev/vdev.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 14cf856237..38d05a9fe9 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -263,22 +263,6 @@ alloc_devargs(const char *name, const char *args)
 	return devargs;
 }
 
-static struct rte_devargs *
-vdev_devargs_lookup(const char *name)
-{
-	struct rte_devargs *devargs;
-	char dev_name[32];
-
-	RTE_EAL_DEVARGS_FOREACH("vdev", devargs) {
-		devargs->bus->parse(devargs->name, &dev_name);
-		if (strcmp(dev_name, name) == 0) {
-			VDEV_LOG(INFO, "devargs matched %s", dev_name);
-			return devargs;
-		}
-	}
-	return NULL;
-}
-
 static int
 insert_vdev(const char *name, const char *args,
 		struct rte_vdev_device **p_dev,
@@ -291,10 +275,7 @@ insert_vdev(const char *name, const char *args,
 	if (name == NULL)
 		return -EINVAL;
 
-	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
-		devargs = alloc_devargs(name, args);
-	else
-		devargs = vdev_devargs_lookup(name);
+	devargs = alloc_devargs(name, args);
 
 	if (!devargs)
 		return -ENOMEM;
-- 
2.25.1

Re: [PATCH 1/3] bus/vdev: revert fix devargs in secondary process

[Thomas Monjalon]

14/03/2024 10:36, Mingjin Ye:
> The asan tool detected a memory leak in the vdev driver
> alloc_devargs. The previous commit was that when inserting
> a vdev device, the primary process alloc devargs and the
> secondary process looks for devargs. This causes the
> device to not be created if the secondary process does
> not initialise the vdev device. And, this is not the
> root cause.
> 
> Therefore the following commit was reverted accordingly.
> 
> Fixes: 6666628362c9 ("bus/vdev: fix devargs in secondary process")
> 
> After restoring this commit, the memory leak still exists.
> A new patch has since fixed this issue.
> 
> Fixes: 6666628362c9 ("bus/vdev: fix devargs in secondary process")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

Applied

The rest of the series is not cleared enough to be applied.
We need more precise explanation of the problem to solve,
and some good reviews please.
At this stage I'm confident enough only for this revert.

[PATCH 2/3] bus/vdev: revert fix devargs after multi-process bus scan

[Mingjin Ye]

The asan tool detected a memory leak in the vdev driver alloc_devargs.
The previous commit does not insert device arguments into devargs_list
when attaching a device during a bus scan of a secondary process.
This resulted in an existing memory leak when removing a vdev device,
since rte_devargs_remove actually does nothing.

Therefore the following commit was reverted accordingly.

Fixes: f5b2eff0847d ("bus/vdev: fix devargs after multi-process bus scan")

Restoring this commit will fix the memory leak. There was an issue with
device parameters using free devargs when inserting a vdev device when
devargs_list already existed, resulting in a core dump. A new patch
will fix this issue.

Fixes: f5b2eff0847d ("bus/vdev: fix devargs after multi-process bus scan")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 drivers/bus/vdev/vdev.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 38d05a9fe9..1a6cc7d12d 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -264,9 +264,7 @@ alloc_devargs(const char *name, const char *args)
 }
 
 static int
-insert_vdev(const char *name, const char *args,
-		struct rte_vdev_device **p_dev,
-		bool init)
+insert_vdev(const char *name, const char *args, struct rte_vdev_device **p_dev)
 {
 	struct rte_vdev_device *dev;
 	struct rte_devargs *devargs;
@@ -300,8 +298,7 @@ insert_vdev(const char *name, const char *args,
 		goto fail;
 	}
 
-	if (init)
-		rte_devargs_insert(&devargs);
+	rte_devargs_insert(&devargs);
 	dev->device.devargs = devargs;
 	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
 
@@ -323,7 +320,7 @@ rte_vdev_init(const char *name, const char *args)
 	int ret;
 
 	rte_spinlock_recursive_lock(&vdev_device_list_lock);
-	ret = insert_vdev(name, args, &dev, true);
+	ret = insert_vdev(name, args, &dev);
 	if (ret == 0) {
 		ret = vdev_probe_all_drivers(dev);
 		if (ret) {
@@ -449,7 +446,7 @@ vdev_action(const struct rte_mp_msg *mp_msg, const void *peer)
 		break;
 	case VDEV_SCAN_ONE:
 		VDEV_LOG(INFO, "receive vdev, %s", in->name);
-		ret = insert_vdev(in->name, NULL, NULL, false);
+		ret = insert_vdev(in->name, NULL, NULL);
 		if (ret == -EEXIST)
 			VDEV_LOG(DEBUG, "device already exist, %s", in->name);
 		else if (ret < 0)
-- 
2.25.1

Re: [PATCH 2/3] bus/vdev: revert fix devargs after multi-process bus scan

[Thomas Monjalon]

14/03/2024 10:36, Mingjin Ye:
> The asan tool detected a memory leak in the vdev driver alloc_devargs.
> The previous commit does not insert device arguments into devargs_list

What is the previous commit?
Where is devargs_list in this function?

> when attaching a device during a bus scan of a secondary process.
> This resulted in an existing memory leak when removing a vdev device,
> since rte_devargs_remove actually does nothing.
> 
> Therefore the following commit was reverted accordingly.
> 
> Fixes: f5b2eff0847d ("bus/vdev: fix devargs after multi-process bus scan")
> 
> Restoring this commit will fix the memory leak. There was an issue with
> device parameters using free devargs when inserting a vdev device when
> devargs_list already existed, resulting in a core dump. A new patch
> will fix this issue.
> 
> Fixes: f5b2eff0847d ("bus/vdev: fix devargs after multi-process bus scan")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

I'm not comfortable with reverting a so old commit.
Your previous attempt in this bus driver was not successful.
Please prove the memory leak cannot be simply fixed.

RE: [PATCH 2/3] bus/vdev: revert fix devargs after multi-process bus scan

[Ye, MingjinX]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, June 20, 2024 4:15 AM
> To: Ye, MingjinX <mingjinx.ye@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; stephen@networkplumber.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Marchand, David
> <david.marchand@redhat.com>
> Subject: Re: [PATCH 2/3] bus/vdev: revert fix devargs after multi-process bus
> scan
> 
> 14/03/2024 10:36, Mingjin Ye:
> > The asan tool detected a memory leak in the vdev driver alloc_devargs.
> > The previous commit does not insert device arguments into devargs_list
> 
> What is the previous commit?
commit: f5b2eff0847d49a66301f0046502c6232cd5da3f


> Where is devargs_list in this function?
In the rte_devargs_insert function.
> 
> > when attaching a device during a bus scan of a secondary process.
> > This resulted in an existing memory leak when removing a vdev device,
> > since rte_devargs_remove actually does nothing.
> >
> > Therefore the following commit was reverted accordingly.
> >
> > Fixes: f5b2eff0847d ("bus/vdev: fix devargs after multi-process bus
> > scan")
> >
> > Restoring this commit will fix the memory leak. There was an issue
> > with device parameters using free devargs when inserting a vdev device
> > when devargs_list already existed, resulting in a core dump. A new
> > patch will fix this issue.
> >
> > Fixes: f5b2eff0847d ("bus/vdev: fix devargs after multi-process bus
> > scan")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> 
> I'm not comfortable with reverting a so old commit.
> Your previous attempt in this bus driver was not successful.
> Please prove the memory leak cannot be simply fixed.

dpdk/drivers/bus/vdev/vdev.c:471
ret = insert_vdev(in->name, NULL, NULL, false);

Since the last argument is always fale, this results in the objects of alloc_devargs in insert_vdev not being added to the devargs_list via rte_devargs_insert.

rte_vdev_uninit->rte_devargs_remove will release the devargs objects constructed through the program startup parameters. 
however the devargs bound to the device (the devargs objects constructed in insert_vdev) are not released. causing a memory leak.

Therefore, after undoing the patch. insert_vdev->rte_devargs_insert can update the devargs object without causing a memory leak.
> 

[PATCH 3/3] net/vdev: fix insert vdev core dump

[Mingjin Ye]

Inserting a vdev device when the device arguments are already stored
in devargs_list, the rte_devargs_insert function replaces the supplied
new devargs with the found devargs and frees the new devargs. As a
result, the use of free devargs results in a core dump.

This patch fixes the issue by using valid devargs.

Fixes: f3a1188cee4a ("devargs: make device representation generic")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 drivers/bus/vdev/vdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 1a6cc7d12d..53fc4a6e21 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -286,7 +286,6 @@ insert_vdev(const char *name, const char *args, struct rte_vdev_device **p_dev)
 
 	dev->device.bus = &rte_vdev_bus;
 	dev->device.numa_node = SOCKET_ID_ANY;
-	dev->device.name = devargs->name;
 
 	if (find_vdev(name)) {
 		/*
@@ -300,6 +299,7 @@ insert_vdev(const char *name, const char *args, struct rte_vdev_device **p_dev)
 
 	rte_devargs_insert(&devargs);
 	dev->device.devargs = devargs;
+	dev->device.name = devargs->name;
 	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
 
 	if (p_dev)
-- 
2.25.1

RE: [PATCH 3/3] net/vdev: fix insert vdev core dump

[Jiang, YuX]

> -----Original Message-----
> From: Mingjin Ye <mingjinx.ye@intel.com>
> Sent: Thursday, March 14, 2024 5:37 PM
> To: dev@dpdk.org
> Cc: Ye, MingjinX <mingjinx.ye@intel.com>; stable@dpdk.org
> Subject: [PATCH 3/3] net/vdev: fix insert vdev core dump
>
> Inserting a vdev device when the device arguments are already stored in
> devargs_list, the rte_devargs_insert function replaces the supplied new
> devargs with the found devargs and frees the new devargs. As a result, the use
> of free devargs results in a core dump.
>
> This patch fixes the issue by using valid devargs.
>
> Fixes: f3a1188cee4a ("devargs: make device representation generic")
> Cc: stable@dpdk.org
>
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
>  drivers/bus/vdev/vdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Tested-by:  Yu Jiang <yux.jiang@intel.com>

Best regards,
Yu Jiang

Re: [PATCH 3/3] net/vdev: fix insert vdev core dump

[Thomas Monjalon]

14/03/2024 10:36, Mingjin Ye:
> Inserting a vdev device when the device arguments are already stored
> in devargs_list, the rte_devargs_insert function replaces the supplied
> new devargs with the found devargs and frees the new devargs. As a
> result, the use of free devargs results in a core dump.
> 
> This patch fixes the issue by using valid devargs.
> 
> Fixes: f3a1188cee4a ("devargs: make device representation generic")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
>  	dev->device.bus = &rte_vdev_bus;
>  	dev->device.numa_node = SOCKET_ID_ANY;
> -	dev->device.name = devargs->name;
>  
>  	if (find_vdev(name)) {
>  		/*
> @@ -300,6 +299,7 @@ insert_vdev(const char *name, const char *args, struct rte_vdev_device **p_dev)
>  
>  	rte_devargs_insert(&devargs);
>  	dev->device.devargs = devargs;
> +	dev->device.name = devargs->name;
>  	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);

How setting the name later can have an impact here?

RE: [PATCH 3/3] net/vdev: fix insert vdev core dump

[Ye, MingjinX]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, June 20, 2024 4:16 AM
> To: Ye, MingjinX <mingjinx.ye@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Marchand, David
> <david.marchand@redhat.com>; stephen@networkplumber.org;
> Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [PATCH 3/3] net/vdev: fix insert vdev core dump
> 
> 14/03/2024 10:36, Mingjin Ye:
> > Inserting a vdev device when the device arguments are already stored
> > in devargs_list, the rte_devargs_insert function replaces the supplied
> > new devargs with the found devargs and frees the new devargs. As a
> > result, the use of free devargs results in a core dump.
> >
> > This patch fixes the issue by using valid devargs.
> >
> > Fixes: f3a1188cee4a ("devargs: make device representation generic")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > ---
> >  	dev->device.bus = &rte_vdev_bus;
> >  	dev->device.numa_node = SOCKET_ID_ANY;
> > -	dev->device.name = devargs->name;
> >
> >  	if (find_vdev(name)) {
> >  		/*
> > @@ -300,6 +299,7 @@ insert_vdev(const char *name, const char *args,
> > struct rte_vdev_device **p_dev)
> >
> >  	rte_devargs_insert(&devargs);
> >  	dev->device.devargs = devargs;
> > +	dev->device.name = devargs->name;
> >  	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
> 
> How setting the name later can have an impact here?
> 
/**
 * Insert an rte_devargs in the global list.
 *
 * @param da
 *  The devargs structure to insert.
 *  If a devargs for the same device is already inserted,
 *  it will be updated and returned. It means *da pointer can change.
 *
 * @return
 *   - 0 on success
 *   - Negative on error.
 */
int
rte_devargs_insert(struct rte_devargs **da);

Info: dpdk/lib/eal/include/rte_devargs.h