Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: alokad=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Mon, 28 Jun 2021 10:16:26 -0700
From:   Aloka Dixit <alokad@codeaurora.org>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org, John Crispin <john@phrozen.org>
Subject: Re: [PATCH v10 2/4] mac80211: multiple bssid support in interface
 handling
In-Reply-To: <6dc09b7dcfef37a703e95e264e6ab64738560160.camel@sipsolutions.net>
References: <20210426190534.12667-1-alokad@codeaurora.org>
 <20210426190534.12667-3-alokad@codeaurora.org>
 <6dc09b7dcfef37a703e95e264e6ab64738560160.camel@sipsolutions.net>
Message-ID: <9451f0db49f603d7e9a00d172ab341e2@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2021-06-23 08:57, Johannes Berg wrote:
> On Mon, 2021-04-26 at 12:05 -0700, Aloka Dixit wrote:
>> 

> Hm wait, didn't I see the same code in cfg80211 - maybe cfg80211 should
> just pass the pointer?
>> 
>>  static int ieee80211_del_iface(struct wiphy *wiphy, struct 
>> wireless_dev *wdev)
>>  {
>> +	struct ieee80211_sub_if_data *sdata = 
>> IEEE80211_WDEV_TO_SUB_IF(wdev);
>> +	struct ieee80211_local *local;
>> +	struct ieee80211_vif *vif;
>> +
>> +	if (!sdata)
>> +		return 0;
>> +
>> +	local = sdata->local;
>> +	vif = &sdata->vif;
>> +	if (vif->type == NL80211_IFTYPE_AP &&
>> +	    ieee80211_hw_check(&local->hw, SUPPORTS_MBSSID_AP)) {
>> +		if (vif->mbssid.flags & IEEE80211_VIF_MBSSID_TX) {
>> +			struct ieee80211_sub_if_data *child, *tmpsdata;
>> +
>> +			wiphy_unlock(local->hw.wiphy);
>> +			mutex_lock(&local->iflist_mtx);
> 
> I really don't think you can drop the locking like that in the middle 
> of
> something. That's almost always a recipe for disaster.
> 
>> @@ -375,6 +375,18 @@ static void ieee80211_do_stop(struct 
>> ieee80211_sub_if_data *sdata, bool going_do
>>  	struct cfg80211_chan_def chandef;
>>  	bool cancel_scan;
>>  	struct cfg80211_nan_func *func;
>> +	struct ieee80211_sub_if_data *parent;
>> +
>> +	if (sdata->vif.type == NL80211_IFTYPE_AP &&
>> +	    ieee80211_hw_check(&local->hw, SUPPORTS_MBSSID_AP) &&
>> +	    sdata->vif.mbssid.flags & IEEE80211_VIF_MBSSID_NON_TX) {
>> +		parent = vif_to_sdata(sdata->vif.mbssid.parent);
>> +		if (parent && ieee80211_sdata_running(parent)) {
>> +			wiphy_unlock(local->hw.wiphy);
>> +			dev_close(parent->wdev.netdev);
>> +			wiphy_lock(local->hw.wiphy);
> 
> Same here.
> 
> johannes

Hi Johannes,

Thanks for the comments, will go through all, but need your help with 
this particular one -

I'm not able to come up with a different solution which does not cause a 
deadlock by continuing to hold wiphy_lock() as we discussed last time 
that dev_close() will in turn call into cfg80211.

You had suggested looking at dev_close() for VLAN, that code also 
doesn't lock wiphy_lock hence I did same here but locked 
'local->iflist_mtx'. Dropping both won't be good for sure.

Can you please suggest a better way? I'm really not able to come up with 
one.

Thanks.