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