[PATCH] mac80211: don't allocate mesh peer under RCU

[PATCH] mac80211: don't allocate mesh peer under RCU

[Thomas Pedersen]

mesh_plink_alloc() was being called with the RCU read lock held, which
triggered warnings because various things inside this function must
sleep. It doesn't make much sense to hold an RCU lock on a pointer we
don't yet have anyway, so do rcu_read_unlock() if allocating and relock
on insertion. This also means there is no need for
cfg80211_notify_new_peer_candidate() to be atomic.

Also balance the RCU lock if we are returning early.

This warning was only visible with CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Reported-by: Bob Copeland <me@bobcopeland.com>
Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 net/mac80211/mesh_plink.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 234fe75..427c87d 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -346,7 +346,7 @@ static struct sta_info *mesh_peer_init(struct ieee80211_sub_if_data *sdata,
 	enum ieee80211_band band = ieee80211_get_sdata_band(sdata);
 	struct ieee80211_supported_band *sband;
 	u32 rates, basic_rates = 0;
-	struct sta_info *sta;
+	struct sta_info *sta = NULL;
 	bool insert = false;
 
 	sband = local->hw.wiphy->bands[band];
@@ -354,18 +354,19 @@ static struct sta_info *mesh_peer_init(struct ieee80211_sub_if_data *sdata,
 
 	sta = sta_info_get(sdata, addr);
 	if (!sta) {
+		rcu_read_unlock();
 		/* Userspace handles peer allocation when security is enabled */
 		if (sdata->u.mesh.security & IEEE80211_MESH_SEC_AUTHED) {
 			cfg80211_notify_new_peer_candidate(sdata->dev, addr,
 							   elems->ie_start,
 							   elems->total_len,
-							   GFP_ATOMIC);
-			return NULL;
+							   GFP_KERNEL);
+			goto lock_out;
 		}
 
 		sta = mesh_plink_alloc(sdata, addr);
 		if (!sta)
-			return NULL;
+			goto lock_out;
 		insert = true;
 	}
 
@@ -397,10 +398,13 @@ static struct sta_info *mesh_peer_init(struct ieee80211_sub_if_data *sdata,
 	rate_control_rate_init(sta);
 	spin_unlock_bh(&sta->lock);
 
-	if (insert && sta_info_insert(sta))
+	if (insert && sta_info_insert_rcu(sta))
 		return NULL;
 
 	return sta;
+lock_out:
+	rcu_read_lock();
+	return sta;
 }
 
 void mesh_neighbour_update(struct ieee80211_sub_if_data *sdata,
-- 
1.7.9.5

Re: [PATCH] mac80211: don't allocate mesh peer under RCU

[Javier Cardona]

On Tue, Oct 23, 2012 at 11:55 AM, Thomas Pedersen <thomas@cozybit.com> wrote:
> mesh_plink_alloc() was being called with the RCU read lock held, which
> triggered warnings because various things inside this function must
> sleep. It doesn't make much sense to hold an RCU lock on a pointer we
> don't yet have anyway, so do rcu_read_unlock() if allocating and relock
> on insertion. This also means there is no need for
> cfg80211_notify_new_peer_candidate() to be atomic.
>
> Also balance the RCU lock if we are returning early.
>
> This warning was only visible with CONFIG_DEBUG_ATOMIC_SLEEP enabled.
>
> Reported-by: Bob Copeland <me@bobcopeland.com>
> Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
Reviewed-by: Javier Cardona <javier@cozybit.com>

Javier

> ---
>  net/mac80211/mesh_plink.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
> index 234fe75..427c87d 100644
> --- a/net/mac80211/mesh_plink.c
> +++ b/net/mac80211/mesh_plink.c
> @@ -346,7 +346,7 @@ static struct sta_info *mesh_peer_init(struct ieee80211_sub_if_data *sdata,
>         enum ieee80211_band band = ieee80211_get_sdata_band(sdata);
>         struct ieee80211_supported_band *sband;
>         u32 rates, basic_rates = 0;
> -       struct sta_info *sta;
> +       struct sta_info *sta = NULL;
>         bool insert = false;
>
>         sband = local->hw.wiphy->bands[band];
> @@ -354,18 +354,19 @@ static struct sta_info *mesh_peer_init(struct ieee80211_sub_if_data *sdata,
>
>         sta = sta_info_get(sdata, addr);
>         if (!sta) {
> +               rcu_read_unlock();
>                 /* Userspace handles peer allocation when security is enabled */
>                 if (sdata->u.mesh.security & IEEE80211_MESH_SEC_AUTHED) {
>                         cfg80211_notify_new_peer_candidate(sdata->dev, addr,
>                                                            elems->ie_start,
>                                                            elems->total_len,
> -                                                          GFP_ATOMIC);
> -                       return NULL;
> +                                                          GFP_KERNEL);
> +                       goto lock_out;
>                 }
>
>                 sta = mesh_plink_alloc(sdata, addr);
>                 if (!sta)
> -                       return NULL;
> +                       goto lock_out;
>                 insert = true;
>         }
>
> @@ -397,10 +398,13 @@ static struct sta_info *mesh_peer_init(struct ieee80211_sub_if_data *sdata,
>         rate_control_rate_init(sta);
>         spin_unlock_bh(&sta->lock);
>
> -       if (insert && sta_info_insert(sta))
> +       if (insert && sta_info_insert_rcu(sta))
>                 return NULL;
>
>         return sta;
> +lock_out:
> +       rcu_read_lock();
> +       return sta;
>  }
>
>  void mesh_neighbour_update(struct ieee80211_sub_if_data *sdata,
> --
> 1.7.9.5
>
> _______________________________________________
> Devel mailing list
> Devel@lists.open80211s.org
> http://lists.open80211s.org/cgi-bin/mailman/listinfo/devel



-- 
Javier Cardona
cozybit Inc.
http://www.cozybit.com

Re: [PATCH] mac80211: don't allocate mesh peer under RCU

[Johannes Berg]

On Tue, 2012-10-23 at 11:55 -0700, Thomas Pedersen wrote:
> mesh_plink_alloc() was being called with the RCU read lock held, which
> triggered warnings because various things inside this function must
> sleep. It doesn't make much sense to hold an RCU lock on a pointer we
> don't yet have anyway, so do rcu_read_unlock() if allocating and relock
> on insertion.

I'm not going to apply this, the locking in mesh_plink.c is *already* a
mess with the spinlock being unlocked in so many places. Adding a random
rcu being possibly dropped in the middle won't help it at all. That may
be safe today, but it's in no way obvious. Dropping a lock in the middle
of a function when it was acquired by the caller is always a *really*
bad idea.

To fix the issue right now, you can simply change GFP_KERNEL to
GFP_ATOMIC in mesh_plink_alloc(). If that's not good enough in the long
term, I'd really like to see code cleanups to make all the locking
easier to follow.

johannes

Re: [PATCH] mac80211: don't allocate mesh peer under RCU

[Thomas Pedersen]

On Tue, Oct 23, 2012 at 12:54 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2012-10-23 at 11:55 -0700, Thomas Pedersen wrote:
>> mesh_plink_alloc() was being called with the RCU read lock held, which
>> triggered warnings because various things inside this function must
>> sleep. It doesn't make much sense to hold an RCU lock on a pointer we
>> don't yet have anyway, so do rcu_read_unlock() if allocating and relock
>> on insertion.
>
> I'm not going to apply this, the locking in mesh_plink.c is *already* a
> mess with the spinlock being unlocked in so many places. Adding a random
> rcu being possibly dropped in the middle won't help it at all. That may
> be safe today, but it's in no way obvious. Dropping a lock in the middle
> of a function when it was acquired by the caller is always a *really*
> bad idea.
>
> To fix the issue right now, you can simply change GFP_KERNEL to
> GFP_ATOMIC in mesh_plink_alloc(). If that's not good enough in the long
> term, I'd really like to see code cleanups to make all the locking
> easier to follow.

That won't really do it though, since sta_info_pre_move_state() needs
to sleep as well

I'll take another look, thanks for reviewing.

Thomas