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
Date: Fri, 14 Jan 2022 12:34:01 -0800 [thread overview]
Message-ID: <661712f782228b735ab65364932bb18e@codeaurora.org> (raw)
In-Reply-To: <ebb1ddc51e6e0eff436de50cbbddec77d61af495.camel@sipsolutions.net>
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)?
next prev parent reply other threads:[~2022-01-14 20:35 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-06 4:09 [v13 0/3] MBSSID and EMA support in AP mode Aloka Dixit
2021-10-06 4:09 ` [v13 1/3] mac80211: split beacon retrieval functions Aloka Dixit
2021-10-06 20:20 ` Aloka Dixit
2021-10-06 4:09 ` [v13 2/3] mac80211: MBSSID and EMA beacon handling in AP mode Aloka Dixit
2021-11-26 11:23 ` Johannes Berg
2022-01-14 19:23 ` Aloka Dixit
2022-01-14 20:12 ` Johannes Berg
2022-01-14 20:34 ` Aloka Dixit [this message]
2022-01-14 20:50 ` Johannes Berg
2021-10-06 4:09 ` [v13 3/3] mac80211: MBSSID channel switch Aloka Dixit
2021-11-26 11:16 ` Johannes Berg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=661712f782228b735ab65364932bb18e@codeaurora.org \
--to=alokad@codeaurora.org \
--cc=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).