diff mbox series

[1/2] baseband/fpga_5gnr_fec: fix segfaults with debug

Message ID 20201006100421.72210-2-maxime.coquelin@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: akhil goyal
Headers
Series baseband: fix segfault in Intel drivers |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Maxime Coquelin Oct. 6, 2020, 10:04 a.m. UTC 
  When RTE_LIBRTE_BBDEV_DEBUG is enabled, rte_device's driver
pointer is dereferenced twice in fpga_5gnr_fec's probe callback.
It causes a segmentation fault because this pointer is only
assigned after probe callback call.

This patch makes use of rte_pci_driver pointer instead.

Fixes: 0b5927cbcba7 ("baseband/fpga_5gnr_fec: add PMD for FPGA 5GNR FEC")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Chautru, Nicolas Oct. 6, 2020, 4:14 p.m. UTC | #1 
Thanks Maxime
I am not totally sure that this actually got broken in the very commit you point to (I think that there was another pci generic commit which changed the assumption when this pointer was set), but it doesn't harm to change anyway for stable build. 
Note this is already like this in the new PMD acc100.

Acked-By: Nicolas Chautru <nicolas.chautru@intel.com>

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 6, 2020 3:04 AM
> To: dev@dpdk.org; stable@dpdk.org; Chautru, Nicolas
> <nicolas.chautru@intel.com>; trix@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 1/2] baseband/fpga_5gnr_fec: fix segfaults with debug
> 
> When RTE_LIBRTE_BBDEV_DEBUG is enabled, rte_device's driver pointer is
> dereferenced twice in fpga_5gnr_fec's probe callback.
> It causes a segmentation fault because this pointer is only assigned after probe
> callback call.
> 
> This patch makes use of rte_pci_driver pointer instead.
> 
> Fixes: 0b5927cbcba7 ("baseband/fpga_5gnr_fec: add PMD for FPGA 5GNR
> FEC")
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> index 61f9c04ba2..11ee4d3e10 100644
> --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> @@ -1839,7 +1839,7 @@ fpga_5gnr_fec_init(struct rte_bbdev *dev, struct
> rte_pci_driver *drv)
> 
>  	rte_bbdev_log_debug(
>  			"Init device %s [%s] @ virtaddr %p phyaddr %#"PRIx64,
> -			dev->device->driver->name, dev->data->name,
> +			drv->driver.name, dev->data->name,
>  			(void *)pci_dev->mem_resource[0].addr,
>  			pci_dev->mem_resource[0].phys_addr);
>  }
> @@ -1895,7 +1895,7 @@ fpga_5gnr_fec_probe(struct rte_pci_driver *pci_drv,
>  		((uint16_t)(version_id >> 16)), ((uint16_t)version_id));
> 
>  #ifdef RTE_LIBRTE_BBDEV_DEBUG
> -	if (!strcmp(bbdev->device->driver->name,
> +	if (!strcmp(pci_drv->driver.name,

Why do you have to change this one?

Acked-by: Nicolas Chautru <nicolas.chautru@intel.com>

>  			RTE_STR(FPGA_5GNR_FEC_PF_DRIVER_NAME)))
>  		print_static_reg_debug_info(d->mmio_base);
>  #endif
> --
> 2.26.2
Maxime Coquelin Oct. 6, 2020, 4:34 p.m. UTC | #2 
Hi Nicolas,

On 10/6/20 6:14 PM, Chautru, Nicolas wrote:
> Thanks Maxime
> I am not totally sure that this actually got broken in the very commit you point to (I think that there was another pci generic commit which changed the assumption when this pointer was set), but it doesn't harm to change anyway for stable build. 
> Note this is already like this in the new PMD acc100.

For fpga_5gnr_fec, we reproduced the issue with v20.05, which was the
version the driver was introduced.

For fpga_lte_fec, it seems to be the same. The driver was in introduced
in v19.08, and dev->device.driver was also assigned after driver's probe
callback is called:

http://code.dpdk.org/dpdk/v19.08/source/drivers/bus/pci/pci_common.c#L212

Thanks,
Maxime

> Acked-By: Nicolas Chautru <nicolas.chautru@intel.com>
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, October 6, 2020 3:04 AM
>> To: dev@dpdk.org; stable@dpdk.org; Chautru, Nicolas
>> <nicolas.chautru@intel.com>; trix@redhat.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH 1/2] baseband/fpga_5gnr_fec: fix segfaults with debug
>>
>> When RTE_LIBRTE_BBDEV_DEBUG is enabled, rte_device's driver pointer is
>> dereferenced twice in fpga_5gnr_fec's probe callback.
>> It causes a segmentation fault because this pointer is only assigned after probe
>> callback call.
>>
>> This patch makes use of rte_pci_driver pointer instead.
>>
>> Fixes: 0b5927cbcba7 ("baseband/fpga_5gnr_fec: add PMD for FPGA 5GNR
>> FEC")
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>> b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>> index 61f9c04ba2..11ee4d3e10 100644
>> --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>> +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>> @@ -1839,7 +1839,7 @@ fpga_5gnr_fec_init(struct rte_bbdev *dev, struct
>> rte_pci_driver *drv)
>>
>>  	rte_bbdev_log_debug(
>>  			"Init device %s [%s] @ virtaddr %p phyaddr %#"PRIx64,
>> -			dev->device->driver->name, dev->data->name,
>> +			drv->driver.name, dev->data->name,
>>  			(void *)pci_dev->mem_resource[0].addr,
>>  			pci_dev->mem_resource[0].phys_addr);
>>  }
>> @@ -1895,7 +1895,7 @@ fpga_5gnr_fec_probe(struct rte_pci_driver *pci_drv,
>>  		((uint16_t)(version_id >> 16)), ((uint16_t)version_id));
>>
>>  #ifdef RTE_LIBRTE_BBDEV_DEBUG
>> -	if (!strcmp(bbdev->device->driver->name,
>> +	if (!strcmp(pci_drv->driver.name,
> 
> Why do you have to change this one?
> 
> Acked-by: Nicolas Chautru <nicolas.chautru@intel.com>
> 
>>  			RTE_STR(FPGA_5GNR_FEC_PF_DRIVER_NAME)))
>>  		print_static_reg_debug_info(d->mmio_base);
>>  #endif
>> --
>> 2.26.2
>
Chautru, Nicolas Oct. 6, 2020, 5:41 p.m. UTC | #3 
Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 6, 2020 9:34 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> stable@dpdk.org; trix@redhat.com
> Subject: Re: [PATCH 1/2] baseband/fpga_5gnr_fec: fix segfaults with debug
> 
> Hi Nicolas,
> 
> On 10/6/20 6:14 PM, Chautru, Nicolas wrote:
> > Thanks Maxime
> > I am not totally sure that this actually got broken in the very commit you
> point to (I think that there was another pci generic commit which changed
> the assumption when this pointer was set), but it doesn't harm to change
> anyway for stable build.
> > Note this is already like this in the new PMD acc100.
> 
> For fpga_5gnr_fec, we reproduced the issue with v20.05, which was the
> version the driver was introduced.
> 
> For fpga_lte_fec, it seems to be the same. The driver was in introduced in
> v19.08, and dev->device.driver was also assigned after driver's probe
> callback is called:
> 
> http://code.dpdk.org/dpdk/v19.08/source/drivers/bus/pci/pci_common.c#L
> 212

OK Thanks you are right. The change moving the driver assignment to end of probing was done in parallel prior to the formal upstreaming of these drivers. 

> 
> Thanks,
> Maxime
> 
> > Acked-By: Nicolas Chautru <nicolas.chautru@intel.com>
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, October 6, 2020 3:04 AM
> >> To: dev@dpdk.org; stable@dpdk.org; Chautru, Nicolas
> >> <nicolas.chautru@intel.com>; trix@redhat.com
> >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Subject: [PATCH 1/2] baseband/fpga_5gnr_fec: fix segfaults with debug
> >>
> >> When RTE_LIBRTE_BBDEV_DEBUG is enabled, rte_device's driver pointer
> >> is dereferenced twice in fpga_5gnr_fec's probe callback.
> >> It causes a segmentation fault because this pointer is only assigned
> >> after probe callback call.
> >>
> >> This patch makes use of rte_pci_driver pointer instead.
> >>
> >> Fixes: 0b5927cbcba7 ("baseband/fpga_5gnr_fec: add PMD for FPGA 5GNR
> >> FEC")
> >>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> ---
> >>  drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> >> b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> >> index 61f9c04ba2..11ee4d3e10 100644
> >> --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> >> +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> >> @@ -1839,7 +1839,7 @@ fpga_5gnr_fec_init(struct rte_bbdev *dev,
> >> struct rte_pci_driver *drv)
> >>
> >>  	rte_bbdev_log_debug(
> >>  			"Init device %s [%s] @ virtaddr %p phyaddr
> %#"PRIx64,
> >> -			dev->device->driver->name, dev->data->name,
> >> +			drv->driver.name, dev->data->name,
> >>  			(void *)pci_dev->mem_resource[0].addr,
> >>  			pci_dev->mem_resource[0].phys_addr);
> >>  }
> >> @@ -1895,7 +1895,7 @@ fpga_5gnr_fec_probe(struct rte_pci_driver
> *pci_drv,
> >>  		((uint16_t)(version_id >> 16)), ((uint16_t)version_id));
> >>
> >>  #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >> -	if (!strcmp(bbdev->device->driver->name,
> >> +	if (!strcmp(pci_drv->driver.name,
> >
> > Why do you have to change this one?
> >
> > Acked-by: Nicolas Chautru <nicolas.chautru@intel.com>
> >
> >>  			RTE_STR(FPGA_5GNR_FEC_PF_DRIVER_NAME)))
> >>  		print_static_reg_debug_info(d->mmio_base);
> >>  #endif
> >> --
> >> 2.26.2
> >
diff mbox series

Patch

diff --git a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
index 61f9c04ba2..11ee4d3e10 100644
--- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
+++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
@@ -1839,7 +1839,7 @@  fpga_5gnr_fec_init(struct rte_bbdev *dev, struct rte_pci_driver *drv)
 
 	rte_bbdev_log_debug(
 			"Init device %s [%s] @ virtaddr %p phyaddr %#"PRIx64,
-			dev->device->driver->name, dev->data->name,
+			drv->driver.name, dev->data->name,
 			(void *)pci_dev->mem_resource[0].addr,
 			pci_dev->mem_resource[0].phys_addr);
 }
@@ -1895,7 +1895,7 @@  fpga_5gnr_fec_probe(struct rte_pci_driver *pci_drv,
 		((uint16_t)(version_id >> 16)), ((uint16_t)version_id));
 
 #ifdef RTE_LIBRTE_BBDEV_DEBUG
-	if (!strcmp(bbdev->device->driver->name,
+	if (!strcmp(pci_drv->driver.name,
 			RTE_STR(FPGA_5GNR_FEC_PF_DRIVER_NAME)))
 		print_static_reg_debug_info(d->mmio_base);
 #endif