[PATCH 0/3] fix insert dev core dump

[PATCH 0/3] fix insert dev core dump

[Mingjin Ye]

revert 2 patches and fix insert vdev core dump.

Mingjin Ye (3):
  bus/vdev: revert fix devargs in secondary process
  bus/vdev: revert fix devargs after multi-process bus scan
  net/vdev: fix insert vdev core dump

 drivers/bus/vdev/vdev.c | 34 ++++++----------------------------
 1 file changed, 6 insertions(+), 28 deletions(-)

-- 
2.25.1

[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

[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

[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 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.

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 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 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.
> 

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

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

[Burakov, Anatoly]

On 3/14/2024 10:36 AM, Mingjin Ye wrote:
> 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>
> ---

Hi Mingjin,

I've been trying to understand what this patchset does.

>   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);

The original commit message states that "it is not necessary" to add 
devargs to the list in secondary processes, but does not state why it is 
unnecessary and what issue was that fix for. With that understanding, I 
think the commit message for this patch can be reworded like follows:

---

This patch reverts commit f5b2eff0847d ("bus/vdev: fix devargs after 
multi-process bus scan")

With current code, we do not add devargs to devargs list when we add a 
vdev in secondary process (because `init` flag is set to `false`).

Because of this, when we do vdev_uninit, we call rte_devargs_remove on 
the &devargs pointer (the one we didn't add to devargs list but did save 
inside rte_vdev_device struct), but in secondary process, because 
devargs were not added to the list in the first place, devargs_remove 
does not find the associated devargs in its list and therefore does not 
free associated resources. As a result, we get a memory leak, because we 
free the rte_vdev_device but not its associated devargs.

Revert this patch to avoid leaking devargs on vdev uninit.

---

I think this would be clearer summation of what is going on in this patch.

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>


>   	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)

-- 
Thanks,
Anatoly

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

[Burakov, Anatoly]

On 3/14/2024 10:36 AM, Mingjin Ye wrote:
> 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>

I am not too familiar with how devargs works, so I have a trivial question.

I understand the point of this patch, and it is roughly as follows:

1) we enter insert_vdev and allocated new devargs structure (and copy 
the `name` into devargs)
2) we set dev->device.name to devargs->name
3) we insert devargs into the list using rte_devargs_insert
4) if devargs list already had devargs with the same name, our devargs 
is destroyed and replaced with devargs that is in the list
5) because of this, dev->device.name becomes invalid as that specific 
devargs has been freed - it points to name from the old devargs

We do need to store devargs->name in dev->device.name, and we need to do 
so after calling rte_devargs_insert to avoid keeping reference to memory 
that was freed. So, provisionally,

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

My question is, under what circumstances would there be duplicate 
entries in devargs list? I assume there is an answer to that question 
because rte_devargs_insert() expects this case, so this is just for my 
understanding - I can't seem to figure out how we would arrive at a 
situation where we have duplicate devargs.

> ---
>   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)

-- 
Thanks,
Anatoly

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

[Ye, MingjinX]

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Friday, July 12, 2024 12:10 AM
> To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: Re: [PATCH 3/3] net/vdev: fix insert vdev core dump
> 
> On 3/14/2024 10:36 AM, Mingjin Ye wrote:
> > 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>
> 
> I am not too familiar with how devargs works, so I have a trivial question.
> 
> I understand the point of this patch, and it is roughly as follows:
> 
> 1) we enter insert_vdev and allocated new devargs structure (and copy the
> `name` into devargs)
> 2) we set dev->device.name to devargs->name
> 3) we insert devargs into the list using rte_devargs_insert
> 4) if devargs list already had devargs with the same name, our devargs is
> destroyed and replaced with devargs that is in the list
> 5) because of this, dev->device.name becomes invalid as that specific
> devargs has been freed - it points to name from the old devargs
> 
> We do need to store devargs->name in dev->device.name, and we need to
> do so after calling rte_devargs_insert to avoid keeping reference to memory
> that was freed. So, provisionally,
> 
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> My question is, under what circumstances would there be duplicate entries
> in devargs list?

In a multi-process circumstances, the primary and secondary processes both have "vdev" startup
Parameters. And their parameters have the same name. This causes multiple devargs objects with the same name
to be created in the secondary process, the 1st one constructed from the startup parameter and
the 2nd one constructed due to the VDEV_SCAN_ONE message received.

Path 1:
eal_parse_args->eal_parse_common_option: OPT_VDEV_NUM
eal_option_device_parse->rte_devargs_add

Path 2:
vdev_action: vdev_scan_one ->insert_vdev(alloc_devargs)

> I assume there is an answer to that question because
> rte_devargs_insert() expects this case, so this is just for my understanding - I
> can't seem to figure out how we would arrive at a situation where we have
> duplicate devargs.
> 
> > ---
> >   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)
> 
> --
> Thanks,
> Anatoly

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

[Burakov, Anatoly]

On 7/12/2024 4:18 AM, Ye, MingjinX wrote:
> 
> 
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Sent: Friday, July 12, 2024 12:10 AM
>> To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org
>> Subject: Re: [PATCH 3/3] net/vdev: fix insert vdev core dump
>>
>> On 3/14/2024 10:36 AM, Mingjin Ye wrote:
>>> 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>
>>
>> I am not too familiar with how devargs works, so I have a trivial question.
>>
>> I understand the point of this patch, and it is roughly as follows:
>>
>> 1) we enter insert_vdev and allocated new devargs structure (and copy the
>> `name` into devargs)
>> 2) we set dev->device.name to devargs->name
>> 3) we insert devargs into the list using rte_devargs_insert
>> 4) if devargs list already had devargs with the same name, our devargs is
>> destroyed and replaced with devargs that is in the list
>> 5) because of this, dev->device.name becomes invalid as that specific
>> devargs has been freed - it points to name from the old devargs
>>
>> We do need to store devargs->name in dev->device.name, and we need to
>> do so after calling rte_devargs_insert to avoid keeping reference to memory
>> that was freed. So, provisionally,
>>
>> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>
>> My question is, under what circumstances would there be duplicate entries
>> in devargs list?
> 
> In a multi-process circumstances, the primary and secondary processes both have "vdev" startup
> Parameters. And their parameters have the same name. This causes multiple devargs objects with the same name
> to be created in the secondary process, the 1st one constructed from the startup parameter and
> the 2nd one constructed due to the VDEV_SCAN_ONE message received.
> 
> Path 1:
> eal_parse_args->eal_parse_common_option: OPT_VDEV_NUM
> eal_option_device_parse->rte_devargs_add
> 
> Path 2:
> vdev_action: vdev_scan_one ->insert_vdev(alloc_devargs)

So, technically, this is only a problem because we're specifying --vdev 
at secondary process startup even though we could've just relied on vdev 
multiprocess hotplug to provide us with a device? Or is there more to it?

In any case, I think this patch is OK. I think the Fixes: tag is 
pointing to an incorrect commit. I've walked through the commit history, 
and it seems that this wasn't a bug initially, but became a bug when 
vdev multiprocess hotplug was added, so I think the correct Fixes: tag 
should be as follows:

Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")

So, on account of the above, I suggest rewording the commit message to 
something like the following:

In secondary processes, insert_vdev() may be called multiple times on 
the same device due to a combination of multiprocess hotplug for vdev 
bus, and EAL argument adding the same vdev. In this circumstance, when 
rte_devargs_insert() is called, the devargs->name reference becomes 
invalid because rte_devargs_insert() will destroy the just-allocated 
devargs and will replace the pointer with one from the devargs list. As 
a result, the reference to devargs->name stored in dev->device.name 
becomes invalid. Fix the issue by not setting device name until after 
rte_devargs_insert() was called.

Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
-- 
Thanks,
Anatoly

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

[Mingjin Ye]

In secondary processes, insert_vdev() may be called multiple times on the
same device due to multi-process hot-plugging of the vdev bus and EAL
parameters to add the same vdev.

In this case, when rte_devargs_insert() is called, the devargs->name
reference will be invalidated because rte_devargs_insert() destroys the
just-allocated devargs and replaces the pointer from the devargs list.
As a result, the reference to devargs->name stored in dev->device.name
will be invalid.

This patch fixes the issue by setting the device name after calling
rte_devargs_insert().

Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
v2: Modify commit log.
---
 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 38d05a9fe9..ec7abe7cda 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -288,7 +288,6 @@ insert_vdev(const char *name, const char *args,
 
 	dev->device.bus = &rte_vdev_bus;
 	dev->device.numa_node = SOCKET_ID_ANY;
-	dev->device.name = devargs->name;
 
 	if (find_vdev(name)) {
 		/*
@@ -303,6 +302,7 @@ insert_vdev(const char *name, const char *args,
 	if (init)
 		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 v2] net/vdev: fix insert vdev core dump

[Burakov, Anatoly]

On 7/16/2024 11:53 AM, Mingjin Ye wrote:
> In secondary processes, insert_vdev() may be called multiple times on the
> same device due to multi-process hot-plugging of the vdev bus and EAL
> parameters to add the same vdev.
> 
> In this case, when rte_devargs_insert() is called, the devargs->name
> reference will be invalidated because rte_devargs_insert() destroys the
> just-allocated devargs and replaces the pointer from the devargs list.
> As a result, the reference to devargs->name stored in dev->device.name
> will be invalid.
> 
> This patch fixes the issue by setting the device name after calling
> rte_devargs_insert().
> 
> Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
> v2: Modify commit log.
> ---

Forgot to add my review tag:

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

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

[Thomas Monjalon]

22/07/2024 14:39, Burakov, Anatoly:
> On 7/16/2024 11:53 AM, Mingjin Ye wrote:
> > In secondary processes, insert_vdev() may be called multiple times on the
> > same device due to multi-process hot-plugging of the vdev bus and EAL
> > parameters to add the same vdev.
> > 
> > In this case, when rte_devargs_insert() is called, the devargs->name
> > reference will be invalidated because rte_devargs_insert() destroys the
> > just-allocated devargs and replaces the pointer from the devargs list.
> > As a result, the reference to devargs->name stored in dev->device.name
> > will be invalid.
> > 
> > This patch fixes the issue by setting the device name after calling
> > rte_devargs_insert().
> > 
> > Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > ---
> > v2: Modify commit log.
> > ---
> 
> Forgot to add my review tag:
> 
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks.