Re: [v13 2/3] mac80211: MBSSID and EMA beacon handling in AP mode

[Aloka Dixit <alokad@codeaurora.org>]

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)?