locking in wiphy_apply_custom_regulatory()

locking in wiphy_apply_custom_regulatory()

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

Hi Johannes,

I stumbled upon locking in wiphy_apply_custom_regulatory() and the 
history of commits make it a bit unclear to me if these locks are all 
really needed.

beee246951571 added the rcu assignments without any locking.
51d62f2f2c501 added the rtnl locking.
a05829a7222e9 added the wiphy lock.

My understanding of RCU operations was that locking is only needed for 
readers, but here we use not one but two mutex locks which makes me 
wonder. Especially, as this function is called before registering the wiphy.

Could you clarify?

Thanks,
Arend

---8<---------------------------------------------------------------
	/*
	 * no point in calling this if it won't have any effect
	 * on your device's supported bands.
	 */
	WARN_ON(!bands_set);
	new_regd = reg_copy_regd(regd);
	if (IS_ERR(new_regd))
		return;

	rtnl_lock();
	wiphy_lock(wiphy);

	tmp = get_wiphy_regdom(wiphy);
	rcu_assign_pointer(wiphy->regd, new_regd);
	rcu_free_regdom(tmp);

	wiphy_unlock(wiphy);
	rtnl_unlock();
}
EXPORT_SYMBOL(wiphy_apply_custom_regulatory);

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: locking in wiphy_apply_custom_regulatory()

[Johannes Berg]

Hi Arend,

> I stumbled upon locking in wiphy_apply_custom_regulatory() and the 
> history of commits make it a bit unclear to me if these locks are all 
> really needed.

Oh how I like the regulatory code ... ;)

> beee246951571 added the rcu assignments without any locking.

Well that was just plain wrong, right?

You have to have some kind of writer-side locking with RCU.

I think this probably was assuming we'd be under some locks already?

In regulatory_set_wiphy_regd() vs. regulatory_set_wiphy_regd_sync() we
later formalized this.

> 51d62f2f2c501 added the rtnl locking.

That was the bugfix.

> a05829a7222e9 added the wiphy lock.

Yes that's because now get_wiphy_regdom() can be called as documented:

/*
 * Returns the regulatory domain associated with the wiphy.
 *
 * Requires any of RTNL, wiphy mutex or RCU protection.
 */


> My understanding of RCU operations was that locking is only needed for 
> readers,
> 

No, readers are lockless (just "rcu_read_lock()" which enters a critical
section, but no locking).

>  but here we use not one but two mutex locks which makes me 
> wonder. Especially, as this function is called before registering the wiphy.
> 
> Could you clarify?
> 

Like I said above, that's because we want to dereference the pointer in
three different contexts:
 * RCU
 * RTNL
 * wiphy mutex

Maybe we can get rid of some of those readers, but e.g. iwlwifi is using

	wiphy_dereference(mvm->hw->wiphy, mvm->hw->wiphy->regd);

which really should use get_wiphy_regdom(), but I'm not sure is holding
the RTNL in all paths? Looking at them, it seems like it is, so perhaps
we can get rid of the wiphy mutex locking.


Why are you asking? :)

johannes

Re: locking in wiphy_apply_custom_regulatory()

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]

On 2/11/2022 11:38 AM, Johannes Berg wrote:
> Hi Arend,
> 
>> I stumbled upon locking in wiphy_apply_custom_regulatory() and the
>> history of commits make it a bit unclear to me if these locks are all
>> really needed.
> 
> Oh how I like the regulatory code ... ;)

Thanks for getting in to this in such detail...

>> beee246951571 added the rcu assignments without any locking.
> 
> Well that was just plain wrong, right?
> 
> You have to have some kind of writer-side locking with RCU.
> 
> I think this probably was assuming we'd be under some locks already?
> 
> In regulatory_set_wiphy_regd() vs. regulatory_set_wiphy_regd_sync() we
> later formalized this.

Yeah. That is clear.

>> 51d62f2f2c501 added the rtnl locking.
> 
> That was the bugfix.

Correct. Just not sure I understand why is has to be RTNL lock.

>> a05829a7222e9 added the wiphy lock.
> 
> Yes that's because now get_wiphy_regdom() can be called as documented:
> 
> /*
>   * Returns the regulatory domain associated with the wiphy.
>   *
>   * Requires any of RTNL, wiphy mutex or RCU protection.
>   */

So would wiphy mutex be sufficient. I guess my question is what is 
protected by these lock in wiphy_apply_custom_regulatory() and is it 
really necessary to have both.

>> My understanding of RCU operations was that locking is only needed for
>> readers,
>>
> 
> No, readers are lockless (just "rcu_read_lock()" which enters a critical
> section, but no locking).

Understood. My bad.

>>   but here we use not one but two mutex locks which makes me
>> wonder. Especially, as this function is called before registering the wiphy.
>>
>> Could you clarify?
>>
> 
> Like I said above, that's because we want to dereference the pointer in
> three different contexts:
>   * RCU
>   * RTNL
>   * wiphy mutex
> 
> Maybe we can get rid of some of those readers, but e.g. iwlwifi is using
> 
> 	wiphy_dereference(mvm->hw->wiphy, mvm->hw->wiphy->regd);
> 
> which really should use get_wiphy_regdom(), but I'm not sure is holding
> the RTNL in all paths? Looking at them, it seems like it is, so perhaps
> we can get rid of the wiphy mutex locking.
> 
> 
> Why are you asking? :)

Just some experimental coding where I ended up calling 
wiphy_apply_custom_regulatory() upon IFF_UP and hit deadlock because 
RTNL was already taken. Anyway, that code already ended up in the 
garbage bin, but wanted to ask anyway. Learning by asking (stupid) 
questions ;-)

Regards,
Arend

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: locking in wiphy_apply_custom_regulatory()

[Johannes Berg]

On Mon, 2022-02-14 at 09:45 +0100, Arend van Spriel wrote:
> 
> Correct. Just not sure I understand why is has to be RTNL lock.

Well, does it? Now I forgot again from when I looked ;-)


Surely for nl80211_get_reg_do() for example it has to be, at least
today, though that is already protecting the regdom with RCU in the non-
wiphy case, so doesn't really need to use RTNL.

Similarly, nl80211_get_reg_dump() could use RCU.

So that's easy, but all the interaction e.g. with brcms_reg_notifier()
calling freq_reg_info() which uses it too, but then is called from
wiphy_update_regulatory() from e.g. update_all_wiphy_regulatory() only
with RTNL makes it all complicated ...


> So would wiphy mutex be sufficient. I guess my question is what is 
> protected by these lock in wiphy_apply_custom_regulatory() and is it 
> really necessary to have both.

See above.

Honestly, I wouldn't mind if wiphy mutex (or RCU) _was_ sufficient. But
we're not there, and the regulatory code is sufficiently complex and
called sufficiently infrequently that I just haven't bothered trying to
reduce its reliance on the RTNL.

> Just some experimental coding where I ended up calling 
> wiphy_apply_custom_regulatory() upon IFF_UP and hit deadlock because 
> RTNL was already taken. Anyway, that code already ended up in the 
> garbage bin, but wanted to ask anyway. Learning by asking (stupid) 
> questions ;-)
> 

Given the length and depth I had to go to to answer it, that really
couldn't be a stupid question :)

johannes