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@xxxxxxxxxxxxxxxx>
[...]
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
