Re: [PATCH] work-in-progress: double-free after hardware reset due to firmware-crash

[Danny van Heumen <danny@dannyvanheumen.nl>]

Hi Arend,

------- Original Message -------
On Tuesday, June 7th, 2022 at 21:00, Arend van Spriel <arend.vanspriel@broadcom.com> wrote:

> [..]
>
> > > While directly return without invoking clean up process makes
> > > perfect sense for the issue described here, it doesn’t address the
> > > broader issue that sdiodev might hold on to couple stale pointers that
> > > might subsequently be used in somewhere down the path because sdiodev is
> > > not freed. Setting these pointer to NULL after freeing them could help
> > > us to catch such issue which is more catastrophic than a double-free.
> > > The perfect solution of course is to rework the code to free sdiodev
> > > whenever brcmf_sdiod_remove() is invoked but that can not be done in
> > > short-term unfortunately.
>
> > - True.
> > - If the two early returns are appropriate -- I think they are -- so
> > we can leave those in. (Again, I'm unfamiliar with the code-base.)
> > - Setting the pointer to NULL at least has the benefit that behavior
> > (even if bugged) is the same irrespective of memory allocation behavior.
> > - I checked your suggestion for 'sdiodev->sgtable': it is not a
>
> pointer, so setting to NULL will not help. If I read this correctly,
> 'sg_free_table(..)' is already resistant to multiple freeing attempts
> with a test of '.sgl'.
>
> > .. as for the control flow. Sure, rework would be nice, but -- to me
> > at least -- it is not clear if it is really necessary. Maybe I'm
> > mistaken, but there seem to be few entry-points to take into account.
> > The "hardware-reset after firmware-crash"-logic was added later IIUC, so
> > maybe it was an oversight? Regardless, I have updated the patch.
>
> The reset-after-fw-crash was indeed added later. I am wondering is we
> can leave the remove-reprobe dance to the mmc core. Maybe mmc_hw_reset()
> could do the trick, ie. simply call mmc_hw_reset() in
> brcmf_sdio_bus_reset() and be done with it. Maybe opening a can of worms
> here, but I always felt uncertain about calling .remove and .probe
> callbacks directly from the driver itself as it may cause issue like
> this double-free, but also the bus is unaware and I don't know that is a
> bad thing or not.

I am pretty sure you found a can of worms indeed :-)

So, a few things to note:

- do you have a reliable way to test this behavior?
- from my PoV: I have encountered various compositions of firmware and uCode
  for the BCM4345/9 (43456-sdio). Earlier versions may occasionally
  exhibit the crashing-behavior, but not reliably.
- do you want to tackle this as a separate effort, given that there is already
  benefit in merging the earlier patch proposal?

Let me try to give my impression of the code, that I get from my cursory scans
through the brcmfmac code: it seems that the device as a whole (the "root")
gets activated. From what I remember, there seems to be a callback that
triggers subsequent initialization. So, it makes somewhat sense to me if a
hardware-reset could flow (back) into the existing initialization flow.
(Again, don't trust info below, as I have very limited knowledge in this domain.
I'm trying to check the extent to which I can make sense of it.)

Kind regards,
Danny



> Regards,
> Arend
>
> > > Also I forgot that our IT attached a legal footer to all emails sent
>
> to an external party. I am sorry about that to anyone reading my mail
> but there is nothing I can do at the moment.
>
> > > Thanks,
>
> > > - Franky
>
> > I have attached the updated patch. As mentioned before, I will be
>
> running the changes myself.
>
> > Regards,
>
> > Danny
>
> > > > So, returning immediately with the errorcode seemed more
>
> appropriate. Regardless, I have only
>
> > > > incidental knowledge from checking the code just for this
>
> particular problem. In the end my goal
>
> > > > is to have the issues addressed so that I am not forced to reboot
>
> my system to get it back in
>
> > > > working order.
>
> > > > As for your remark about sg-table: I had not considered that, but
>
> if my notes above check out,
>
> > > > maybe this does not need to be treated conditionally at all.
>
> > > > Kind regards,
>
> > > > Danny
>
> > > > > diff --git
>
> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>
> > > > > index ac02244a6fdf..e9bad7197ba9 100644
>
> > > > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>
> > > > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>
> > > > > @@ -802,6 +802,7 @@ static void brcmf_sdiod_freezer_detach(struct
>
> brcmf_sdio_dev *sdiodev)
>
> > > > > if (sdiodev->freezer) {
>
> > > > > WARN_ON(atomic_read(&sdiodev->freezer->freezing));
>
> > > > > kfree(sdiodev->freezer);
>
> > > > > + sdiodev->freezer = NULL;
>
> > > > > }
>
> > > > > }
>
> > > > > @@ -885,7 +886,11 @@ int brcmf_sdiod_remove(struct brcmf_sdio_dev
>
> *sdiodev)
>
> > > > > sdio_disable_func(sdiodev->func1);
>
> > > > > sdio_release_host(sdiodev->func1);
>
> > > > > - sg_free_table(&sdiodev->sgtable);
>
> > > > > + if (sdiodev->sgtable) {
>
> > > > > + sg_free_table(&sdiodev->sgtable);
>
> > > > > + sdiodev->sgtable = NULL;
>
> > > > > + }
>
> > > > > +
>
> > > > > sdiodev->sbwad = 0;
>
> > > > > pm_runtime_allow(sdiodev->func1->card->host->parent);
>
> > > > > As for submitting patch to linux-wireless, please follow the
>
> guideline. [1]
>
> > > > > Thanks,
>
> > > > > - Franky
>
> > > > > [1]
>
> https://www.google.com/url?q=https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches&source=gmail-imap&ust=1654959604000000&usg=AOvVaw1Q6aXVZjiKkrq9qmyYVVDa
>
> > > --
>
> > > This electronic communication and the information and any files
>
> transmitted
>
> > > with it, or attached to it, are confidential and are intended solely for
>
> > > the use of the individual or entity to whom it is addressed and may
>
> contain
>
> > > information that is confidential, legally privileged, protected by
>
> privacy
>
> > > laws, or otherwise restricted from disclosure to anyone else. If you are
>
> > > not the intended recipient or the person responsible for delivering the
>
> > > e-mail to the intended recipient, you are hereby notified that any use,
>
> > > copying, distributing, dissemination, forwarding, printing, or
>
> copying of
>
> > > this e-mail is strictly prohibited. If you received this e-mail in
>
> error,
>
> > > please return the e-mail to the sender, delete it from your
>
> computer, and
>
> > > destroy any printed copy of it.