Re: [PATCH net v2 1/2] bnge: fix initial HWRM sequence

[Vikas Gupta]

> > @@ -73,25 +73,35 @@ static int bnge_func_qcaps(struct bnge_dev *bd)
>
> [ ... ]
>
> >       return 0;
> > +
> > +err_free_ctx_mem:
> > +     bnge_free_ctx_mem(bd);
> > +     return rc;
> >  }
>
> Could this error path leave the firmware with active DMA pointers to freed
> memory?
>
> Looking at bnge_alloc_ctx_mem(), it configures the firmware with host DMA
> addresses for the backing store via bnge_backing_store_cfg(). If a
> subsequent step like bnge_hwrm_func_resc_qcaps() fails, this new error path
> directly frees the context memory.

HW does not access the context memory unless rings are active (traffic
is running).
Therefore, It is safe to free the memory on the host.

>
> Previously, errors in this sequence triggered bnge_fw_unregister_dev(),
> which safely unregistered the firmware before freeing the memory. There is
> even a comment in bnge_fw_unregister_dev() documenting this requirement:

bnge_fw_unregister_dev() is called in both success and failure scenarios.
Therefore, free context memory using bnge_free_ctx_mem() after
bnge_hwrm_func_drv_unrgtr() ensures that the context memory is invalidated
at the hardware level.

>
> static void bnge_fw_unregister_dev(struct bnge_dev *bd)
> {
>         /* ctx mem free after unrgtr only */
>         bnge_hwrm_func_drv_unrgtr(bd);
>         bnge_free_ctx_mem(bd);
> }
>
> Since the firmware isn't told to stop using the memory before it is freed
> here, is there a risk the hardware might DMA into the freed host pages?

There is no communication mechanism to inform the firmware to free the
context memory.

>
> > @@ -132,32 +142,25 @@ static int bnge_fw_register_dev(struct bnge_dev *bd)
>
> [ ... ]
>
> > -err_func_unrgtr:
> > -     bnge_fw_unregister_dev(bd);
> > +err_free_ctx_mem:
> > +     bnge_free_ctx_mem(bd);
> >       return rc;
> >  }
>
> This error path appears to have the same regression. If
> bnge_hwrm_func_drv_rgtr() fails after bnge_func_qcaps() has already
> configured the backing store, freeing the context memory directly without
> unregistering might allow the hardware to access freed memory.

Even if bnge_hwrm_func_drv_rgtr() fails, it is still safe to free the context
memory at the host because the driver unloads from this point.

AI reviews appear to ignore logic related to handling context memory
in the patch.
I see no valid comments on the patch.

Thanks,
Vikas

> --
> pw-bot: cr

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature