From: Antonio Quartulli <[email protected]>
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 <[email protected]>
---
net/mac80211/key.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
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);
}
--
1.8.5.1
From: Antonio Quartulli <[email protected]>
By using RCU it is now possible to iterate over the key list
without holding lock (other than rcu_read).
This allows to simplify ieee80211_iter_keys() so that it can
now run without holding any lock (other than rcu_read)
Signed-off-by: Antonio Quartulli <[email protected]>
---
net/mac80211/key.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 04c885a..e2a1e3c 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -261,7 +261,7 @@ static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
bool defunikey, defmultikey, defmgmtkey;
if (new)
- list_add_tail(&new->list, &sdata->key_list);
+ list_add_tail_rcu(&new->list, &sdata->key_list);
if (sta && pairwise) {
rcu_assign_pointer(sta->ptk, new);
@@ -309,7 +309,7 @@ static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
}
if (old)
- list_del(&old->list);
+ list_del_rcu(&old->list);
}
struct ieee80211_key *ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
@@ -537,6 +537,10 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
mutex_unlock(&sdata->local->key_mtx);
}
+/*
+ * This function is supposed to be used for reading operations only.
+ * Iterations over the key list are performed with RCU_read lock only
+ */
void ieee80211_iter_keys(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
void (*iter)(struct ieee80211_hw *hw,
@@ -547,27 +551,24 @@ void ieee80211_iter_keys(struct ieee80211_hw *hw,
void *iter_data)
{
struct ieee80211_local *local = hw_to_local(hw);
- struct ieee80211_key *key, *tmp;
struct ieee80211_sub_if_data *sdata;
mutex_lock(&local->key_mtx);
+ rcu_read_lock();
if (vif) {
sdata = vif_to_sdata(vif);
- list_for_each_entry_safe(key, tmp, &sdata->key_list, list)
+ list_for_each_entry(key, &sdata->key_list, list)
iter(hw, &sdata->vif,
key->sta ? &key->sta->sta : NULL,
&key->conf, iter_data);
} else {
- rcu_read_lock();
list_for_each_entry_rcu(sdata, &local->interfaces, list)
- list_for_each_entry_safe(key, tmp,
- &sdata->key_list, list)
+ list_for_each_entry_rcu(key, &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);
+ rcu_read_unlock();
}
EXPORT_SYMBOL(ieee80211_iter_keys);
--
1.8.5.1
hi,
On Tue, Dec 10, 2013 at 6:39 PM, Antonio Quartulli
<[email protected]> 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.
On Tue, Dec 10, 2013 at 7:08 PM, Antonio Quartulli
<[email protected]> 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.
On 10/12/13 18:05, Eliad Peller wrote:
> hi,
>
> On Tue, Dec 10, 2013 at 6:39 PM, Antonio Quartulli
> <[email protected]> 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
--
Antonio Quartulli
On 10/12/13 18:23, Eliad Peller wrote:
> On Tue, Dec 10, 2013 at 7:08 PM, Antonio Quartulli
> <[email protected]> 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.
--
Antonio Quartulli