[RFC,1/2] mac80211: iterate over vif using RCU

Commit Message

Antonio Quartulli Dec. 10, 2013, 4:39 p.m. UTC

From: Antonio Quartulli <[e-mail]>

I need to invoke ieee80211_iter_keys() from a periodic worker in a driver and
therefore I would prefer to get rid of any of locks to avoid problems.
These two patches try to use rcu lock to protect the iteration, but I'd like to
get a feedback before sending this stuff as a patch :-)

Moreover, why do we use list_for_each_entry_safe() is ieee80211_iter_keys() if
the list cannot be altered (pointer to key is not passed to iter() so we should
be sure that nobody is going to invoke list_del())?

Cheers,


In ieee80211_iter_keys the local->interfaces list is
accessed for reading only. RCU can be used instead
of pretending to be under RTNL lock.

This can simplify future users of this function.

Signed-off-by: Antonio Quartulli <[e-mail]>
---
 net/mac80211/key.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Eliad Peller Dec. 10, 2013, 5:05 p.m. UTC | #1

hi,

On Tue, Dec 10, 2013 at 6:39 PM, Antonio Quartulli
<[e-mail]> wrote:
> I need to invoke ieee80211_iter_keys() from a periodic worker in a driver and
> therefore I would prefer to get rid of any of locks to avoid problems.
> These two patches try to use rcu lock to protect the iteration, but I'd like to
> get a feedback before sending this stuff as a patch :-)
>
at least iwlwifi might sleep inside the iterator, so you can't just
convert it to rcu (atomic).

> Moreover, why do we use list_for_each_entry_safe() is ieee80211_iter_keys() if
> the list cannot be altered (pointer to key is not passed to iter() so we should
> be sure that nobody is going to invoke list_del())?

see the documentation of ieee80211_remove_key():
/**
 * ieee80211_remove_key - remove the given key
 * @keyconf: the parameter passed with the set key
 *
 * Remove the given key. If the key was uploaded to the hardware at the
 * time this function is called, it is not deleted in the hardware but
 * instead assumed to have been removed already.
 *
 * Note that due to locking considerations this function can (currently)
 * only be called during key iteration (ieee80211_iter_keys().)
 */

Eliad.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Antonio Quartulli Dec. 10, 2013, 5:08 p.m. UTC | #2

On 10/12/13 18:05, Eliad Peller wrote:
> hi,
> 
> On Tue, Dec 10, 2013 at 6:39 PM, Antonio Quartulli
> <[e-mail]> wrote:
>> I need to invoke ieee80211_iter_keys() from a periodic worker in a driver and
>> therefore I would prefer to get rid of any of locks to avoid problems.
>> These two patches try to use rcu lock to protect the iteration, but I'd like to
>> get a feedback before sending this stuff as a patch :-)
>>
> at least iwlwifi might sleep inside the iterator, so you can't just
> convert it to rcu (atomic).
> 

mh, this "might sleep" could be an issue, ok.

>> Moreover, why do we use list_for_each_entry_safe() is ieee80211_iter_keys() if
>> the list cannot be altered (pointer to key is not passed to iter() so we should
>> be sure that nobody is going to invoke list_del())?
> 
> see the documentation of ieee80211_remove_key():

This function does not exist anymore

Eliad Peller Dec. 10, 2013, 5:23 p.m. UTC | #3

On Tue, Dec 10, 2013 at 7:08 PM, Antonio Quartulli
<[e-mail]> wrote:
>>> Moreover, why do we use list_for_each_entry_safe() is ieee80211_iter_keys() if
>>> the list cannot be altered (pointer to key is not passed to iter() so we should
>>> be sure that nobody is going to invoke list_del())?
>>
>> see the documentation of ieee80211_remove_key():
>
> This function does not exist anymore
>
it does seem to exist here :)
https://git.kernel.org/cgit/linux/kernel/git/linville/wireless-next.git/tree/include/net/mac80211.h

Eliad.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Antonio Quartulli Dec. 10, 2013, 5:34 p.m. UTC | #4

On 10/12/13 18:23, Eliad Peller wrote:
> On Tue, Dec 10, 2013 at 7:08 PM, Antonio Quartulli
> <[e-mail]> wrote:
>>>> Moreover, why do we use list_for_each_entry_safe() is ieee80211_iter_keys() if
>>>> the list cannot be altered (pointer to key is not passed to iter() so we should
>>>> be sure that nobody is going to invoke list_del())?
>>>
>>> see the documentation of ieee80211_remove_key():
>>
>> This function does not exist anymore
>>
> it does seem to exist here :)
> https://git.kernel.org/cgit/linux/kernel/git/linville/wireless-next.git/tree/include/net/mac80211.h
> 

ops you are right. I got confused because I was discussing about that
function some days ago: it is not used by any driver, so it could be
removed.

Therefore it should not really be a problem here.

Patch

diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 3e51dd7..04c885a 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -550,8 +550,6 @@  void ieee80211_iter_keys(struct ieee80211_hw *hw,
 	struct ieee80211_key *key, *tmp;
 	struct ieee80211_sub_if_data *sdata;
 
-	ASSERT_RTNL();
-
 	mutex_lock(&local->key_mtx);
 	if (vif) {
 		sdata = vif_to_sdata(vif);
@@ -560,12 +558,14 @@  void ieee80211_iter_keys(struct ieee80211_hw *hw,
 			     key->sta ? &key->sta->sta : NULL,
 			     &key->conf, iter_data);
 	} else {
-		list_for_each_entry(sdata, &local->interfaces, list)
+		rcu_read_lock();
+		list_for_each_entry_rcu(sdata, &local->interfaces, list)
 			list_for_each_entry_safe(key, tmp,
 						 &sdata->key_list, list)
 				iter(hw, &sdata->vif,
 				     key->sta ? &key->sta->sta : NULL,
 				     &key->conf, iter_data);
+		rcu_read_unlock();
 	}
 	mutex_unlock(&local->key_mtx);
 }