[PATCH] Assign next hop address to pending mesh frames once the path is resolved.

[PATCH] Assign next hop address to pending mesh frames once the path is resolved.

[Javier Cardona]

Regression.  Frames transmitted when a mesh path was wating to be resolved were
being transmitted with an invalid Receiver Address.

Signed-off-by: Javier Cardona <javier@cozybit.com>
---
 net/mac80211/mesh.h         |    2 +-
 net/mac80211/mesh_pathtbl.c |   18 +++++++++++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/mesh.h b/net/mac80211/mesh.h
index 2a2ed18..1827725 100644
--- a/net/mac80211/mesh.h
+++ b/net/mac80211/mesh.h
@@ -61,7 +61,7 @@ enum mesh_path_flags {
  * 	retry
  * @discovery_retries: number of discovery retries
  * @flags: mesh path flags, as specified on &enum mesh_path_flags
- * @state_lock: mesh pat state lock
+ * @state_lock: mesh path state lock
  *
  *
  * The combination of dst and sdata is unique in the mesh path table. Since the
diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index ae98766..75a13ee 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -55,7 +55,23 @@ static DEFINE_RWLOCK(pathtbl_resize_lock);
  */
 void mesh_path_assign_nexthop(struct mesh_path *mpath, struct sta_info *sta)
 {
-	rcu_assign_pointer(mpath->next_hop, sta);
+	struct sk_buff *skb, *skb_first = NULL;
+	struct ieee80211_hdr *hdr;
+
+	rcu_read_lock();
+	mpath->next_hop = sta;
+
+	while ((skb = skb_dequeue(&mpath->frame_queue)) != skb_first) {
+		if (!skb_first)
+			skb_first = skb;
+		hdr = (struct ieee80211_hdr *) skb->data;
+		memcpy(hdr->addr1, sta->sta.addr, ETH_ALEN);
+		skb_queue_tail(&mpath->frame_queue, skb);
+	}
+	if (skb_first)
+		skb_queue_tail(&mpath->frame_queue, skb_first);
+
+	rcu_read_unlock();
 }
 
 
-- 
1.5.4.3

Re: [PATCH] Assign next hop address to pending mesh frames once the path is resolved.

[Johannes Berg]

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

On Tue, 2009-07-07 at 22:53 -0700, Javier Cardona wrote:
> Regression.  Frames transmitted when a mesh path was wating to be resolved were
> being transmitted with an invalid Receiver Address.

> -	rcu_assign_pointer(mpath->next_hop, sta);
> +	struct sk_buff *skb, *skb_first = NULL;
> +	struct ieee80211_hdr *hdr;
> +
> +	rcu_read_lock();
> +	mpath->next_hop = sta;
> +
> +	while ((skb = skb_dequeue(&mpath->frame_queue)) != skb_first) {
> +		if (!skb_first)
> +			skb_first = skb;
> +		hdr = (struct ieee80211_hdr *) skb->data;
> +		memcpy(hdr->addr1, sta->sta.addr, ETH_ALEN);
> +		skb_queue_tail(&mpath->frame_queue, skb);
> +	}
> +	if (skb_first)
> +		skb_queue_tail(&mpath->frame_queue, skb_first);
> +
> +	rcu_read_unlock();

Since skb queues have a locks, why use rcu too?

Also I think you should probably use a different pattern -- this looks
prone to breakage, maybe something like

	sk_buff_head tmpq;
	unsigned long flags;

	__skb_queue_head_init(&tmpq);

	spin_lock_irqsave(&frame_queue->lock);

	while (skb = __skb_dequeue(&frame_queue)) {
		hdr = (struct ieee80211_hdr *) skb->data;
		memcpy(hdr->addr1, sta->sta.addr, ETH_ALEN);
		__skb_queue_tail(&tmpq, skb);
	}

	skb_queue_splice(&tmpq, frame_queue);
	spin_unlock_irqrestore(&frame_queue->lock);

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Assign next hop address to pending mesh frames once the path is resolved.

[Javier Cardona]

Johannes,

On Wed, Jul 8, 2009 at 3:40 AM, Johannes Berg<johannes@sipsolutions.net> wrote:
> On Tue, 2009-07-07 at 22:53 -0700, Javier Cardona wrote:
>> Regression.  Frames transmitted when a mesh path was wating to be resolved were
>> being transmitted with an invalid Receiver Address.
>
>> -     rcu_assign_pointer(mpath->next_hop, sta);
>> +     struct sk_buff *skb, *skb_first = NULL;
>> +     struct ieee80211_hdr *hdr;
>> +
>> +     rcu_read_lock();
>> +     mpath->next_hop = sta;
>> +
>> +     while ((skb = skb_dequeue(&mpath->frame_queue)) != skb_first) {
>> +             if (!skb_first)
>> +                     skb_first = skb;
>> +             hdr = (struct ieee80211_hdr *) skb->data;
>> +             memcpy(hdr->addr1, sta->sta.addr, ETH_ALEN);
>> +             skb_queue_tail(&mpath->frame_queue, skb);
>> +     }
>> +     if (skb_first)
>> +             skb_queue_tail(&mpath->frame_queue, skb_first);
>> +
>> +     rcu_read_unlock();
>
> Since skb queues have a locks, why use rcu too?

The some mpath members are rcu protected.  I thought I had to extend
the rcu section to cover both mpath->next_hop and mpath->frame_queue.
But now I see that the latter does not need protection, so I'll revert
that to just rcu_assign_pointer(mpath->next_hop, sta);

> Also I think you should probably use a different pattern -- this looks
> prone to breakage, maybe something like
>
>        sk_buff_head tmpq;
>        unsigned long flags;
>
>        __skb_queue_head_init(&tmpq);
>
>        spin_lock_irqsave(&frame_queue->lock);
>
>        while (skb = __skb_dequeue(&frame_queue)) {
>                hdr = (struct ieee80211_hdr *) skb->data;
>                memcpy(hdr->addr1, sta->sta.addr, ETH_ALEN);
>                __skb_queue_tail(&tmpq, skb);
>        }
>
>        skb_queue_splice(&tmpq, frame_queue);
>        spin_unlock_irqrestore(&frame_queue->lock);

Oh, nice, cleaner and less locking. v2 will follow shortly.

Thanks!

Javier

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

[PATCH v2] Assign next hop address to pending mesh frames once the path is resolved.

[Javier Cardona]

Regression.  Frames transmitted when a mesh path was wating to be resolved were
being transmitted with an invalid Receiver Address.

[Changes since v1]

Suggested by Johannes:
 - Improved frame_queue traversal
 - Narower RCU scope

Signed-off-by: Javier Cardona <javier@cozybit.com>
Signed-off-by: Andrey Yurovsky <andrey@cozybit.com>
---
 net/mac80211/mesh_pathtbl.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index ae98766..1981af9 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -55,7 +55,25 @@ static DEFINE_RWLOCK(pathtbl_resize_lock);
  */
 void mesh_path_assign_nexthop(struct mesh_path *mpath, struct sta_info *sta)
 {
+	struct sk_buff *skb;
+	struct ieee80211_hdr *hdr;
+	struct sk_buff_head tmpq;
+	unsigned long flags;
+
 	rcu_assign_pointer(mpath->next_hop, sta);
+
+	__skb_queue_head_init(&tmpq);
+
+	spin_lock_irqsave(&mpath->frame_queue.lock, flags);
+
+	while ((skb = __skb_dequeue(&mpath->frame_queue)) != NULL) {
+		hdr = (struct ieee80211_hdr *) skb->data;
+		memcpy(hdr->addr1, sta->sta.addr, ETH_ALEN);
+		__skb_queue_tail(&tmpq, skb);
+	}
+
+	skb_queue_splice(&tmpq, &mpath->frame_queue);
+	spin_unlock_irqrestore(&mpath->frame_queue.lock, flags);
 }
 
 
-- 
1.5.4.3

Re: [PATCH v2] Assign next hop address to pending mesh frames once the path is resolved.

[Johannes Berg]

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

On Thu, 2009-07-09 at 14:42 -0700, Javier Cardona wrote:
> Regression.  Frames transmitted when a mesh path was wating to be resolved were
> being transmitted with an invalid Receiver Address.
> 
> [Changes since v1]
> 
> Suggested by Johannes:
>  - Improved frame_queue traversal
>  - Narower RCU scope
> 
> Signed-off-by: Javier Cardona <javier@cozybit.com>
> Signed-off-by: Andrey Yurovsky <andrey@cozybit.com>

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

> ---
>  net/mac80211/mesh_pathtbl.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
> index ae98766..1981af9 100644
> --- a/net/mac80211/mesh_pathtbl.c
> +++ b/net/mac80211/mesh_pathtbl.c
> @@ -55,7 +55,25 @@ static DEFINE_RWLOCK(pathtbl_resize_lock);
>   */
>  void mesh_path_assign_nexthop(struct mesh_path *mpath, struct sta_info *sta)
>  {
> +	struct sk_buff *skb;
> +	struct ieee80211_hdr *hdr;
> +	struct sk_buff_head tmpq;
> +	unsigned long flags;
> +
>  	rcu_assign_pointer(mpath->next_hop, sta);
> +
> +	__skb_queue_head_init(&tmpq);
> +
> +	spin_lock_irqsave(&mpath->frame_queue.lock, flags);
> +
> +	while ((skb = __skb_dequeue(&mpath->frame_queue)) != NULL) {
> +		hdr = (struct ieee80211_hdr *) skb->data;
> +		memcpy(hdr->addr1, sta->sta.addr, ETH_ALEN);
> +		__skb_queue_tail(&tmpq, skb);
> +	}
> +
> +	skb_queue_splice(&tmpq, &mpath->frame_queue);
> +	spin_unlock_irqrestore(&mpath->frame_queue.lock, flags);
>  }
>  
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]