linux-kernel - Re: [PATCH RFC 1/2] brcmfmac: remove interface before notifying listener

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 18 Jun 2016 21:26:36 +0200
From:	Arend van Spriel <arend.vanspriel@...adcom.com>
To:	Rafał Miłecki <zajec5@...il.com>,
	Kalle Valo <kvalo@...eaurora.org>
Cc:	Franky Lin <franky.lin@...adcom.com>,
	Hante Meuleman <hante.meuleman@...adcom.com>,
	Pieter-Paul Giesberts <pieter-paul.giesberts@...adcom.com>,
	"Franky (Zhenhui) Lin" <frankyl@...adcom.com>,
	"open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" 
	<linux-wireless@...r.kernel.org>,
	"open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" 
	<brcm80211-dev-list.pdl@...adcom.com>,
	"open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC 1/2] brcmfmac: remove interface before notifying
 listener

On 18-06-16 20:18, Rafał Miłecki wrote:
> So far when receiving event about in-firmware-interface removal we were
> notifying our listener and afterwards we were removing Linux interface.
> 

[snip]

> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
> index 9da7a4c..5fd1886 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
> @@ -18,6 +18,7 @@
>  #include "brcmu_wifi.h"
>  #include "brcmu_utils.h"
>  
> +#include "cfg80211.h"
>  #include "core.h"
>  #include "debug.h"
>  #include "tracepoint.h"
> @@ -180,10 +181,16 @@ static void brcmf_fweh_handle_if_event(struct brcmf_pub *drvr,
>  	if (ifp && ifevent->action == BRCMF_E_IF_CHANGE)
>  		brcmf_fws_reset_interface(ifp);
>  
> -	err = brcmf_fweh_call_event_handler(ifp, emsg->event_code, emsg, data);

The reason for doing this first is because we are passing the ifp, which
is netdev_priv(ifp->ndev). In brcmf_remove_interface() we only
unregister the netdev, which will end up (after scheduling) in
brcmf_free_netdev() thus freeing the ifp. By moving the event handler
function ifp may be stale already.

> +	if (ifp && ifevent->action == BRCMF_E_IF_DEL) {
> +		bool rtnl_locked = brcmf_cfg80211_vif_event_armed(drvr->config);
> +
> +		brcmf_remove_interface(ifp, rtnl_locked);

I guess rtnl_locked here means "rtnl_is_locked() by brcmfmac". It
actually does not matter who is holding the rtnl_lock. At least when it
is brcmfmac it is still a different task, ie. hostapd, iw, etc. Also
when brcmf_cfg80211_vif_event_armed() return false there may still be
some task holding the rtnl_lock.

Regards,
Arend