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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Andrey Yurovsky <[email protected]>
---
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
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
Johannes,
On Wed, Jul 8, 2009 at 3:40 AM, Johannes Berg<[email protected]> 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
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 <[email protected]>
> Signed-off-by: Andrey Yurovsky <[email protected]>
Reviewed-by: Johannes Berg <[email protected]>
> ---
> 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);
> }
>
>