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

[Leon Romanovsky <leon@kernel.org>]

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