[rt2x00-users] [PATCH] rt2x00: Fix for race condition while update beacon (RFC)

[Ivo van Doorn]

Hi,

> The patch "Implement set_tim callback for all drivers" can cause kernel
> oops in rt73usb_write_beacon. The oops is caused by one of the following
> race conditions:
> * In case of two near calls to set_tim: rt2x00lib_beacondone_iter is
> cleaning the beacon skb, whereas rt73usb_write_beacon is still using it.
> * In case of two near updates of beacon: first as the result of set_tim
> and second as the result of a call from an application (e.g. hostapd).
> This patch fixes the race condition by rearranging the update logic and
> guarding rt2x00_intf->beacon->skb with a mutex.

Patch looks good, I'll send it upstream later today.

Thanks,

Ivo

> Signed-off-by: Igor Perminov <igor.perminov at inbox.ru>
> ---
> diff -urN a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c	2009-06-29 21:07:32.000000000 +0400
> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c	2009-07-22 23:41:40.000000000 +0400
> @@ -187,7 +187,6 @@
>  static void rt2x00lib_beacondone_iter(void *data, u8 *mac,
>  				      struct ieee80211_vif *vif)
>  {
> -	struct rt2x00_dev *rt2x00dev = data;
>  	struct rt2x00_intf *intf = vif_to_intf(vif);
>  
>  	if (vif->type != NL80211_IFTYPE_AP &&
> @@ -196,12 +195,6 @@
>  	    vif->type != NL80211_IFTYPE_WDS)
>  		return;
>  
> -	/*
> -	 * Clean up the beacon skb.
> -	 */
> -	rt2x00queue_free_skb(rt2x00dev, intf->beacon->skb);
> -	intf->beacon->skb = NULL;
> -
>  	spin_lock(&intf->lock);
>  	intf->delayed_flags |= DELAYED_UPDATE_BEACON;
>  	spin_unlock(&intf->lock);
> diff -urN a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
> --- a/drivers/net/wireless/rt2x00/rt2x00.h	2009-07-16 23:12:43.000000000 +0400
> +++ b/drivers/net/wireless/rt2x00/rt2x00.h	2009-07-23 00:17:04.000000000 +0400
> @@ -316,6 +316,11 @@
>  	u8 bssid[ETH_ALEN];
>  
>  	/*
> +	 * beacon->skb must be protected with the mutex.
> +	 */
> +	struct mutex beacon_skb_mutex;
> +
> +	/*
>  	 * Entry in the beacon queue which belongs to
>  	 * this interface. Each interface has its own
>  	 * dedicated beacon entry.
> diff -urN a/drivers/net/wireless/rt2x00/rt2x00mac.c b/drivers/net/wireless/rt2x00/rt2x00mac.c
> --- a/drivers/net/wireless/rt2x00/rt2x00mac.c	2009-07-16 23:12:43.000000000 +0400
> +++ b/drivers/net/wireless/rt2x00/rt2x00mac.c	2009-07-23 00:17:25.000000000 +0400
> @@ -273,6 +273,7 @@
>  
>  	spin_lock_init(&intf->lock);
>  	spin_lock_init(&intf->seqlock);
> +	mutex_init(&intf->beacon_skb_mutex);
>  	intf->beacon = entry;
>  
>  	if (conf->type == NL80211_IFTYPE_AP)
> diff -urN a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c	2009-06-10 07:05:27.000000000 +0400
> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c	2009-07-23 00:18:35.000000000 +0400
> @@ -454,14 +454,25 @@
>  	if (unlikely(!intf->beacon))
>  		return -ENOBUFS;
>  
> +	mutex_lock(&intf->beacon_skb_mutex);
> +
> +	/*
> +	 * Clean up the beacon skb.
> +	 */
> +	rt2x00queue_free_skb(rt2x00dev, intf->beacon->skb);
> +	intf->beacon->skb = NULL;
> +
>  	if (!enable_beacon) {
>  		rt2x00dev->ops->lib->kill_tx_queue(rt2x00dev, QID_BEACON);
> +		mutex_unlock(&intf->beacon_skb_mutex);
>  		return 0;
>  	}
>  
>  	intf->beacon->skb = ieee80211_beacon_get(rt2x00dev->hw, vif);
> -	if (!intf->beacon->skb)
> +	if (!intf->beacon->skb) {
> +		mutex_unlock(&intf->beacon_skb_mutex);
>  		return -ENOMEM;
> +	}
>  
>  	/*
>  	 * Copy all TX descriptor information into txdesc,
> @@ -499,6 +510,8 @@
>  	rt2x00dev->ops->lib->write_beacon(intf->beacon);
>  	rt2x00dev->ops->lib->kick_tx_queue(rt2x00dev, QID_BEACON);
>  
> +	mutex_unlock(&intf->beacon_skb_mutex);
> +
>  	return 0;
>  }
>  
> 
> 
> _______________________________________________
> users mailing list
> users at rt2x00.serialmonkey.com
> http://rt2x00.serialmonkey.com/mailman/listinfo/users_rt2x00.serialmonkey.com
>