[PATCH rdma-rc 0/4] RDMA/bnxt_re: Bug fixes for 6.14

[PATCH rdma-rc 0/4] RDMA/bnxt_re: Bug fixes for 6.14

[Selvin Xavier]

Includes some of the critical fixes found while testing
the 6.14 branch. Please review and apply.

Thanks,
Selvin

Kalesh AP (3):
  RDMA/bnxt_re: Fix an issue in bnxt_re_async_notifier
  RDMA/bnxt_re: Add sanity checks on rdev validity
  RDMA/bnxt_re: Fix issue in the unload path

Selvin Xavier (1):
  RDMA/bnxt_re: Fix the statistics for Gen P7 VF

 drivers/infiniband/hw/bnxt_re/bnxt_re.h     |  1 -
 drivers/infiniband/hw/bnxt_re/hw_counters.c |  4 ++--
 drivers/infiniband/hw/bnxt_re/main.c        | 22 +++++++++++-----------
 drivers/infiniband/hw/bnxt_re/qplib_res.h   |  8 ++++++++
 4 files changed, 21 insertions(+), 14 deletions(-)

-- 
2.5.5

[PATCH rdma-rc 1/4] RDMA/bnxt_re: Fix an issue in bnxt_re_async_notifier

[Selvin Xavier]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

In the bnxt_re_async_notifier() callback, the way driver retrieves
rdev pointer is wrong. The rdev pointer should be parsed from
adev pointer as while registering with the L2 for ULP, driver uses
the aux device pointer for the handle.

Fixes: 7fea32784068 ("RDMA/bnxt_re: Add Async event handling support")
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index e9e4da4..c4c3d67 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -396,11 +396,16 @@ static void bnxt_re_dcb_wq_task(struct work_struct *work)
 
 static void bnxt_re_async_notifier(void *handle, struct hwrm_async_event_cmpl *cmpl)
 {
-	struct bnxt_re_dev *rdev = (struct bnxt_re_dev *)handle;
+	struct bnxt_re_en_dev_info *en_info = auxiliary_get_drvdata(handle);
 	struct bnxt_re_dcb_work *dcb_work;
+	struct bnxt_re_dev *rdev;
 	u32 data1, data2;
 	u16 event_id;
 
+	rdev = en_info->rdev;
+	if (!rdev)
+		return;
+
 	event_id = le16_to_cpu(cmpl->event_id);
 	data1 = le32_to_cpu(cmpl->event_data1);
 	data2 = le32_to_cpu(cmpl->event_data2);
-- 
2.5.5

[PATCH rdma-rc 2/4] RDMA/bnxt_re: Add sanity checks on rdev validity

[Selvin Xavier]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

There is a possibility that ulp_irq_stop and ulp_irq_start
callbacks will be called when the device is in detached state.
This can cause a crash due to NULL pointer dereference as
the rdev is already freed.

Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index c4c3d67..89ac5c2 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -438,6 +438,8 @@ static void bnxt_re_stop_irq(void *handle, bool reset)
 	int indx;
 
 	rdev = en_info->rdev;
+	if (!rdev)
+		return;
 	rcfw = &rdev->rcfw;
 
 	if (reset) {
@@ -466,6 +468,8 @@ static void bnxt_re_start_irq(void *handle, struct bnxt_msix_entry *ent)
 	int indx, rc;
 
 	rdev = en_info->rdev;
+	if (!rdev)
+		return;
 	msix_ent = rdev->nqr->msix_entries;
 	rcfw = &rdev->rcfw;
 	if (!ent) {
@@ -2438,6 +2442,7 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
 	ibdev_info(&rdev->ibdev, "%s: L2 driver notified to stop en_state 0x%lx",
 		   __func__, en_dev->en_state);
 	bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
+	bnxt_re_update_en_info_rdev(NULL, en_info, adev);
 	mutex_unlock(&bnxt_re_mutex);
 
 	return 0;
-- 
2.5.5

[PATCH rdma-rc 3/4] RDMA/bnxt_re: Fix issue in the unload path

[Selvin Xavier]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

The cited comment removed the netdev notifier register call
from the driver. But, it did not remove the cleanup code from
the unload path. As a result, driver unload is not clean and
resulted in undesired behaviour.

Fixes: d3b15fcc4201 ("RDMA/bnxt_re: Remove deliver net device event")
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/bnxt_re.h |  1 -
 drivers/infiniband/hw/bnxt_re/main.c    | 10 ----------
 2 files changed, 11 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/bnxt_re.h b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
index b91a85a..3721446 100644
--- a/drivers/infiniband/hw/bnxt_re/bnxt_re.h
+++ b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
@@ -187,7 +187,6 @@ struct bnxt_re_dev {
 #define BNXT_RE_FLAG_ISSUE_ROCE_STATS          29
 	struct net_device		*netdev;
 	struct auxiliary_device         *adev;
-	struct notifier_block		nb;
 	unsigned int			version, major, minor;
 	struct bnxt_qplib_chip_ctx	*chip_ctx;
 	struct bnxt_en_dev		*en_dev;
diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index 89ac5c2..a94c8c5 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -1359,7 +1359,6 @@ static struct bnxt_re_dev *bnxt_re_dev_add(struct auxiliary_device *adev,
 		return NULL;
 	}
 	/* Default values */
-	rdev->nb.notifier_call = NULL;
 	rdev->netdev = en_dev->net;
 	rdev->en_dev = en_dev;
 	rdev->adev = adev;
@@ -2354,15 +2353,6 @@ static int bnxt_re_add_device(struct auxiliary_device *adev, u8 op_type)
 static void bnxt_re_remove_device(struct bnxt_re_dev *rdev, u8 op_type,
 				  struct auxiliary_device *aux_dev)
 {
-	if (rdev->nb.notifier_call) {
-		unregister_netdevice_notifier(&rdev->nb);
-		rdev->nb.notifier_call = NULL;
-	} else {
-		/* If notifier is null, we should have already done a
-		 * clean up before coming here.
-		 */
-		return;
-	}
 	bnxt_re_setup_cc(rdev, false);
 	ib_unregister_device(&rdev->ibdev);
 	bnxt_re_dev_uninit(rdev, op_type);
-- 
2.5.5

[PATCH rdma-rc 4/4] RDMA/bnxt_re: Fix the statistics for Gen P7 VF

[Selvin Xavier]

Gen P7 VF support the extended stats and is prevented
by a VF check. Fix the check to issue the FW command
for GenP7 VFs also.

Fixes: 1801d87b3598 ("RDMA/bnxt_re: Support new 5760X P7 devices")
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/hw_counters.c | 4 ++--
 drivers/infiniband/hw/bnxt_re/qplib_res.h   | 8 ++++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/hw_counters.c b/drivers/infiniband/hw/bnxt_re/hw_counters.c
index 3ac47f4..f039aef 100644
--- a/drivers/infiniband/hw/bnxt_re/hw_counters.c
+++ b/drivers/infiniband/hw/bnxt_re/hw_counters.c
@@ -348,8 +348,8 @@ int bnxt_re_ib_get_hw_stats(struct ib_device *ibdev,
 			goto done;
 		}
 		bnxt_re_copy_err_stats(rdev, stats, err_s);
-		if (_is_ext_stats_supported(rdev->dev_attr->dev_cap_flags) &&
-		    !rdev->is_virtfn) {
+		if (bnxt_ext_stats_supported(rdev->chip_ctx, rdev->dev_attr->dev_cap_flags,
+					     rdev->is_virtfn)) {
 			rc = bnxt_re_get_ext_stat(rdev, stats);
 			if (rc) {
 				clear_bit(BNXT_RE_FLAG_ISSUE_ROCE_STATS,
diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.h b/drivers/infiniband/hw/bnxt_re/qplib_res.h
index be5d907..7119902 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_res.h
+++ b/drivers/infiniband/hw/bnxt_re/qplib_res.h
@@ -547,6 +547,14 @@ static inline bool _is_ext_stats_supported(u16 dev_cap_flags)
 		CREQ_QUERY_FUNC_RESP_SB_EXT_STATS;
 }
 
+static inline int bnxt_ext_stats_supported(struct bnxt_qplib_chip_ctx *ctx,
+					   u16 flags, bool virtfn)
+{
+	/* ext stats supported if cap flag is set AND is a PF OR a Thor2 VF */
+	return (_is_ext_stats_supported(flags) &&
+		((virtfn && bnxt_qplib_is_chip_gen_p7(ctx)) || (!virtfn)));
+}
+
 static inline bool _is_hw_retx_supported(u16 dev_cap_flags)
 {
 	return dev_cap_flags &
-- 
2.5.5

Re: [PATCH rdma-rc 2/4] RDMA/bnxt_re: Add sanity checks on rdev validity

[Leon Romanovsky]

On Tue, Feb 04, 2025 at 12:21:23AM -0800, Selvin Xavier wrote:
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> 
> There is a possibility that ulp_irq_stop and ulp_irq_start
> callbacks will be called when the device is in detached state.
> This can cause a crash due to NULL pointer dereference as
> the rdev is already freed.

Description and code doesn't match. If "possibility" exists, there is
no protection from concurrent detach and ulp_irq_stop. If there is such
protection, they can't race.

The main idea of auxiliary bus is to remove the need to implement driver
specific ops.

> 
> Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
>  drivers/infiniband/hw/bnxt_re/main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index c4c3d67..89ac5c2 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -438,6 +438,8 @@ static void bnxt_re_stop_irq(void *handle, bool reset)
>  	int indx;
>  
>  	rdev = en_info->rdev;
> +	if (!rdev)
> +		return;

This can be seen as an example why I'm so negative about assigning NULL
to the pointers after object is destroyed. Such assignment makes layer
violation much easier job to do.

Thanks

>  	rcfw = &rdev->rcfw;
>  
>  	if (reset) {
> @@ -466,6 +468,8 @@ static void bnxt_re_start_irq(void *handle, struct bnxt_msix_entry *ent)
>  	int indx, rc;
>  
>  	rdev = en_info->rdev;
> +	if (!rdev)
> +		return;
>  	msix_ent = rdev->nqr->msix_entries;
>  	rcfw = &rdev->rcfw;
>  	if (!ent) {
> @@ -2438,6 +2442,7 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
>  	ibdev_info(&rdev->ibdev, "%s: L2 driver notified to stop en_state 0x%lx",
>  		   __func__, en_dev->en_state);
>  	bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
> +	bnxt_re_update_en_info_rdev(NULL, en_info, adev);
>  	mutex_unlock(&bnxt_re_mutex);
>  
>  	return 0;
> -- 
> 2.5.5
> 

Re: [PATCH rdma-rc 2/4] RDMA/bnxt_re: Add sanity checks on rdev validity

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 3312 bytes --]

Hi Leon,

On Tue, Feb 4, 2025 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Tue, Feb 04, 2025 at 12:21:23AM -0800, Selvin Xavier wrote:
> > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> >
> > There is a possibility that ulp_irq_stop and ulp_irq_start
> > callbacks will be called when the device is in detached state.
> > This can cause a crash due to NULL pointer dereference as
> > the rdev is already freed.
>
> Description and code doesn't match. If "possibility" exists, there is
> no protection from concurrent detach and ulp_irq_stop. If there is such
> protection, they can't race.
>
> The main idea of auxiliary bus is to remove the need to implement driver
> specific ops.

There is no race condition here.

Let me explain the scenario.

User is doing a devlink reload reinit. As part of that, the Ethernet
driver first invokes the auxiliary bus suspend callback. The RDMA driver
will do the unwinding operation and hence rdev will be freed.

After that, during the devlink sequence, Ethernet driver invokes the
ulp_irq_stop() callback and this resulted in the NULL pointer
dereference as the RDMA driver is in detached state and the rdev is
already freed.

We are trying to address the NULL pointer dereference issue here.

The driver specific ops, ulp_irq_stop and ulp_irq_start are required.
Broadcom Ethernet and RDMA drivers are designed like that to manage
IRQs between them.

Hope this clarifies your question.
>
> >
> > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > ---
> >  drivers/infiniband/hw/bnxt_re/main.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > index c4c3d67..89ac5c2 100644
> > --- a/drivers/infiniband/hw/bnxt_re/main.c
> > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > @@ -438,6 +438,8 @@ static void bnxt_re_stop_irq(void *handle, bool reset)
> >       int indx;
> >
> >       rdev = en_info->rdev;
> > +     if (!rdev)
> > +             return;
>
> This can be seen as an example why I'm so negative about assigning NULL
> to the pointers after object is destroyed. Such assignment makes layer
> violation much easier job to do.
>
> Thanks
>
> >       rcfw = &rdev->rcfw;
> >
> >       if (reset) {
> > @@ -466,6 +468,8 @@ static void bnxt_re_start_irq(void *handle, struct bnxt_msix_entry *ent)
> >       int indx, rc;
> >
> >       rdev = en_info->rdev;
> > +     if (!rdev)
> > +             return;
> >       msix_ent = rdev->nqr->msix_entries;
> >       rcfw = &rdev->rcfw;
> >       if (!ent) {
> > @@ -2438,6 +2442,7 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
> >       ibdev_info(&rdev->ibdev, "%s: L2 driver notified to stop en_state 0x%lx",
> >                  __func__, en_dev->en_state);
> >       bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
> > +     bnxt_re_update_en_info_rdev(NULL, en_info, adev);
> >       mutex_unlock(&bnxt_re_mutex);
> >
> >       return 0;
> > --
> > 2.5.5
> >



-- 
Regards,
Kalesh AP

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

Re: [PATCH rdma-rc 2/4] RDMA/bnxt_re: Add sanity checks on rdev validity

[Leon Romanovsky]

On Tue, Feb 04, 2025 at 10:10:38PM +0530, Kalesh Anakkur Purayil wrote:
> Hi Leon,
> 
> On Tue, Feb 4, 2025 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Tue, Feb 04, 2025 at 12:21:23AM -0800, Selvin Xavier wrote:
> > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > >
> > > There is a possibility that ulp_irq_stop and ulp_irq_start
> > > callbacks will be called when the device is in detached state.
> > > This can cause a crash due to NULL pointer dereference as
> > > the rdev is already freed.
> >
> > Description and code doesn't match. If "possibility" exists, there is
> > no protection from concurrent detach and ulp_irq_stop. If there is such
> > protection, they can't race.
> >
> > The main idea of auxiliary bus is to remove the need to implement driver
> > specific ops.
> 
> There is no race condition here.
> 
> Let me explain the scenario.
> 
> User is doing a devlink reload reinit. As part of that, the Ethernet
> driver first invokes the auxiliary bus suspend callback. The RDMA driver
> will do the unwinding operation and hence rdev will be freed.
> 
> After that, during the devlink sequence, Ethernet driver invokes the
> ulp_irq_stop() callback and this resulted in the NULL pointer
> dereference as the RDMA driver is in detached state and the rdev is
> already freed.

Shouldn't devlink reload completely release all auxiliary drivers?
Why are you keeping BNXT RDMA driver during reload?
BNXT core driver controls reload, it shouldn't call to drivers which
doesn't exist.

> 
> We are trying to address the NULL pointer dereference issue here.

You are hiding bugs and not fixing them.

> 
> The driver specific ops, ulp_irq_stop and ulp_irq_start are required.
> Broadcom Ethernet and RDMA drivers are designed like that to manage
> IRQs between them.
> 
> Hope this clarifies your question.
> >
> > >
> > > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > ---
> > >  drivers/infiniband/hw/bnxt_re/main.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > > index c4c3d67..89ac5c2 100644
> > > --- a/drivers/infiniband/hw/bnxt_re/main.c
> > > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > > @@ -438,6 +438,8 @@ static void bnxt_re_stop_irq(void *handle, bool reset)
> > >       int indx;
> > >
> > >       rdev = en_info->rdev;
> > > +     if (!rdev)
> > > +             return;
> >
> > This can be seen as an example why I'm so negative about assigning NULL
> > to the pointers after object is destroyed. Such assignment makes layer
> > violation much easier job to do.
> >
> > Thanks
> >
> > >       rcfw = &rdev->rcfw;
> > >
> > >       if (reset) {
> > > @@ -466,6 +468,8 @@ static void bnxt_re_start_irq(void *handle, struct bnxt_msix_entry *ent)
> > >       int indx, rc;
> > >
> > >       rdev = en_info->rdev;
> > > +     if (!rdev)
> > > +             return;
> > >       msix_ent = rdev->nqr->msix_entries;
> > >       rcfw = &rdev->rcfw;
> > >       if (!ent) {
> > > @@ -2438,6 +2442,7 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
> > >       ibdev_info(&rdev->ibdev, "%s: L2 driver notified to stop en_state 0x%lx",
> > >                  __func__, en_dev->en_state);
> > >       bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
> > > +     bnxt_re_update_en_info_rdev(NULL, en_info, adev);
> > >       mutex_unlock(&bnxt_re_mutex);
> > >
> > >       return 0;
> > > --
> > > 2.5.5
> > >
> 
> 
> 
> -- 
> Regards,
> Kalesh AP

Re: [PATCH rdma-rc 2/4] RDMA/bnxt_re: Add sanity checks on rdev validity

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 4583 bytes --]

Hi Leon,

On Wed, Feb 5, 2025 at 12:47 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Tue, Feb 04, 2025 at 10:10:38PM +0530, Kalesh Anakkur Purayil wrote:
> > Hi Leon,
> >
> > On Tue, Feb 4, 2025 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Tue, Feb 04, 2025 at 12:21:23AM -0800, Selvin Xavier wrote:
> > > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > >
> > > > There is a possibility that ulp_irq_stop and ulp_irq_start
> > > > callbacks will be called when the device is in detached state.
> > > > This can cause a crash due to NULL pointer dereference as
> > > > the rdev is already freed.
> > >
> > > Description and code doesn't match. If "possibility" exists, there is
> > > no protection from concurrent detach and ulp_irq_stop. If there is such
> > > protection, they can't race.
> > >
> > > The main idea of auxiliary bus is to remove the need to implement driver
> > > specific ops.
> >
> > There is no race condition here.
> >
> > Let me explain the scenario.
> >
> > User is doing a devlink reload reinit. As part of that, the Ethernet
> > driver first invokes the auxiliary bus suspend callback. The RDMA driver
> > will do the unwinding operation and hence rdev will be freed.
> >
> > After that, during the devlink sequence, Ethernet driver invokes the
> > ulp_irq_stop() callback and this resulted in the NULL pointer
> > dereference as the RDMA driver is in detached state and the rdev is
> > already freed.
>
> Shouldn't devlink reload completely release all auxiliary drivers?
> Why are you keeping BNXT RDMA driver during reload?

That is the current design. BNXT core Ethernet driver will not destroy
the auxiliary device for RDMA, but just calls the suspend callback. As
a result, RDMA driver will remains loaded and remains registered with
the Ethernet driver instance.
> BNXT core driver controls reload, it shouldn't call to drivers which
> doesn't exist.
Since the RDMA driver instance is registered with Ethernet driver,
core Ethernet driver invokes the callback.
>
> >
> > We are trying to address the NULL pointer dereference issue here.
>
> You are hiding bugs and not fixing them.

Yes, but this change is critical for the current design of the driver.
>
> >
> > The driver specific ops, ulp_irq_stop and ulp_irq_start are required.
> > Broadcom Ethernet and RDMA drivers are designed like that to manage
> > IRQs between them.
> >
> > Hope this clarifies your question.
> > >
> > > >
> > > > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > > ---
> > > >  drivers/infiniband/hw/bnxt_re/main.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > > > index c4c3d67..89ac5c2 100644
> > > > --- a/drivers/infiniband/hw/bnxt_re/main.c
> > > > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > > > @@ -438,6 +438,8 @@ static void bnxt_re_stop_irq(void *handle, bool reset)
> > > >       int indx;
> > > >
> > > >       rdev = en_info->rdev;
> > > > +     if (!rdev)
> > > > +             return;
> > >
> > > This can be seen as an example why I'm so negative about assigning NULL
> > > to the pointers after object is destroyed. Such assignment makes layer
> > > violation much easier job to do.
> > >
> > > Thanks
> > >
> > > >       rcfw = &rdev->rcfw;
> > > >
> > > >       if (reset) {
> > > > @@ -466,6 +468,8 @@ static void bnxt_re_start_irq(void *handle, struct bnxt_msix_entry *ent)
> > > >       int indx, rc;
> > > >
> > > >       rdev = en_info->rdev;
> > > > +     if (!rdev)
> > > > +             return;
> > > >       msix_ent = rdev->nqr->msix_entries;
> > > >       rcfw = &rdev->rcfw;
> > > >       if (!ent) {
> > > > @@ -2438,6 +2442,7 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
> > > >       ibdev_info(&rdev->ibdev, "%s: L2 driver notified to stop en_state 0x%lx",
> > > >                  __func__, en_dev->en_state);
> > > >       bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
> > > > +     bnxt_re_update_en_info_rdev(NULL, en_info, adev);
> > > >       mutex_unlock(&bnxt_re_mutex);
> > > >
> > > >       return 0;
> > > > --
> > > > 2.5.5
> > > >
> >
> >
> >
> > --
> > Regards,
> > Kalesh AP
>
>


-- 
Regards,
Kalesh AP

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

Re: [PATCH rdma-rc 2/4] RDMA/bnxt_re: Add sanity checks on rdev validity

[Leon Romanovsky]

On Wed, Feb 05, 2025 at 01:54:14PM +0530, Kalesh Anakkur Purayil wrote:
> Hi Leon,
> 
> On Wed, Feb 5, 2025 at 12:47 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Tue, Feb 04, 2025 at 10:10:38PM +0530, Kalesh Anakkur Purayil wrote:
> > > Hi Leon,
> > >
> > > On Tue, Feb 4, 2025 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Tue, Feb 04, 2025 at 12:21:23AM -0800, Selvin Xavier wrote:
> > > > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > > >
> > > > > There is a possibility that ulp_irq_stop and ulp_irq_start
> > > > > callbacks will be called when the device is in detached state.
> > > > > This can cause a crash due to NULL pointer dereference as
> > > > > the rdev is already freed.
> > > >
> > > > Description and code doesn't match. If "possibility" exists, there is
> > > > no protection from concurrent detach and ulp_irq_stop. If there is such
> > > > protection, they can't race.
> > > >
> > > > The main idea of auxiliary bus is to remove the need to implement driver
> > > > specific ops.
> > >
> > > There is no race condition here.
> > >
> > > Let me explain the scenario.
> > >
> > > User is doing a devlink reload reinit. As part of that, the Ethernet
> > > driver first invokes the auxiliary bus suspend callback. The RDMA driver
> > > will do the unwinding operation and hence rdev will be freed.
> > >
> > > After that, during the devlink sequence, Ethernet driver invokes the
> > > ulp_irq_stop() callback and this resulted in the NULL pointer
> > > dereference as the RDMA driver is in detached state and the rdev is
> > > already freed.
> >
> > Shouldn't devlink reload completely release all auxiliary drivers?
> > Why are you keeping BNXT RDMA driver during reload?
> 
> That is the current design. BNXT core Ethernet driver will not destroy
> the auxiliary device for RDMA, but just calls the suspend callback. As
> a result, RDMA driver will remains loaded and remains registered with
> the Ethernet driver instance.

This is wrong.

> > BNXT core driver controls reload, it shouldn't call to drivers which
> > doesn't exist.
> Since the RDMA driver instance is registered with Ethernet driver,
> core Ethernet driver invokes the callback.
> >
> > >
> > > We are trying to address the NULL pointer dereference issue here.
> >
> > You are hiding bugs and not fixing them.
> 
> Yes, but this change is critical for the current design of the driver.

Please fix it once and for all by doing proper reload sequence.
I warned you that setting NULLs to pointers hide bugs.
https://lore.kernel.org/linux-rdma/20250114112555.GG3146852@unreal/

Thanks

> >
> > >
> > > The driver specific ops, ulp_irq_stop and ulp_irq_start are required.
> > > Broadcom Ethernet and RDMA drivers are designed like that to manage
> > > IRQs between them.
> > >
> > > Hope this clarifies your question.
> > > >
> > > > >
> > > > > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> > > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > > > ---
> > > > >  drivers/infiniband/hw/bnxt_re/main.c | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > > > > index c4c3d67..89ac5c2 100644
> > > > > --- a/drivers/infiniband/hw/bnxt_re/main.c
> > > > > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > > > > @@ -438,6 +438,8 @@ static void bnxt_re_stop_irq(void *handle, bool reset)
> > > > >       int indx;
> > > > >
> > > > >       rdev = en_info->rdev;
> > > > > +     if (!rdev)
> > > > > +             return;
> > > >
> > > > This can be seen as an example why I'm so negative about assigning NULL
> > > > to the pointers after object is destroyed. Such assignment makes layer
> > > > violation much easier job to do.
> > > >
> > > > Thanks
> > > >
> > > > >       rcfw = &rdev->rcfw;
> > > > >
> > > > >       if (reset) {
> > > > > @@ -466,6 +468,8 @@ static void bnxt_re_start_irq(void *handle, struct bnxt_msix_entry *ent)
> > > > >       int indx, rc;
> > > > >
> > > > >       rdev = en_info->rdev;
> > > > > +     if (!rdev)
> > > > > +             return;
> > > > >       msix_ent = rdev->nqr->msix_entries;
> > > > >       rcfw = &rdev->rcfw;
> > > > >       if (!ent) {
> > > > > @@ -2438,6 +2442,7 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
> > > > >       ibdev_info(&rdev->ibdev, "%s: L2 driver notified to stop en_state 0x%lx",
> > > > >                  __func__, en_dev->en_state);
> > > > >       bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
> > > > > +     bnxt_re_update_en_info_rdev(NULL, en_info, adev);
> > > > >       mutex_unlock(&bnxt_re_mutex);
> > > > >
> > > > >       return 0;
> > > > > --
> > > > > 2.5.5
> > > > >
> > >
> > >
> > >
> > > --
> > > Regards,
> > > Kalesh AP
> >
> >
> 
> 
> -- 
> Regards,
> Kalesh AP

Re: [PATCH rdma-rc 2/4] RDMA/bnxt_re: Add sanity checks on rdev validity

[Jason Gunthorpe]

On Wed, Feb 05, 2025 at 01:54:14PM +0530, Kalesh Anakkur Purayil wrote:
> > Shouldn't devlink reload completely release all auxiliary drivers?
> > Why are you keeping BNXT RDMA driver during reload?
> 
> That is the current design. BNXT core Ethernet driver will not destroy
> the auxiliary device for RDMA, but just calls the suspend callback. As
> a result, RDMA driver will remains loaded and remains registered with
> the Ethernet driver instance.

What is the difference between your suspend and reloading the driver??

Do you keep the ib device registered during suspend and all uverbs
contexts open? How does that work???

Jason

Re: [PATCH rdma-rc 2/4] RDMA/bnxt_re: Add sanity checks on rdev validity

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 5990 bytes --]

Hi Leon,

On Wed, Feb 5, 2025 at 3:22 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Wed, Feb 05, 2025 at 01:54:14PM +0530, Kalesh Anakkur Purayil wrote:
> > Hi Leon,
> >
> > On Wed, Feb 5, 2025 at 12:47 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Tue, Feb 04, 2025 at 10:10:38PM +0530, Kalesh Anakkur Purayil wrote:
> > > > Hi Leon,
> > > >
> > > > On Tue, Feb 4, 2025 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > >
> > > > > On Tue, Feb 04, 2025 at 12:21:23AM -0800, Selvin Xavier wrote:
> > > > > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > > > >
> > > > > > There is a possibility that ulp_irq_stop and ulp_irq_start
> > > > > > callbacks will be called when the device is in detached state.
> > > > > > This can cause a crash due to NULL pointer dereference as
> > > > > > the rdev is already freed.
> > > > >
> > > > > Description and code doesn't match. If "possibility" exists, there is
> > > > > no protection from concurrent detach and ulp_irq_stop. If there is such
> > > > > protection, they can't race.
> > > > >
> > > > > The main idea of auxiliary bus is to remove the need to implement driver
> > > > > specific ops.
> > > >
> > > > There is no race condition here.
> > > >
> > > > Let me explain the scenario.
> > > >
> > > > User is doing a devlink reload reinit. As part of that, the Ethernet
> > > > driver first invokes the auxiliary bus suspend callback. The RDMA driver
> > > > will do the unwinding operation and hence rdev will be freed.
> > > >
> > > > After that, during the devlink sequence, Ethernet driver invokes the
> > > > ulp_irq_stop() callback and this resulted in the NULL pointer
> > > > dereference as the RDMA driver is in detached state and the rdev is
> > > > already freed.
> > >
> > > Shouldn't devlink reload completely release all auxiliary drivers?
> > > Why are you keeping BNXT RDMA driver during reload?
> >
> > That is the current design. BNXT core Ethernet driver will not destroy
> > the auxiliary device for RDMA, but just calls the suspend callback. As
> > a result, RDMA driver will remains loaded and remains registered with
> > the Ethernet driver instance.
>
> This is wrong.

We understand that. BNXT core driver team has already started working
on the auxiliary device removal instead of invoking auxdrv->suspend
callback in the devlink relaod path. That will avoid these NULL checks
in RDMA driver. for time being we need these NULL checks.
That will be posted to net-next tree once internal testing and review is done.
>
> > > BNXT core driver controls reload, it shouldn't call to drivers which
> > > doesn't exist.
> > Since the RDMA driver instance is registered with Ethernet driver,
> > core Ethernet driver invokes the callback.
> > >
> > > >
> > > > We are trying to address the NULL pointer dereference issue here.
> > >
> > > You are hiding bugs and not fixing them.
> >
> > Yes, but this change is critical for the current design of the driver.
>
> Please fix it once and for all by doing proper reload sequence.
> I warned you that setting NULLs to pointers hide bugs.
> https://lore.kernel.org/linux-rdma/20250114112555.GG3146852@unreal/

Yes, I understand. We will work on the suggestion that you had given
based on the new design mentioned in last comment.
>
> Thanks
>
> > >
> > > >
> > > > The driver specific ops, ulp_irq_stop and ulp_irq_start are required.
> > > > Broadcom Ethernet and RDMA drivers are designed like that to manage
> > > > IRQs between them.
> > > >
> > > > Hope this clarifies your question.
> > > > >
> > > > > >
> > > > > > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> > > > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > > > > ---
> > > > > >  drivers/infiniband/hw/bnxt_re/main.c | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > > > > > index c4c3d67..89ac5c2 100644
> > > > > > --- a/drivers/infiniband/hw/bnxt_re/main.c
> > > > > > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > > > > > @@ -438,6 +438,8 @@ static void bnxt_re_stop_irq(void *handle, bool reset)
> > > > > >       int indx;
> > > > > >
> > > > > >       rdev = en_info->rdev;
> > > > > > +     if (!rdev)
> > > > > > +             return;
> > > > >
> > > > > This can be seen as an example why I'm so negative about assigning NULL
> > > > > to the pointers after object is destroyed. Such assignment makes layer
> > > > > violation much easier job to do.
> > > > >
> > > > > Thanks
> > > > >
> > > > > >       rcfw = &rdev->rcfw;
> > > > > >
> > > > > >       if (reset) {
> > > > > > @@ -466,6 +468,8 @@ static void bnxt_re_start_irq(void *handle, struct bnxt_msix_entry *ent)
> > > > > >       int indx, rc;
> > > > > >
> > > > > >       rdev = en_info->rdev;
> > > > > > +     if (!rdev)
> > > > > > +             return;
> > > > > >       msix_ent = rdev->nqr->msix_entries;
> > > > > >       rcfw = &rdev->rcfw;
> > > > > >       if (!ent) {
> > > > > > @@ -2438,6 +2442,7 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
> > > > > >       ibdev_info(&rdev->ibdev, "%s: L2 driver notified to stop en_state 0x%lx",
> > > > > >                  __func__, en_dev->en_state);
> > > > > >       bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
> > > > > > +     bnxt_re_update_en_info_rdev(NULL, en_info, adev);
> > > > > >       mutex_unlock(&bnxt_re_mutex);
> > > > > >
> > > > > >       return 0;
> > > > > > --
> > > > > > 2.5.5
> > > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Regards,
> > > > Kalesh AP
> > >
> > >
> >
> >
> > --
> > Regards,
> > Kalesh AP
>
>


-- 
Regards,
Kalesh AP

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

Re: [PATCH rdma-rc 2/4] RDMA/bnxt_re: Add sanity checks on rdev validity

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

On Wed, Feb 5, 2025 at 8:17 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Feb 05, 2025 at 01:54:14PM +0530, Kalesh Anakkur Purayil wrote:
> > > Shouldn't devlink reload completely release all auxiliary drivers?
> > > Why are you keeping BNXT RDMA driver during reload?
> >
> > That is the current design. BNXT core Ethernet driver will not destroy
> > the auxiliary device for RDMA, but just calls the suspend callback. As
> > a result, RDMA driver will remains loaded and remains registered with
> > the Ethernet driver instance.
>
> What is the difference between your suspend and reloading the driver??
>
> Do you keep the ib device registered during suspend and all uverbs
> contexts open? How does that work???

We unregister the IB device in both cases.
The only difference is in the suspend path, the communication channel
with the Ethernet driver is still active. As a result, the ethernet
driver may invoke ULP callbacks even though the IB device is removed.

As I mentioned in my last response to Leon, we are working on to
improve this design to remove the auxiliary device completely instead
of invoking auxdrv->suspend.
>
> Jason



-- 
Regards,
Kalesh AP

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

Re: [PATCH rdma-rc 2/4] RDMA/bnxt_re: Add sanity checks on rdev validity

[Selvin Xavier]

[-- Attachment #1: Type: text/plain, Size: 6546 bytes --]

On Wed, Feb 5, 2025 at 9:45 PM Kalesh Anakkur Purayil
<kalesh-anakkur.purayil@broadcom.com> wrote:
>
> Hi Leon,
>
> On Wed, Feb 5, 2025 at 3:22 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Wed, Feb 05, 2025 at 01:54:14PM +0530, Kalesh Anakkur Purayil wrote:
> > > Hi Leon,
> > >
> > > On Wed, Feb 5, 2025 at 12:47 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Tue, Feb 04, 2025 at 10:10:38PM +0530, Kalesh Anakkur Purayil wrote:
> > > > > Hi Leon,
> > > > >
> > > > > On Tue, Feb 4, 2025 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > >
> > > > > > On Tue, Feb 04, 2025 at 12:21:23AM -0800, Selvin Xavier wrote:
> > > > > > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > > > > >
> > > > > > > There is a possibility that ulp_irq_stop and ulp_irq_start
> > > > > > > callbacks will be called when the device is in detached state.
> > > > > > > This can cause a crash due to NULL pointer dereference as
> > > > > > > the rdev is already freed.
> > > > > >
> > > > > > Description and code doesn't match. If "possibility" exists, there is
> > > > > > no protection from concurrent detach and ulp_irq_stop. If there is such
> > > > > > protection, they can't race.
> > > > > >
> > > > > > The main idea of auxiliary bus is to remove the need to implement driver
> > > > > > specific ops.
> > > > >
> > > > > There is no race condition here.
> > > > >
> > > > > Let me explain the scenario.
> > > > >
> > > > > User is doing a devlink reload reinit. As part of that, the Ethernet
> > > > > driver first invokes the auxiliary bus suspend callback. The RDMA driver
> > > > > will do the unwinding operation and hence rdev will be freed.
> > > > >
> > > > > After that, during the devlink sequence, Ethernet driver invokes the
> > > > > ulp_irq_stop() callback and this resulted in the NULL pointer
> > > > > dereference as the RDMA driver is in detached state and the rdev is
> > > > > already freed.
> > > >
> > > > Shouldn't devlink reload completely release all auxiliary drivers?
> > > > Why are you keeping BNXT RDMA driver during reload?
> > >
> > > That is the current design. BNXT core Ethernet driver will not destroy
> > > the auxiliary device for RDMA, but just calls the suspend callback. As
> > > a result, RDMA driver will remains loaded and remains registered with
> > > the Ethernet driver instance.
> >
> > This is wrong.
>
> We understand that. BNXT core driver team has already started working
> on the auxiliary device removal instead of invoking auxdrv->suspend
> callback in the devlink relaod path. That will avoid these NULL checks
> in RDMA driver. for time being we need these NULL checks.
> That will be posted to net-next tree once internal testing and review is done.
> >
> > > > BNXT core driver controls reload, it shouldn't call to drivers which
> > > > doesn't exist.
> > > Since the RDMA driver instance is registered with Ethernet driver,
> > > core Ethernet driver invokes the callback.
> > > >
> > > > >
> > > > > We are trying to address the NULL pointer dereference issue here.
> > > >
> > > > You are hiding bugs and not fixing them.
> > >
> > > Yes, but this change is critical for the current design of the driver.
> >
> > Please fix it once and for all by doing proper reload sequence.
> > I warned you that setting NULLs to pointers hide bugs.
> > https://lore.kernel.org/linux-rdma/20250114112555.GG3146852@unreal/
>
> Yes, I understand. We will work on the suggestion that you had given
> based on the new design mentioned in last comment.
Hi Leon,
 Is it okay to merge this change along with the other fixes in the
series? Its important
for the current driver design.

Thanks,

> >
> > Thanks
> >
> > > >
> > > > >
> > > > > The driver specific ops, ulp_irq_stop and ulp_irq_start are required.
> > > > > Broadcom Ethernet and RDMA drivers are designed like that to manage
> > > > > IRQs between them.
> > > > >
> > > > > Hope this clarifies your question.
> > > > > >
> > > > > > >
> > > > > > > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> > > > > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > > > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > > > > > ---
> > > > > > >  drivers/infiniband/hw/bnxt_re/main.c | 5 +++++
> > > > > > >  1 file changed, 5 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > > > > > > index c4c3d67..89ac5c2 100644
> > > > > > > --- a/drivers/infiniband/hw/bnxt_re/main.c
> > > > > > > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > > > > > > @@ -438,6 +438,8 @@ static void bnxt_re_stop_irq(void *handle, bool reset)
> > > > > > >       int indx;
> > > > > > >
> > > > > > >       rdev = en_info->rdev;
> > > > > > > +     if (!rdev)
> > > > > > > +             return;
> > > > > >
> > > > > > This can be seen as an example why I'm so negative about assigning NULL
> > > > > > to the pointers after object is destroyed. Such assignment makes layer
> > > > > > violation much easier job to do.
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >       rcfw = &rdev->rcfw;
> > > > > > >
> > > > > > >       if (reset) {
> > > > > > > @@ -466,6 +468,8 @@ static void bnxt_re_start_irq(void *handle, struct bnxt_msix_entry *ent)
> > > > > > >       int indx, rc;
> > > > > > >
> > > > > > >       rdev = en_info->rdev;
> > > > > > > +     if (!rdev)
> > > > > > > +             return;
> > > > > > >       msix_ent = rdev->nqr->msix_entries;
> > > > > > >       rcfw = &rdev->rcfw;
> > > > > > >       if (!ent) {
> > > > > > > @@ -2438,6 +2442,7 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
> > > > > > >       ibdev_info(&rdev->ibdev, "%s: L2 driver notified to stop en_state 0x%lx",
> > > > > > >                  __func__, en_dev->en_state);
> > > > > > >       bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
> > > > > > > +     bnxt_re_update_en_info_rdev(NULL, en_info, adev);
> > > > > > >       mutex_unlock(&bnxt_re_mutex);
> > > > > > >
> > > > > > >       return 0;
> > > > > > > --
> > > > > > > 2.5.5
> > > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Kalesh AP
> > > >
> > > >
> > >
> > >
> > > --
> > > Regards,
> > > Kalesh AP
> >
> >
>
>
> --
> Regards,
> Kalesh AP

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4224 bytes --]

Re: [PATCH rdma-rc 2/4] RDMA/bnxt_re: Add sanity checks on rdev validity

[Leon Romanovsky]

On Sun, Feb 9, 2025, at 19:18, Selvin Xavier wrote:
> On Wed, Feb 5, 2025 at 9:45 PM Kalesh Anakkur Purayil
> <kalesh-anakkur.purayil@broadcom.com> wrote:
>>
>> Hi Leon,
>>
>> On Wed, Feb 5, 2025 at 3:22 PM Leon Romanovsky <leon@kernel.org> wrote:
>> >
>> > On Wed, Feb 05, 2025 at 01:54:14PM +0530, Kalesh Anakkur Purayil wrote:
>> > > Hi Leon,
>> > >
>> > > On Wed, Feb 5, 2025 at 12:47 PM Leon Romanovsky <leon@kernel.org> wrote:
>> > > >
>> > > > On Tue, Feb 04, 2025 at 10:10:38PM +0530, Kalesh Anakkur Purayil wrote:
>> > > > > Hi Leon,
>> > > > >
>> > > > > On Tue, Feb 4, 2025 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
>> > > > > >
>> > > > > > On Tue, Feb 04, 2025 at 12:21:23AM -0800, Selvin Xavier wrote:
>> > > > > > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>> > > > > > >
>> > > > > > > There is a possibility that ulp_irq_stop and ulp_irq_start
>> > > > > > > callbacks will be called when the device is in detached state.
>> > > > > > > This can cause a crash due to NULL pointer dereference as
>> > > > > > > the rdev is already freed.
>> > > > > >
>> > > > > > Description and code doesn't match. If "possibility" exists, there is
>> > > > > > no protection from concurrent detach and ulp_irq_stop. If there is such
>> > > > > > protection, they can't race.
>> > > > > >
>> > > > > > The main idea of auxiliary bus is to remove the need to implement driver
>> > > > > > specific ops.
>> > > > >
>> > > > > There is no race condition here.
>> > > > >
>> > > > > Let me explain the scenario.
>> > > > >
>> > > > > User is doing a devlink reload reinit. As part of that, the Ethernet
>> > > > > driver first invokes the auxiliary bus suspend callback. The RDMA driver
>> > > > > will do the unwinding operation and hence rdev will be freed.
>> > > > >
>> > > > > After that, during the devlink sequence, Ethernet driver invokes the
>> > > > > ulp_irq_stop() callback and this resulted in the NULL pointer
>> > > > > dereference as the RDMA driver is in detached state and the rdev is
>> > > > > already freed.
>> > > >
>> > > > Shouldn't devlink reload completely release all auxiliary drivers?
>> > > > Why are you keeping BNXT RDMA driver during reload?
>> > >
>> > > That is the current design. BNXT core Ethernet driver will not destroy
>> > > the auxiliary device for RDMA, but just calls the suspend callback. As
>> > > a result, RDMA driver will remains loaded and remains registered with
>> > > the Ethernet driver instance.
>> >
>> > This is wrong.
>>
>> We understand that. BNXT core driver team has already started working
>> on the auxiliary device removal instead of invoking auxdrv->suspend
>> callback in the devlink relaod path. That will avoid these NULL checks
>> in RDMA driver. for time being we need these NULL checks.
>> That will be posted to net-next tree once internal testing and review is done.
>> >
>> > > > BNXT core driver controls reload, it shouldn't call to drivers which
>> > > > doesn't exist.
>> > > Since the RDMA driver instance is registered with Ethernet driver,
>> > > core Ethernet driver invokes the callback.
>> > > >
>> > > > >
>> > > > > We are trying to address the NULL pointer dereference issue here.
>> > > >
>> > > > You are hiding bugs and not fixing them.
>> > >
>> > > Yes, but this change is critical for the current design of the driver.
>> >
>> > Please fix it once and for all by doing proper reload sequence.
>> > I warned you that setting NULLs to pointers hide bugs.
>> > https://lore.kernel.org/linux-rdma/20250114112555.GG3146852@unreal/
>>
>> Yes, I understand. We will work on the suggestion that you had given
>> based on the new design mentioned in last comment.
> Hi Leon,
>  Is it okay to merge this change along with the other fixes in the
> series? Its important
> for the current driver design.

Yes, because it is current situation,  i will merge it tomorrow.

Thanks 

>
> Thanks,
>
>> >
>> > Thanks
>> >
>> > > >
>> > > > >
>> > > > > The driver specific ops, ulp_irq_stop and ulp_irq_start are required.
>> > > > > Broadcom Ethernet and RDMA drivers are designed like that to manage
>> > > > > IRQs between them.
>> > > > >
>> > > > > Hope this clarifies your question.
>> > > > > >
>> > > > > > >
>> > > > > > > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
>> > > > > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>> > > > > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
>> > > > > > > ---
>> > > > > > >  drivers/infiniband/hw/bnxt_re/main.c | 5 +++++
>> > > > > > >  1 file changed, 5 insertions(+)
>> > > > > > >
>> > > > > > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
>> > > > > > > index c4c3d67..89ac5c2 100644
>> > > > > > > --- a/drivers/infiniband/hw/bnxt_re/main.c
>> > > > > > > +++ b/drivers/infiniband/hw/bnxt_re/main.c
>> > > > > > > @@ -438,6 +438,8 @@ static void bnxt_re_stop_irq(void *handle, bool reset)
>> > > > > > >       int indx;
>> > > > > > >
>> > > > > > >       rdev = en_info->rdev;
>> > > > > > > +     if (!rdev)
>> > > > > > > +             return;
>> > > > > >
>> > > > > > This can be seen as an example why I'm so negative about assigning NULL
>> > > > > > to the pointers after object is destroyed. Such assignment makes layer
>> > > > > > violation much easier job to do.
>> > > > > >
>> > > > > > Thanks
>> > > > > >
>> > > > > > >       rcfw = &rdev->rcfw;
>> > > > > > >
>> > > > > > >       if (reset) {
>> > > > > > > @@ -466,6 +468,8 @@ static void bnxt_re_start_irq(void *handle, struct bnxt_msix_entry *ent)
>> > > > > > >       int indx, rc;
>> > > > > > >
>> > > > > > >       rdev = en_info->rdev;
>> > > > > > > +     if (!rdev)
>> > > > > > > +             return;
>> > > > > > >       msix_ent = rdev->nqr->msix_entries;
>> > > > > > >       rcfw = &rdev->rcfw;
>> > > > > > >       if (!ent) {
>> > > > > > > @@ -2438,6 +2442,7 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
>> > > > > > >       ibdev_info(&rdev->ibdev, "%s: L2 driver notified to stop en_state 0x%lx",
>> > > > > > >                  __func__, en_dev->en_state);
>> > > > > > >       bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
>> > > > > > > +     bnxt_re_update_en_info_rdev(NULL, en_info, adev);
>> > > > > > >       mutex_unlock(&bnxt_re_mutex);
>> > > > > > >
>> > > > > > >       return 0;
>> > > > > > > --
>> > > > > > > 2.5.5
>> > > > > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > --
>> > > > > Regards,
>> > > > > Kalesh AP
>> > > >
>> > > >
>> > >
>> > >
>> > > --
>> > > Regards,
>> > > Kalesh AP
>> >
>> >
>>
>>
>> --
>> Regards,
>> Kalesh AP
>
> Attachments:
> * smime.p7s

Re: [PATCH rdma-rc 0/4] RDMA/bnxt_re: Bug fixes for 6.14

[Leon Romanovsky]

On Tue, 04 Feb 2025 00:21:21 -0800, Selvin Xavier wrote:
> Includes some of the critical fixes found while testing
> the 6.14 branch. Please review and apply.
> 
> Thanks,
> Selvin
> 
> Kalesh AP (3):
>   RDMA/bnxt_re: Fix an issue in bnxt_re_async_notifier
>   RDMA/bnxt_re: Add sanity checks on rdev validity
>   RDMA/bnxt_re: Fix issue in the unload path
> 
> [...]

Applied, thanks!

[1/4] RDMA/bnxt_re: Fix an issue in bnxt_re_async_notifier
      https://git.kernel.org/rdma/rdma/c/a27c6f46dcec8f
[2/4] RDMA/bnxt_re: Add sanity checks on rdev validity
      https://git.kernel.org/rdma/rdma/c/f0df225d12fcb0
[3/4] RDMA/bnxt_re: Fix issue in the unload path
      https://git.kernel.org/rdma/rdma/c/e2f105277411c4
[4/4] RDMA/bnxt_re: Fix the statistics for Gen P7 VF
      https://git.kernel.org/rdma/rdma/c/8238c7bd84209c

Best regards,
-- 
Leon Romanovsky <leon@kernel.org>

Re: [PATCH rdma-rc 3/4] RDMA/bnxt_re: Fix issue in the unload path

[Jason Gunthorpe]

On Tue, Feb 04, 2025 at 12:21:24AM -0800, Selvin Xavier wrote:
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> 
> The cited comment removed the netdev notifier register call
> from the driver. But, it did not remove the cleanup code from
> the unload path. As a result, driver unload is not clean and
> resulted in undesired behaviour.

Please don't use vauge generic commit titles:

 RDMA/bnxt_re: Fix issue in the unload path

Be specific and exact, there are several cases like this in this
series and I'm actively wondering if Linus will object to this for rc
patches.

 RDMA/bnxt_re: Fix missed call to ib_unregister_device()

 Since rdev->nb.notifier_call is always NULL the actual work in 
 bnxt_re_remove_device() was skipped

Jason

Re: [PATCH rdma-rc 3/4] RDMA/bnxt_re: Fix issue in the unload path

[Selvin Xavier]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

On Wed, Feb 26, 2025 at 12:13 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Tue, Feb 04, 2025 at 12:21:24AM -0800, Selvin Xavier wrote:
> > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> >
> > The cited comment removed the netdev notifier register call
> > from the driver. But, it did not remove the cleanup code from
> > the unload path. As a result, driver unload is not clean and
> > resulted in undesired behaviour.
>
> Please don't use vauge generic commit titles:
>
>  RDMA/bnxt_re: Fix issue in the unload path
>
> Be specific and exact, there are several cases like this in this
> series and I'm actively wondering if Linus will object to this for rc
> patches.
>
>  RDMA/bnxt_re: Fix missed call to ib_unregister_device()
>
>  Since rdev->nb.notifier_call is always NULL the actual work in
>  bnxt_re_remove_device() was skipped

Sure. will try to be  specific in the commit messages in future.  thanks
>
> Jason

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4224 bytes --]