Re: [PATCH V2 1/2] ath6kl: Fix race in aggregation reorder logic

[Kalle Valo <kvalo@qca.qualcomm.com>]

On 05/25/2012 01:19 PM, Vasanthakumar Thiagarajan wrote:
> There are many places where tid data are accessed without
> the lock (rxtid->lock), this can lead to a race condition
> when the timeout handler for aggregatin reorder and the
> receive function are getting executed at the same time.
> Fix this race, but still there are races which can not
> be fixed without rewriting the whole aggregation reorder
> logic, for now fix the obvious ones.
> 
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>

[...]

> @@ -1188,6 +1189,7 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid,
>  		rxtid->progress = true;
>  	else
>  		for (idx = 0 ; idx < rxtid->hold_q_sz; idx++) {
> +			spin_lock_bh(&rxtid->lock);
>  			if (rxtid->hold_q[idx].skb) {
>  				/*
>  				 * There is a frame in the queue and no
> @@ -1201,8 +1203,10 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid,
>  					   HZ * (AGGR_RX_TIMEOUT) / 1000));
>  				rxtid->progress = false;
>  				rxtid->timer_mon = true;
> +				spin_unlock_bh(&rxtid->lock);
>  				break;
>  			}
> +			spin_unlock_bh(&rxtid->lock);
>  		}

Here you take and release the lock multiple times inside the loop. Why
not take the lock before the loop?

> @@ -1627,12 +1631,15 @@ static void aggr_timeout(unsigned long arg)
>  
>  		if (rxtid->aggr && rxtid->hold_q) {
>  			for (j = 0; j < rxtid->hold_q_sz; j++) {
> +				spin_lock_bh(&rxtid->lock);
>  				if (rxtid->hold_q[j].skb) {
>  					aggr_conn->timer_scheduled = true;
>  					rxtid->timer_mon = true;
>  					rxtid->progress = false;
> +					spin_unlock_bh(&rxtid->lock);
>  					break;
>  				}
> +				spin_unlock_bh(&rxtid->lock);
>  			}

Same here.

Kalle