DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 30FEA205F4
Message-ID: <070e08cfd9e3b778f2012ae8f7532a7c6f5d2fff.camel@sipsolutions.net>
Subject: Re: Should we check netif_running in cfg80211_calculate_bi_data?
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Ben Greear <greearb@candelatech.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Date:   Tue, 23 Oct 2018 22:26:25 +0200
In-Reply-To: <52037f15-c48f-52fd-255b-dbd6f094ce17@candelatech.com>
References: <8c876445-a065-ddb2-4dcf-e922f22a1b7d@candelatech.com>
         <7ac1d7a5f25ebcd44107c3a19e0d8140f37227c6.camel@sipsolutions.net>
         <5e048867-0294-4218-6b2b-bcbcad5be763@candelatech.com>
         <5d6d273615166550749ae68b6eb2ab37f8265b06.camel@sipsolutions.net>
         <52037f15-c48f-52fd-255b-dbd6f094ce17@candelatech.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Tue, 2018-10-23 at 13:22 -0700, Ben Greear wrote:
> On 10/23/2018 12:53 PM, Johannes Berg wrote:
> > On Tue, 2018-10-23 at 12:19 -0700, Ben Greear wrote:
> > > 
> > > Oct 23 12:11:05 ben-ota-2.candelatech.com kernel: Assigning beacon-in-gcd to: 240 from wdev: vap39
> > > Oct 23 12:11:05 ben-ota-2.candelatech.com kernel: beacon-int-diff, beacon-int-gcd: 240  new-beacon-int: 100
> > 
> > This new-beacon-int 100 seems strange and suspicious. Why is it even
> > trying to look at this? Hmm.
> > 
> > > Maybe we need to clear beacon-interval back to 0 on admin down of the wifi dev?
> > 
> > We should be doing this, we should end up in __cfg80211_stop_ap() and
> > that does clear it? Hmm... perhaps this _fails_ somehow, and we don't
> > clear it in the error path? I suppose we really should make that
> > unconditional because there's nothing we can do to recover from that
> > error ...
> 
> I am suspicious about this...  that is not the same memory location as wdev->beacon_interval,
> so maybe this is the thing that is not properly cleared?
> 
> if (sdata->vif.type == NL80211_IFTYPE_AP ||
> 	    sdata->vif.type == NL80211_IFTYPE_MESH_POINT) {
> 		/*
> 		 * always passing this is harmless, since it'll be the
> 		 * same value that cfg80211 finds if it finds the same
> 		 * interface ... and that's always allowed
> 		 */
> 		params.new_beacon_int = sdata->vif.bss_conf.beacon_int;
> 	}

Oh, this is mac80211. Yes, that has a different place, and that does
indeed look like it doesn't get cleared? Odd. Perhaps you can try what
happens if you just clear that in ieee80211_stop_ap()?

johannes