diff mbox series

net/vdev_netvsc: fix erronous cast

Message ID 20190313021253.525-1-stephen@networkplumber.org (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series net/vdev_netvsc: fix erronous cast |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/Intel-compilation fail Compilation issues

Commit Message

Stephen Hemminger March 13, 2019, 2:12 a.m. UTC 
  The return value from bus->find_device is a rte_device
which is not safe to cast to a rte_vdev_device structure.
It doesn't really matter since only being checked for NULL
but static checkers might find a bug here.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/vdev_netvsc/vdev_netvsc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Matan Azrad March 14, 2019, 11:13 a.m. UTC | #1 
Hi

From: Stephen Hemminger
> The return value from bus->find_device is a rte_device which is not safe to
> cast to a rte_vdev_device structure.
> It doesn't really matter since only being checked for NULL but static checkers
> might find a bug here.
> 

Fix line is missing.

> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/vdev_netvsc/vdev_netvsc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c
> b/drivers/net/vdev_netvsc/vdev_netvsc.c
> index ba63fac2a598..801f54c96e01 100644
> --- a/drivers/net/vdev_netvsc/vdev_netvsc.c
> +++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
> @@ -808,7 +808,7 @@ vdev_netvsc_cmp_rte_device(const struct
> rte_device *dev1,  static void  vdev_netvsc_scan_callback(__rte_unused
> void *arg)  {
> -	struct rte_vdev_device *dev;
> +	struct rte_device *dev;
>  	struct rte_devargs *devargs;
>  	struct rte_bus *vbus = rte_bus_find_by_name("vdev");
> 
> @@ -816,8 +816,9 @@ vdev_netvsc_scan_callback(__rte_unused void *arg)
>  		if (!strncmp(devargs->name, VDEV_NETVSC_DRIVER_NAME,
>  			     VDEV_NETVSC_DRIVER_NAME_LEN))
>  			return;
> -	dev = (struct rte_vdev_device *)vbus->find_device(NULL,
> -		vdev_netvsc_cmp_rte_device,
> VDEV_NETVSC_DRIVER_NAME);
> +
> +	dev = vbus->find_device(NULL, vdev_netvsc_cmp_rte_device,
> +				VDEV_NETVSC_DRIVER_NAME);

Since the device must be vdev here,
It is better to use explicit cast to make the checker happy.

>  	if (dev)
>  		return;
>  	if (rte_devargs_add(RTE_DEVTYPE_VIRTUAL,
> VDEV_NETVSC_DRIVER_NAME))
> --
> 2.17.1
Stephen Hemminger March 14, 2019, 3:44 p.m. UTC | #2 
On Thu, 14 Mar 2019 11:13:10 +0000
Matan Azrad <matan@mellanox.com> wrote:

> Hi
> 
> From: Stephen Hemminger
> > The return value from bus->find_device is a rte_device which is not safe to
> > cast to a rte_vdev_device structure.
> > It doesn't really matter since only being checked for NULL but static checkers
> > might find a bug here.
> >   
> 
> Fix line is missing.
> 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  drivers/net/vdev_netvsc/vdev_netvsc.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c
> > b/drivers/net/vdev_netvsc/vdev_netvsc.c
> > index ba63fac2a598..801f54c96e01 100644
> > --- a/drivers/net/vdev_netvsc/vdev_netvsc.c
> > +++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
> > @@ -808,7 +808,7 @@ vdev_netvsc_cmp_rte_device(const struct
> > rte_device *dev1,  static void  vdev_netvsc_scan_callback(__rte_unused
> > void *arg)  {
> > -	struct rte_vdev_device *dev;
> > +	struct rte_device *dev;
> >  	struct rte_devargs *devargs;
> >  	struct rte_bus *vbus = rte_bus_find_by_name("vdev");
> > 
> > @@ -816,8 +816,9 @@ vdev_netvsc_scan_callback(__rte_unused void *arg)
> >  		if (!strncmp(devargs->name, VDEV_NETVSC_DRIVER_NAME,
> >  			     VDEV_NETVSC_DRIVER_NAME_LEN))
> >  			return;
> > -	dev = (struct rte_vdev_device *)vbus->find_device(NULL,
> > -		vdev_netvsc_cmp_rte_device,
> > VDEV_NETVSC_DRIVER_NAME);
> > +
> > +	dev = vbus->find_device(NULL, vdev_netvsc_cmp_rte_device,
> > +				VDEV_NETVSC_DRIVER_NAME);  
> 
> Since the device must be vdev here,
> It is better to use explicit cast to make the checker happy.

The cast is converting from rte_device to rte_vdev_device which incorrect.

The device returned by vbus->find_device is a rte_device not a rte_vdev_device:

vbus->find_device points to rte_vdev_find_device

struct rte_device *
rte_vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
		     const void *data)

but definition of rte_vdev_device is:

struct rte_vdev_device {
	TAILQ_ENTRY(rte_vdev_device) next;      /**< Next attached vdev */
	struct rte_device device;               /**< Inherit core device */
};

The only safe way to get rte_vdev_device would be to use container_of().

	dev = vbus->find_device(NULL, vdev_netvsc_cmp_rte_device,
				VDEV_NETVSC_DRIVER_NAME);
	vdev = dev ? container_of(dev, struct rte_vdev_device, device) : NULL;

But as the PATCH demonstrated, this is not necessary.
Matan Azrad March 17, 2019, 6:16 a.m. UTC | #3 
Hi

From: Stephen Hemminger
> > Hi
> >
> > From: Stephen Hemminger
> > > The return value from bus->find_device is a rte_device which is not
> > > safe to cast to a rte_vdev_device structure.
> > > It doesn't really matter since only being checked for NULL but
> > > static checkers might find a bug here.
> > >
> >
> > Fix line is missing.
> >
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> > >  drivers/net/vdev_netvsc/vdev_netvsc.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c
> > > b/drivers/net/vdev_netvsc/vdev_netvsc.c
> > > index ba63fac2a598..801f54c96e01 100644
> > > --- a/drivers/net/vdev_netvsc/vdev_netvsc.c
> > > +++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
> > > @@ -808,7 +808,7 @@ vdev_netvsc_cmp_rte_device(const struct
> > > rte_device *dev1,  static void
> > > vdev_netvsc_scan_callback(__rte_unused
> > > void *arg)  {
> > > -	struct rte_vdev_device *dev;
> > > +	struct rte_device *dev;
> > >  	struct rte_devargs *devargs;
> > >  	struct rte_bus *vbus = rte_bus_find_by_name("vdev");
> > >
> > > @@ -816,8 +816,9 @@ vdev_netvsc_scan_callback(__rte_unused void
> *arg)
> > >  		if (!strncmp(devargs->name, VDEV_NETVSC_DRIVER_NAME,
> > >  			     VDEV_NETVSC_DRIVER_NAME_LEN))
> > >  			return;
> > > -	dev = (struct rte_vdev_device *)vbus->find_device(NULL,
> > > -		vdev_netvsc_cmp_rte_device,
> > > VDEV_NETVSC_DRIVER_NAME);
> > > +
> > > +	dev = vbus->find_device(NULL, vdev_netvsc_cmp_rte_device,
> > > +				VDEV_NETVSC_DRIVER_NAME);
> >
> > Since the device must be vdev here,
> > It is better to use explicit cast to make the checker happy.
> 
> The cast is converting from rte_device to rte_vdev_device which incorrect.
> 
> The device returned by vbus->find_device is a rte_device not a
> rte_vdev_device:
> 
> vbus->find_device points to rte_vdev_find_device
> 
> struct rte_device *
> rte_vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
> 		     const void *data)
> 
> but definition of rte_vdev_device is:
> 
> struct rte_vdev_device {
> 	TAILQ_ENTRY(rte_vdev_device) next;      /**< Next attached vdev */
> 	struct rte_device device;               /**< Inherit core device */
> };
> 
> The only safe way to get rte_vdev_device would be to use container_of().
> 
> 	dev = vbus->find_device(NULL, vdev_netvsc_cmp_rte_device,
> 				VDEV_NETVSC_DRIVER_NAME);
> 	vdev = dev ? container_of(dev, struct rte_vdev_device, device) :
> NULL;
> 
> But as the PATCH demonstrated, this is not necessary.

Yes, you right.
Please add fix line to the patch + stable line.


Matan
Ferruh Yigit March 25, 2019, 1:51 p.m. UTC | #4 
On 3/17/2019 6:16 AM, Matan Azrad wrote:
> Hi
> 
> From: Stephen Hemminger
>>> Hi
>>>
>>> From: Stephen Hemminger
>>>> The return value from bus->find_device is a rte_device which is not
>>>> safe to cast to a rte_vdev_device structure.
>>>> It doesn't really matter since only being checked for NULL but
>>>> static checkers might find a bug here.
>>>>
>>>
>>> Fix line is missing.
>>>
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>> ---
>>>>  drivers/net/vdev_netvsc/vdev_netvsc.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c
>>>> b/drivers/net/vdev_netvsc/vdev_netvsc.c
>>>> index ba63fac2a598..801f54c96e01 100644
>>>> --- a/drivers/net/vdev_netvsc/vdev_netvsc.c
>>>> +++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
>>>> @@ -808,7 +808,7 @@ vdev_netvsc_cmp_rte_device(const struct
>>>> rte_device *dev1,  static void
>>>> vdev_netvsc_scan_callback(__rte_unused
>>>> void *arg)  {
>>>> -	struct rte_vdev_device *dev;
>>>> +	struct rte_device *dev;
>>>>  	struct rte_devargs *devargs;
>>>>  	struct rte_bus *vbus = rte_bus_find_by_name("vdev");
>>>>
>>>> @@ -816,8 +816,9 @@ vdev_netvsc_scan_callback(__rte_unused void
>> *arg)
>>>>  		if (!strncmp(devargs->name, VDEV_NETVSC_DRIVER_NAME,
>>>>  			     VDEV_NETVSC_DRIVER_NAME_LEN))
>>>>  			return;
>>>> -	dev = (struct rte_vdev_device *)vbus->find_device(NULL,
>>>> -		vdev_netvsc_cmp_rte_device,
>>>> VDEV_NETVSC_DRIVER_NAME);
>>>> +
>>>> +	dev = vbus->find_device(NULL, vdev_netvsc_cmp_rte_device,
>>>> +				VDEV_NETVSC_DRIVER_NAME);
>>>
>>> Since the device must be vdev here,
>>> It is better to use explicit cast to make the checker happy.
>>
>> The cast is converting from rte_device to rte_vdev_device which incorrect.
>>
>> The device returned by vbus->find_device is a rte_device not a
>> rte_vdev_device:
>>
>> vbus->find_device points to rte_vdev_find_device
>>
>> struct rte_device *
>> rte_vdev_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
>> 		     const void *data)
>>
>> but definition of rte_vdev_device is:
>>
>> struct rte_vdev_device {
>> 	TAILQ_ENTRY(rte_vdev_device) next;      /**< Next attached vdev */
>> 	struct rte_device device;               /**< Inherit core device */
>> };
>>
>> The only safe way to get rte_vdev_device would be to use container_of().
>>
>> 	dev = vbus->find_device(NULL, vdev_netvsc_cmp_rte_device,
>> 				VDEV_NETVSC_DRIVER_NAME);
>> 	vdev = dev ? container_of(dev, struct rte_vdev_device, device) :
>> NULL;
>>
>> But as the PATCH demonstrated, this is not necessary.
> 
> Yes, you right.
> Please add fix line to the patch + stable line.

Adding explicit cast from Matan:
Acked-by: Matan Azrad <matan@mellanox.com>

    Fixes: 56252de779a6 ("net/vdev_netvsc: add automatic probing")
    Cc: stable@dpdk.org

Applied to dpdk-next-net/master, thanks.
diff mbox series

Patch

diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c
index ba63fac2a598..801f54c96e01 100644
--- a/drivers/net/vdev_netvsc/vdev_netvsc.c
+++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
@@ -808,7 +808,7 @@  vdev_netvsc_cmp_rte_device(const struct rte_device *dev1,
 static void
 vdev_netvsc_scan_callback(__rte_unused void *arg)
 {
-	struct rte_vdev_device *dev;
+	struct rte_device *dev;
 	struct rte_devargs *devargs;
 	struct rte_bus *vbus = rte_bus_find_by_name("vdev");
 
@@ -816,8 +816,9 @@  vdev_netvsc_scan_callback(__rte_unused void *arg)
 		if (!strncmp(devargs->name, VDEV_NETVSC_DRIVER_NAME,
 			     VDEV_NETVSC_DRIVER_NAME_LEN))
 			return;
-	dev = (struct rte_vdev_device *)vbus->find_device(NULL,
-		vdev_netvsc_cmp_rte_device, VDEV_NETVSC_DRIVER_NAME);
+
+	dev = vbus->find_device(NULL, vdev_netvsc_cmp_rte_device,
+				VDEV_NETVSC_DRIVER_NAME);
 	if (dev)
 		return;
 	if (rte_devargs_add(RTE_DEVTYPE_VIRTUAL, VDEV_NETVSC_DRIVER_NAME))