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=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Fri, 14 Jan 2022 12:34:01 -0800
From:   Aloka Dixit <alokad@codeaurora.org>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org
Subject: Re: [v13 2/3] mac80211: MBSSID and EMA beacon handling in AP mode
In-Reply-To: <ebb1ddc51e6e0eff436de50cbbddec77d61af495.camel@sipsolutions.net>
References: <20211006040938.9531-1-alokad@codeaurora.org>
 <20211006040938.9531-3-alokad@codeaurora.org>
 <16a03353cee422340c8ac36240b1e088fd45802e.camel@sipsolutions.net>
 <d2c980b72af1488282f18e8b1814b56c@codeaurora.org>
 <ebb1ddc51e6e0eff436de50cbbddec77d61af495.camel@sipsolutions.net>
Message-ID: <661712f782228b735ab65364932bb18e@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2022-01-14 12:12, Johannes Berg wrote:
> Hi!
> 
>> 
>> > This function is called from ieee80211_beacon_get_ap(). That's called
>> > from __ieee80211_beacon_get(), under RCU read lock.
>> >
>> > > +	for (i = 0; i < beacon->mbssid_ies->cnt; i++) {
>> > > +		struct ieee80211_ema_bcns *bcn;
>> > > +
>> > > +		bcn = kzalloc(sizeof(*bcn), GFP_KERNEL);
>> >
>> > Therefore, you really cannot GFP_KERNEL allocate anything. But I really
>> > only saw this because I went back to my comments on v12 where this was
>> > still more obvious.
>> >
>> 
>> Okay, I understand now that it is illegal because GFP_KERNEL is
>> blocking.
> 
> Right.
> 
>> I thought of following:
>> lock rcu -> get mbssid count first -> unlock rcu -> allocate memory.
>> But in that case, will have again: lock -> dereference to get beacon
>> snapshot.
>> Beacon can change in between so new count might be wrong. In general
>> sounds complicated and wrong.
> 
> Indeed. You could make it work (and count changing is highly unlikely!)
> by going back and checking if the count was correct in the critical
> section, and then going back if necessary (i.e. if it was wrong). But 
> if
> you do this, you should do something like this pseudo-code:
> 
> rcu_read_lock();
> repeat:
> calculated_size = calculate_size();
> rcu_read_unlock();
> 
> alloc = kzalloc(calculated_size, GFP_KERNEL);
> // omitting error handling
> 
> rcu_read_lock();
> calculated_size = calculate_size();
> if (ksize(alloc) < calculated_size)
> 	goto repeat;
> ...
> 
> 
> i.e. note the ksize(), since allocations are rounded up. Even if the
> count increased, you might not need a new allocation.
> 
> Also maybe anyway it'd make sense to allocate all of them together as 
> an
> array, rather than individual pointers for each beacon?
> 
> 
>> I read that GFP_ATOMIC should be used sparingly, mainly for interrupt
>> handlers etc.
> 
> I guess once every beacon is still fairly sparingly though :)
> 
> 

Thank you so much for the quick response, will enable the debug flags 
henceforth.

With that, what would be a better way:
(1) Making it work with pseudo code, still using GFP_KERNEL or
(2) Changing to GFP_ATOMIC but otherwise keep the code fairly similar to 
v13 (preferably allocating an array instead of separate pointers as you 
suggested)?