Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: brcmfmac: NULL pointer dereference during brcmf_detach() after
 firmware crash
To:     =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>
Cc:     "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" 
        <brcm80211-dev-list.pdl@broadcom.com>,
        "open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" 
        <brcm80211-dev-list.pdl@broadcom.com>,
        brcm80211-dev-list@cypress.com, Aaron Blair <aaron@aaronpb.me>
References: <b519e746-ddfd-421f-d897-7620d229e4b2@gmail.com>
 <16ea722f-e08d-044f-216c-4ea745cc6344@broadcom.com>
 <CACna6rxR4t1MpniasnUfyiuBhc9tTkJRx=O7GrMnmVPOXCZ-dQ@mail.gmail.com>
 <CACna6rwF7rai_PMY3VtYXD=uBAT+jHsr8zrWP28vKjUx2S7Uyg@mail.gmail.com>
From:   Arend Van Spriel <arend.vanspriel@broadcom.com>
Message-ID: <e967a7b3-a8b5-5a77-31e0-a06515d37383@broadcom.com>
Date:   Thu, 25 Apr 2019 09:17:48 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <CACna6rwF7rai_PMY3VtYXD=uBAT+jHsr8zrWP28vKjUx2S7Uyg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 4/18/2019 1:55 PM, Rafał Miłecki wrote:
> On Fri, 15 Feb 2019 at 07:15, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On Thu, 14 Feb 2019 at 23:37, Arend Van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> On 2/14/2019 11:30 PM, Rafał Miłecki wrote:
>>>> I've just found a well reproducible brcmfmac crash (NULL pointer
>>>> dereference).
>>>>
>>>> Steps:
>>>> 1. Wait for or trigger a FullMAC firmware crash
>>>> 2. Wait for some skb to get queued on a flowring
>>>> 3. Call rmmod brcmfmac
>>>>
>>>> Problem:
>>>> There is a NULL pointer dereference in one of the brcmf_detach() calls.
>>>>
>>>> Explanation:
>>>> brcmf_detach() first frees all "ifp"s and then deletes flowrings. If any
>>>> flowring has a skb it results in calling brcmf_txfinalize() which tries
>>>> to access "ifp" (struct brcmf_if) which is a NULL.
>>>
>>> Hi Rafał,
>>>
>>> Thanks for diving in. That was my suspicion. Does it mean you are
>>> working on a patch or shall I take care of it.
>>
>> It would be nice to have someone more experienced with detaching &
>> rings look at it. Is adding a simple
>> if (ifp)
>> enough? Or should that code get redesigned? Should we e.g. reorder detach order?
> 
> Hi Arend, would you find a moment to look at that crash, please?

Hi Rafał,

Sorry for getting back on this so late. The driver tries to gracefully 
teardown stuff by sending firmware commands to the device. I think that 
makes no sense as that level of communication is not possible once our 
driver .remove() callback is called. So I think upon calling 
brcmf_detach() we should cleanup everything bottom-up. I will rework the 
code and see how that goes.

Thanks,
Arend