diff mbox series

[1/3] bus/vmbus: fix leak on device scan

Message ID 20210917082313.21934-2-david.marchand@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series Experiment ASAN in GHA |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

David Marchand Sept. 17, 2021, 8:23 a.m. UTC 
  Caught running ASAN.

The device name is leaked on scan.
rte_device name field being a const, use the private vmbus struct to
store the device name and point at it.

Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/bus/vmbus/linux/vmbus_bus.c | 5 ++++-
 drivers/bus/vmbus/rte_bus_vmbus.h   | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Long Li Sept. 29, 2021, 8:57 p.m. UTC | #1 
> Subject: [PATCH 1/3] bus/vmbus: fix leak on device scan
> 
> Caught running ASAN.
> 
> The device name is leaked on scan.
> rte_device name field being a const, use the private vmbus struct to store the
> device name and point at it.
> 
> Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/bus/vmbus/linux/vmbus_bus.c | 5 ++++-
>  drivers/bus/vmbus/rte_bus_vmbus.h   | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c
> b/drivers/bus/vmbus/linux/vmbus_bus.c
> index 3c924eee14..d8eb07d398 100644
> --- a/drivers/bus/vmbus/linux/vmbus_bus.c
> +++ b/drivers/bus/vmbus/linux/vmbus_bus.c
> @@ -242,7 +242,7 @@ vmbus_scan_one(const char *name)
>  		return -1;
> 
>  	dev->device.bus = &rte_vmbus_bus.bus;
> -	dev->device.name = strdup(name);
> +	dev->device.name = dev->name = strdup(name);


I suggest not to add a "name" in struct rte_vmbus_device. How about defining a local variable in this function, like:
dev->device.name = dev_name = strdup(name);

If failed, just free(dev_name).

>  	if (!dev->device.name)
>  		goto error;
> 
> @@ -261,6 +261,7 @@ vmbus_scan_one(const char *name)
> 
>  	/* skip non-network devices */
>  	if (rte_uuid_compare(dev->class_id, vmbus_nic_uuid) != 0) {
> +		free(dev->name);
>  		free(dev);
>  		return 0;
>  	}
> @@ -312,6 +313,7 @@ vmbus_scan_one(const char *name)
>  		} else { /* already registered */
>  			VMBUS_LOG(NOTICE,
>  				"%s already registered", name);
> +			free(dev->name);
>  			free(dev);
>  		}
>  		return 0;
> @@ -322,6 +324,7 @@ vmbus_scan_one(const char *name)
>  error:
>  	VMBUS_LOG(DEBUG, "failed");
> 
> +	free(dev->name);
>  	free(dev);
>  	return -1;
>  }
> diff --git a/drivers/bus/vmbus/rte_bus_vmbus.h
> b/drivers/bus/vmbus/rte_bus_vmbus.h
> index 4cf73ce815..438eb481fc 100644
> --- a/drivers/bus/vmbus/rte_bus_vmbus.h
> +++ b/drivers/bus/vmbus/rte_bus_vmbus.h
> @@ -65,6 +65,7 @@ struct rte_vmbus_device {
>  	TAILQ_ENTRY(rte_vmbus_device) next;    /**< Next probed VMBUS
> device */
>  	const struct rte_vmbus_driver *driver; /**< Associated driver */
>  	struct rte_device device;              /**< Inherit core device */
> +	char *name;                            /**< Device name */
>  	rte_uuid_t device_id;		       /**< VMBUS device id */
>  	rte_uuid_t class_id;		       /**< VMBUS device type */
>  	uint32_t relid;			       /**< id for primary */
> --
> 2.23.0
David Marchand Sept. 30, 2021, 7:50 a.m. UTC | #2 
On Wed, Sep 29, 2021 at 10:57 PM Long Li <longli@microsoft.com> wrote:
>
> > Subject: [PATCH 1/3] bus/vmbus: fix leak on device scan
> >
> > Caught running ASAN.
> >
> > The device name is leaked on scan.
> > rte_device name field being a const, use the private vmbus struct to store the
> > device name and point at it.
> >
> > Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  drivers/bus/vmbus/linux/vmbus_bus.c | 5 ++++-
> >  drivers/bus/vmbus/rte_bus_vmbus.h   | 1 +
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c
> > b/drivers/bus/vmbus/linux/vmbus_bus.c
> > index 3c924eee14..d8eb07d398 100644
> > --- a/drivers/bus/vmbus/linux/vmbus_bus.c
> > +++ b/drivers/bus/vmbus/linux/vmbus_bus.c
> > @@ -242,7 +242,7 @@ vmbus_scan_one(const char *name)
> >               return -1;
> >
> >       dev->device.bus = &rte_vmbus_bus.bus;
> > -     dev->device.name = strdup(name);
> > +     dev->device.name = dev->name = strdup(name);
>
>
> I suggest not to add a "name" in struct rte_vmbus_device. How about defining a local variable in this function, like:
> dev->device.name = dev_name = strdup(name);

What is your concern for storing the name?


rte_device name only points at some location where the name is stored.
In general this storage is in the bus object or (in some buses) the
devarg that resulted in the rte_device object creation.

If we won't store the name in the bus object, then we lose the ability
to release the name later.
This is probably fine as long as we never release rte_vmbus_device
objects which is the case atm.
But I don't understand why vmbus should be an exception when comparing
to other buses.
Long Li Sept. 30, 2021, 7:14 p.m. UTC | #3 
> Subject: Re: [PATCH 1/3] bus/vmbus: fix leak on device scan
> 
> On Wed, Sep 29, 2021 at 10:57 PM Long Li <longli@microsoft.com> wrote:
> >
> > > Subject: [PATCH 1/3] bus/vmbus: fix leak on device scan
> > >
> > > Caught running ASAN.
> > >
> > > The device name is leaked on scan.
> > > rte_device name field being a const, use the private vmbus struct to
> > > store the device name and point at it.
> > >
> > > Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > >  drivers/bus/vmbus/linux/vmbus_bus.c | 5 ++++-
> > >  drivers/bus/vmbus/rte_bus_vmbus.h   | 1 +
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c
> > > b/drivers/bus/vmbus/linux/vmbus_bus.c
> > > index 3c924eee14..d8eb07d398 100644
> > > --- a/drivers/bus/vmbus/linux/vmbus_bus.c
> > > +++ b/drivers/bus/vmbus/linux/vmbus_bus.c
> > > @@ -242,7 +242,7 @@ vmbus_scan_one(const char *name)
> > >               return -1;
> > >
> > >       dev->device.bus = &rte_vmbus_bus.bus;
> > > -     dev->device.name = strdup(name);
> > > +     dev->device.name = dev->name = strdup(name);
> >
> >
> > I suggest not to add a "name" in struct rte_vmbus_device. How about
> defining a local variable in this function, like:
> > dev->device.name = dev_name = strdup(name);
> 
> What is your concern for storing the name?

This name is only used in this function. I see little value of putting it in struct rte_vmbus_device. Am I missing another patch that uses it from struct rte_vmbus_device from another place?

> 
> 
> rte_device name only points at some location where the name is stored.
> In general this storage is in the bus object or (in some buses) the devarg that
> resulted in the rte_device object creation.
> 
> If we won't store the name in the bus object, then we lose the ability to
> release the name later.
> This is probably fine as long as we never release rte_vmbus_device objects
> which is the case atm.
> But I don't understand why vmbus should be an exception when comparing
> to other buses.

I don’t understand why you want to put a name there if it's never used outside vmbus_scan_one().

Long
David Marchand Oct. 1, 2021, 6:47 a.m. UTC | #4 
On Thu, Sep 30, 2021 at 9:14 PM Long Li <longli@microsoft.com> wrote:
> > rte_device name only points at some location where the name is stored.
> > In general this storage is in the bus object or (in some buses) the devarg that
> > resulted in the rte_device object creation.
> >
> > If we won't store the name in the bus object, then we lose the ability to
> > release the name later.
> > This is probably fine as long as we never release rte_vmbus_device objects
> > which is the case atm.
> > But I don't understand why vmbus should be an exception when comparing
> > to other buses.
>
> I don’t understand why you want to put a name there if it's never used outside vmbus_scan_one().

Since you don't care about releasing rte_vmbus_device objects, I'll
rework as you suggest.
Long Li Oct. 1, 2021, 4:28 p.m. UTC | #5 
> Subject: Re: [PATCH 1/3] bus/vmbus: fix leak on device scan
> 
> On Thu, Sep 30, 2021 at 9:14 PM Long Li <longli@microsoft.com> wrote:
> > > rte_device name only points at some location where the name is stored.
> > > In general this storage is in the bus object or (in some buses) the
> > > devarg that resulted in the rte_device object creation.
> > >
> > > If we won't store the name in the bus object, then we lose the
> > > ability to release the name later.
> > > This is probably fine as long as we never release rte_vmbus_device
> > > objects which is the case atm.
> > > But I don't understand why vmbus should be an exception when
> > > comparing to other buses.
> >
> > I don’t understand why you want to put a name there if it's never used outside
> vmbus_scan_one().
> 
> Since you don't care about releasing rte_vmbus_device objects, I'll rework as
> you suggest.

I don't have any objection defining new data fields in the device struct, as long as it's used during the life cycle of the device.

The rest of the patch looks good, thank you.

Long
diff mbox series

Patch

diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c b/drivers/bus/vmbus/linux/vmbus_bus.c
index 3c924eee14..d8eb07d398 100644
--- a/drivers/bus/vmbus/linux/vmbus_bus.c
+++ b/drivers/bus/vmbus/linux/vmbus_bus.c
@@ -242,7 +242,7 @@  vmbus_scan_one(const char *name)
 		return -1;
 
 	dev->device.bus = &rte_vmbus_bus.bus;
-	dev->device.name = strdup(name);
+	dev->device.name = dev->name = strdup(name);
 	if (!dev->device.name)
 		goto error;
 
@@ -261,6 +261,7 @@  vmbus_scan_one(const char *name)
 
 	/* skip non-network devices */
 	if (rte_uuid_compare(dev->class_id, vmbus_nic_uuid) != 0) {
+		free(dev->name);
 		free(dev);
 		return 0;
 	}
@@ -312,6 +313,7 @@  vmbus_scan_one(const char *name)
 		} else { /* already registered */
 			VMBUS_LOG(NOTICE,
 				"%s already registered", name);
+			free(dev->name);
 			free(dev);
 		}
 		return 0;
@@ -322,6 +324,7 @@  vmbus_scan_one(const char *name)
 error:
 	VMBUS_LOG(DEBUG, "failed");
 
+	free(dev->name);
 	free(dev);
 	return -1;
 }
diff --git a/drivers/bus/vmbus/rte_bus_vmbus.h b/drivers/bus/vmbus/rte_bus_vmbus.h
index 4cf73ce815..438eb481fc 100644
--- a/drivers/bus/vmbus/rte_bus_vmbus.h
+++ b/drivers/bus/vmbus/rte_bus_vmbus.h
@@ -65,6 +65,7 @@  struct rte_vmbus_device {
 	TAILQ_ENTRY(rte_vmbus_device) next;    /**< Next probed VMBUS device */
 	const struct rte_vmbus_driver *driver; /**< Associated driver */
 	struct rte_device device;              /**< Inherit core device */
+	char *name;                            /**< Device name */
 	rte_uuid_t device_id;		       /**< VMBUS device id */
 	rte_uuid_t class_id;		       /**< VMBUS device type */
 	uint32_t relid;			       /**< id for primary */