Commit 0faf7458 authored by Vasanthakumar Thiagarajan's avatar Vasanthakumar Thiagarajan Committed by Kalle Valo

ath6kl: Fix race in aggregation reorder logic

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: default avatarVasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
Signed-off-by: default avatarKalle Valo <kvalo@qca.qualcomm.com>
parent d154f32e
...@@ -273,9 +273,15 @@ struct rxtid { ...@@ -273,9 +273,15 @@ struct rxtid {
struct sk_buff_head q; struct sk_buff_head q;
/* /*
* FIXME: No clue what this should protect. Apparently it should * lock mainly protects seq_next and hold_q. Movement of seq_next
* protect some of the fields above but they are also accessed * needs to be protected between aggr_timeout() and
* without taking the lock. * aggr_process_recv_frm(). hold_q will be holding the pending
* reorder frames and it's access should also be protected.
* Some of the other fields like hold_q_sz, win_sz and aggr are
* initialized/reset when receiving addba/delba req, also while
* deleting aggr state all the pending buffers are flushed before
* resetting these fields, so there should not be any race in accessing
* these fields.
*/ */
spinlock_t lock; spinlock_t lock;
}; };
......
...@@ -1036,6 +1036,7 @@ static void aggr_deque_frms(struct aggr_info_conn *agg_conn, u8 tid, ...@@ -1036,6 +1036,7 @@ static void aggr_deque_frms(struct aggr_info_conn *agg_conn, u8 tid,
rxtid = &agg_conn->rx_tid[tid]; rxtid = &agg_conn->rx_tid[tid];
stats = &agg_conn->stat[tid]; stats = &agg_conn->stat[tid];
spin_lock_bh(&rxtid->lock);
idx = AGGR_WIN_IDX(rxtid->seq_next, rxtid->hold_q_sz); idx = AGGR_WIN_IDX(rxtid->seq_next, rxtid->hold_q_sz);
/* /*
...@@ -1054,8 +1055,6 @@ static void aggr_deque_frms(struct aggr_info_conn *agg_conn, u8 tid, ...@@ -1054,8 +1055,6 @@ static void aggr_deque_frms(struct aggr_info_conn *agg_conn, u8 tid,
seq_end = seq_no ? seq_no : rxtid->seq_next; seq_end = seq_no ? seq_no : rxtid->seq_next;
idx_end = AGGR_WIN_IDX(seq_end, rxtid->hold_q_sz); idx_end = AGGR_WIN_IDX(seq_end, rxtid->hold_q_sz);
spin_lock_bh(&rxtid->lock);
do { do {
node = &rxtid->hold_q[idx]; node = &rxtid->hold_q[idx];
if ((order == 1) && (!node->skb)) if ((order == 1) && (!node->skb))
...@@ -1127,11 +1126,13 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid, ...@@ -1127,11 +1126,13 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid,
((end > extended_end) && (cur > extended_end) && ((end > extended_end) && (cur > extended_end) &&
(cur < end))) { (cur < end))) {
aggr_deque_frms(agg_conn, tid, 0, 0); aggr_deque_frms(agg_conn, tid, 0, 0);
spin_lock_bh(&rxtid->lock);
if (cur >= rxtid->hold_q_sz - 1) if (cur >= rxtid->hold_q_sz - 1)
rxtid->seq_next = cur - (rxtid->hold_q_sz - 1); rxtid->seq_next = cur - (rxtid->hold_q_sz - 1);
else else
rxtid->seq_next = ATH6KL_MAX_SEQ_NO - rxtid->seq_next = ATH6KL_MAX_SEQ_NO -
(rxtid->hold_q_sz - 2 - cur); (rxtid->hold_q_sz - 2 - cur);
spin_unlock_bh(&rxtid->lock);
} else { } else {
/* /*
* Dequeue only those frames that are outside the * Dequeue only those frames that are outside the
...@@ -1186,7 +1187,8 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid, ...@@ -1186,7 +1187,8 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid,
if (agg_conn->timer_scheduled) if (agg_conn->timer_scheduled)
rxtid->progress = true; rxtid->progress = true;
else else {
spin_lock_bh(&rxtid->lock);
for (idx = 0 ; idx < rxtid->hold_q_sz; idx++) { for (idx = 0 ; idx < rxtid->hold_q_sz; idx++) {
if (rxtid->hold_q[idx].skb) { if (rxtid->hold_q[idx].skb) {
/* /*
...@@ -1204,6 +1206,8 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid, ...@@ -1204,6 +1206,8 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid,
break; break;
} }
} }
spin_unlock_bh(&rxtid->lock);
}
return is_queued; return is_queued;
} }
...@@ -1626,6 +1630,7 @@ static void aggr_timeout(unsigned long arg) ...@@ -1626,6 +1630,7 @@ static void aggr_timeout(unsigned long arg)
rxtid = &aggr_conn->rx_tid[i]; rxtid = &aggr_conn->rx_tid[i];
if (rxtid->aggr && rxtid->hold_q) { if (rxtid->aggr && rxtid->hold_q) {
spin_lock_bh(&rxtid->lock);
for (j = 0; j < rxtid->hold_q_sz; j++) { for (j = 0; j < rxtid->hold_q_sz; j++) {
if (rxtid->hold_q[j].skb) { if (rxtid->hold_q[j].skb) {
aggr_conn->timer_scheduled = true; aggr_conn->timer_scheduled = true;
...@@ -1634,6 +1639,7 @@ static void aggr_timeout(unsigned long arg) ...@@ -1634,6 +1639,7 @@ static void aggr_timeout(unsigned long arg)
break; break;
} }
} }
spin_unlock_bh(&rxtid->lock);
if (j >= rxtid->hold_q_sz) if (j >= rxtid->hold_q_sz)
rxtid->timer_mon = false; rxtid->timer_mon = false;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment