Skip to content

Commit 8f43470

Browse files
jwrdegoedegregkh
jwrdegoede
authored and
gregkh
committed
staging: rtl8723bs: Fix access-point mode deadlock
Commit 54659ca ("staging: rtl8723bs: remove possible deadlock when disconnect (v2)") split the locking of pxmitpriv->lock vs sleep_q/lock into 2 locks in attempt to fix a lockdep reported issue with the locking order of the sta_hash_lock vs pxmitpriv->lock. But in the end this turned out to not fully solve the sta_hash_lock issue so commit a7ac783 ("staging: rtl8723bs: remove a second possible deadlock") was added to fix this in another way. The original fix was kept as it was still seen as a good thing to have, but now it turns out that it creates a deadlock in access-point mode: [Feb20 23:47] ====================================================== [ +0.074085] WARNING: possible circular locking dependency detected [ +0.074077] 5.16.0-1-amd64 #1 Tainted: G C E [ +0.064710] ------------------------------------------------------ [ +0.074075] ksoftirqd/3/29 is trying to acquire lock: [ +0.060542] ffffb8b30062ab00 (&pxmitpriv->lock){+.-.}-{2:2}, at: rtw_xmit_classifier+0x8a/0x140 [r8723bs] [ +0.114921] but task is already holding lock: [ +0.069908] ffffb8b3007ab704 (&psta->sleep_q.lock){+.-.}-{2:2}, at: wakeup_sta_to_xmit+0x3b/0x300 [r8723bs] [ +0.116976] which lock already depends on the new lock. [ +0.098037] the existing dependency chain (in reverse order) is: [ +0.089704] -> #1 (&psta->sleep_q.lock){+.-.}-{2:2}: [ +0.077232] _raw_spin_lock_bh+0x34/0x40 [ +0.053261] xmitframe_enqueue_for_sleeping_sta+0xc1/0x2f0 [r8723bs] [ +0.082572] rtw_xmit+0x58b/0x940 [r8723bs] [ +0.056528] _rtw_xmit_entry+0xba/0x350 [r8723bs] [ +0.062755] dev_hard_start_xmit+0xf1/0x320 [ +0.056381] sch_direct_xmit+0x9e/0x360 [ +0.052212] __dev_queue_xmit+0xce4/0x1080 [ +0.055334] ip6_finish_output2+0x18f/0x6e0 [ +0.056378] ndisc_send_skb+0x2c8/0x870 [ +0.052209] ndisc_send_ns+0xd3/0x210 [ +0.050130] addrconf_dad_work+0x3df/0x5a0 [ +0.055338] process_one_work+0x274/0x5a0 [ +0.054296] worker_thread+0x52/0x3b0 [ +0.050124] kthread+0x16c/0x1a0 [ +0.044925] ret_from_fork+0x1f/0x30 [ +0.049092] -> #0 (&pxmitpriv->lock){+.-.}-{2:2}: [ +0.074101] __lock_acquire+0x10f5/0x1d80 [ +0.054298] lock_acquire+0xd7/0x300 [ +0.049088] _raw_spin_lock_bh+0x34/0x40 [ +0.053248] rtw_xmit_classifier+0x8a/0x140 [r8723bs] [ +0.066949] rtw_xmitframe_enqueue+0xa/0x20 [r8723bs] [ +0.066946] rtl8723bs_hal_xmitframe_enqueue+0x14/0x50 [r8723bs] [ +0.078386] wakeup_sta_to_xmit+0xa6/0x300 [r8723bs] [ +0.065903] rtw_recv_entry+0xe36/0x1160 [r8723bs] [ +0.063809] rtl8723bs_recv_tasklet+0x349/0x6c0 [r8723bs] [ +0.071093] tasklet_action_common.constprop.0+0xe5/0x110 [ +0.070966] __do_softirq+0x16f/0x50a [ +0.050134] __irq_exit_rcu+0xeb/0x140 [ +0.051172] irq_exit_rcu+0xa/0x20 [ +0.047006] common_interrupt+0xb8/0xd0 [ +0.052214] asm_common_interrupt+0x1e/0x40 [ +0.056381] finish_task_switch.isra.0+0x100/0x3a0 [ +0.063670] __schedule+0x3ad/0xd20 [ +0.048047] schedule+0x4e/0xc0 [ +0.043880] smpboot_thread_fn+0xc4/0x220 [ +0.054298] kthread+0x16c/0x1a0 [ +0.044922] ret_from_fork+0x1f/0x30 [ +0.049088] other info that might help us debug this: [ +0.095950] Possible unsafe locking scenario: [ +0.070952] CPU0 CPU1 [ +0.054282] ---- ---- [ +0.054285] lock(&psta->sleep_q.lock); [ +0.047004] lock(&pxmitpriv->lock); [ +0.074082] lock(&psta->sleep_q.lock); [ +0.077209] lock(&pxmitpriv->lock); [ +0.043873] *** DEADLOCK *** [ +0.070950] 1 lock held by ksoftirqd/3/29: [ +0.049082] #0: ffffb8b3007ab704 (&psta->sleep_q.lock){+.-.}-{2:2}, at: wakeup_sta_to_xmit+0x3b/0x300 [r8723bs] Analysis shows that in hindsight the splitting of the lock was not a good idea, so revert this to fix the access-point mode deadlock. Note this is a straight-forward revert done with git revert, the commented out "/* spin_lock_bh(&psta_bmc->sleep_q.lock); */" lines were part of the code before the reverted changes. Fixes: 54659ca ("staging: rtl8723bs: remove possible deadlock when disconnect (v2)") Cc: stable <stable@vger.kernel.org> Cc: Fabio Aiuto <fabioaiuto83@gmail.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215542 Link: https://lore.kernel.org/r/20220302101637.26542-1-hdegoede@redhat.com Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent fc7f750 commit 8f43470

5 files changed

Lines changed: 33 additions & 24 deletions

File tree

‎drivers/staging/rtl8723bs/core/rtw_mlme_ext.c‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5907,6 +5907,7 @@ u8 chk_bmc_sleepq_hdl(struct adapter *padapter, unsigned char *pbuf)
59075907
struct sta_info *psta_bmc;
59085908
struct list_head *xmitframe_plist, *xmitframe_phead, *tmp;
59095909
struct xmit_frame *pxmitframe = NULL;
5910+
struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
59105911
struct sta_priv *pstapriv = &padapter->stapriv;
59115912

59125913
/* for BC/MC Frames */
@@ -5917,7 +5918,8 @@ u8 chk_bmc_sleepq_hdl(struct adapter *padapter, unsigned char *pbuf)
59175918
if ((pstapriv->tim_bitmap&BIT(0)) && (psta_bmc->sleepq_len > 0)) {
59185919
msleep(10);/* 10ms, ATIM(HIQ) Windows */
59195920

5920-
spin_lock_bh(&psta_bmc->sleep_q.lock);
5921+
/* spin_lock_bh(&psta_bmc->sleep_q.lock); */
5922+
spin_lock_bh(&pxmitpriv->lock);
59215923

59225924
xmitframe_phead = get_list_head(&psta_bmc->sleep_q);
59235925
list_for_each_safe(xmitframe_plist, tmp, xmitframe_phead) {
@@ -5940,7 +5942,8 @@ u8 chk_bmc_sleepq_hdl(struct adapter *padapter, unsigned char *pbuf)
59405942
rtw_hal_xmitframe_enqueue(padapter, pxmitframe);
59415943
}
59425944

5943-
spin_unlock_bh(&psta_bmc->sleep_q.lock);
5945+
/* spin_unlock_bh(&psta_bmc->sleep_q.lock); */
5946+
spin_unlock_bh(&pxmitpriv->lock);
59445947

59455948
/* check hi queue and bmc_sleepq */
59465949
rtw_chk_hi_queue_cmd(padapter);

‎drivers/staging/rtl8723bs/core/rtw_recv.c‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -957,8 +957,10 @@ static signed int validate_recv_ctrl_frame(struct adapter *padapter, union recv_
957957
if ((psta->state&WIFI_SLEEP_STATE) && (pstapriv->sta_dz_bitmap&BIT(psta->aid))) {
958958
struct list_head *xmitframe_plist, *xmitframe_phead;
959959
struct xmit_frame *pxmitframe = NULL;
960+
struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
960961

961-
spin_lock_bh(&psta->sleep_q.lock);
962+
/* spin_lock_bh(&psta->sleep_q.lock); */
963+
spin_lock_bh(&pxmitpriv->lock);
962964

963965
xmitframe_phead = get_list_head(&psta->sleep_q);
964966
xmitframe_plist = get_next(xmitframe_phead);
@@ -989,10 +991,12 @@ static signed int validate_recv_ctrl_frame(struct adapter *padapter, union recv_
989991
update_beacon(padapter, WLAN_EID_TIM, NULL, true);
990992
}
991993

992-
spin_unlock_bh(&psta->sleep_q.lock);
994+
/* spin_unlock_bh(&psta->sleep_q.lock); */
995+
spin_unlock_bh(&pxmitpriv->lock);
993996

994997
} else {
995-
spin_unlock_bh(&psta->sleep_q.lock);
998+
/* spin_unlock_bh(&psta->sleep_q.lock); */
999+
spin_unlock_bh(&pxmitpriv->lock);
9961000

9971001
if (pstapriv->tim_bitmap&BIT(psta->aid)) {
9981002
if (psta->sleepq_len == 0) {

‎drivers/staging/rtl8723bs/core/rtw_sta_mgt.c‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -293,48 +293,46 @@ u32 rtw_free_stainfo(struct adapter *padapter, struct sta_info *psta)
293293

294294
/* list_del_init(&psta->wakeup_list); */
295295

296-
spin_lock_bh(&psta->sleep_q.lock);
296+
spin_lock_bh(&pxmitpriv->lock);
297+
297298
rtw_free_xmitframe_queue(pxmitpriv, &psta->sleep_q);
298299
psta->sleepq_len = 0;
299-
spin_unlock_bh(&psta->sleep_q.lock);
300-
301-
spin_lock_bh(&pxmitpriv->lock);
302300

303301
/* vo */
304-
spin_lock_bh(&pstaxmitpriv->vo_q.sta_pending.lock);
302+
/* spin_lock_bh(&(pxmitpriv->vo_pending.lock)); */
305303
rtw_free_xmitframe_queue(pxmitpriv, &pstaxmitpriv->vo_q.sta_pending);
306304
list_del_init(&(pstaxmitpriv->vo_q.tx_pending));
307305
phwxmit = pxmitpriv->hwxmits;
308306
phwxmit->accnt -= pstaxmitpriv->vo_q.qcnt;
309307
pstaxmitpriv->vo_q.qcnt = 0;
310-
spin_unlock_bh(&pstaxmitpriv->vo_q.sta_pending.lock);
308+
/* spin_unlock_bh(&(pxmitpriv->vo_pending.lock)); */
311309

312310
/* vi */
313-
spin_lock_bh(&pstaxmitpriv->vi_q.sta_pending.lock);
311+
/* spin_lock_bh(&(pxmitpriv->vi_pending.lock)); */
314312
rtw_free_xmitframe_queue(pxmitpriv, &pstaxmitpriv->vi_q.sta_pending);
315313
list_del_init(&(pstaxmitpriv->vi_q.tx_pending));
316314
phwxmit = pxmitpriv->hwxmits+1;
317315
phwxmit->accnt -= pstaxmitpriv->vi_q.qcnt;
318316
pstaxmitpriv->vi_q.qcnt = 0;
319-
spin_unlock_bh(&pstaxmitpriv->vi_q.sta_pending.lock);
317+
/* spin_unlock_bh(&(pxmitpriv->vi_pending.lock)); */
320318

321319
/* be */
322-
spin_lock_bh(&pstaxmitpriv->be_q.sta_pending.lock);
320+
/* spin_lock_bh(&(pxmitpriv->be_pending.lock)); */
323321
rtw_free_xmitframe_queue(pxmitpriv, &pstaxmitpriv->be_q.sta_pending);
324322
list_del_init(&(pstaxmitpriv->be_q.tx_pending));
325323
phwxmit = pxmitpriv->hwxmits+2;
326324
phwxmit->accnt -= pstaxmitpriv->be_q.qcnt;
327325
pstaxmitpriv->be_q.qcnt = 0;
328-
spin_unlock_bh(&pstaxmitpriv->be_q.sta_pending.lock);
326+
/* spin_unlock_bh(&(pxmitpriv->be_pending.lock)); */
329327

330328
/* bk */
331-
spin_lock_bh(&pstaxmitpriv->bk_q.sta_pending.lock);
329+
/* spin_lock_bh(&(pxmitpriv->bk_pending.lock)); */
332330
rtw_free_xmitframe_queue(pxmitpriv, &pstaxmitpriv->bk_q.sta_pending);
333331
list_del_init(&(pstaxmitpriv->bk_q.tx_pending));
334332
phwxmit = pxmitpriv->hwxmits+3;
335333
phwxmit->accnt -= pstaxmitpriv->bk_q.qcnt;
336334
pstaxmitpriv->bk_q.qcnt = 0;
337-
spin_unlock_bh(&pstaxmitpriv->bk_q.sta_pending.lock);
335+
/* spin_unlock_bh(&(pxmitpriv->bk_pending.lock)); */
338336

339337
spin_unlock_bh(&pxmitpriv->lock);
340338

‎drivers/staging/rtl8723bs/core/rtw_xmit.c‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1734,12 +1734,15 @@ void rtw_free_xmitframe_queue(struct xmit_priv *pxmitpriv, struct __queue *pfram
17341734
struct list_head *plist, *phead, *tmp;
17351735
struct xmit_frame *pxmitframe;
17361736

1737+
spin_lock_bh(&pframequeue->lock);
1738+
17371739
phead = get_list_head(pframequeue);
17381740
list_for_each_safe(plist, tmp, phead) {
17391741
pxmitframe = list_entry(plist, struct xmit_frame, list);
17401742

17411743
rtw_free_xmitframe(pxmitpriv, pxmitframe);
17421744
}
1745+
spin_unlock_bh(&pframequeue->lock);
17431746
}
17441747

17451748
s32 rtw_xmitframe_enqueue(struct adapter *padapter, struct xmit_frame *pxmitframe)
@@ -1794,7 +1797,6 @@ s32 rtw_xmit_classifier(struct adapter *padapter, struct xmit_frame *pxmitframe)
17941797
struct sta_info *psta;
17951798
struct tx_servq *ptxservq;
17961799
struct pkt_attrib *pattrib = &pxmitframe->attrib;
1797-
struct xmit_priv *xmit_priv = &padapter->xmitpriv;
17981800
struct hw_xmit *phwxmits = padapter->xmitpriv.hwxmits;
17991801
signed int res = _SUCCESS;
18001802

@@ -1812,14 +1814,12 @@ s32 rtw_xmit_classifier(struct adapter *padapter, struct xmit_frame *pxmitframe)
18121814

18131815
ptxservq = rtw_get_sta_pending(padapter, psta, pattrib->priority, (u8 *)(&ac_index));
18141816

1815-
spin_lock_bh(&xmit_priv->lock);
18161817
if (list_empty(&ptxservq->tx_pending))
18171818
list_add_tail(&ptxservq->tx_pending, get_list_head(phwxmits[ac_index].sta_queue));
18181819

18191820
list_add_tail(&pxmitframe->list, get_list_head(&ptxservq->sta_pending));
18201821
ptxservq->qcnt++;
18211822
phwxmits[ac_index].accnt++;
1822-
spin_unlock_bh(&xmit_priv->lock);
18231823

18241824
exit:
18251825

@@ -2202,10 +2202,11 @@ void wakeup_sta_to_xmit(struct adapter *padapter, struct sta_info *psta)
22022202
struct list_head *xmitframe_plist, *xmitframe_phead, *tmp;
22032203
struct xmit_frame *pxmitframe = NULL;
22042204
struct sta_priv *pstapriv = &padapter->stapriv;
2205+
struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
22052206

22062207
psta_bmc = rtw_get_bcmc_stainfo(padapter);
22072208

2208-
spin_lock_bh(&psta->sleep_q.lock);
2209+
spin_lock_bh(&pxmitpriv->lock);
22092210

22102211
xmitframe_phead = get_list_head(&psta->sleep_q);
22112212
list_for_each_safe(xmitframe_plist, tmp, xmitframe_phead) {
@@ -2306,7 +2307,7 @@ void wakeup_sta_to_xmit(struct adapter *padapter, struct sta_info *psta)
23062307

23072308
_exit:
23082309

2309-
spin_unlock_bh(&psta->sleep_q.lock);
2310+
spin_unlock_bh(&pxmitpriv->lock);
23102311

23112312
if (update_mask)
23122313
update_beacon(padapter, WLAN_EID_TIM, NULL, true);
@@ -2318,8 +2319,9 @@ void xmit_delivery_enabled_frames(struct adapter *padapter, struct sta_info *pst
23182319
struct list_head *xmitframe_plist, *xmitframe_phead, *tmp;
23192320
struct xmit_frame *pxmitframe = NULL;
23202321
struct sta_priv *pstapriv = &padapter->stapriv;
2322+
struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
23212323

2322-
spin_lock_bh(&psta->sleep_q.lock);
2324+
spin_lock_bh(&pxmitpriv->lock);
23232325

23242326
xmitframe_phead = get_list_head(&psta->sleep_q);
23252327
list_for_each_safe(xmitframe_plist, tmp, xmitframe_phead) {
@@ -2372,7 +2374,7 @@ void xmit_delivery_enabled_frames(struct adapter *padapter, struct sta_info *pst
23722374
}
23732375
}
23742376

2375-
spin_unlock_bh(&psta->sleep_q.lock);
2377+
spin_unlock_bh(&pxmitpriv->lock);
23762378
}
23772379

23782380
void enqueue_pending_xmitbuf(struct xmit_priv *pxmitpriv, struct xmit_buf *pxmitbuf)

‎drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -502,7 +502,9 @@ s32 rtl8723bs_hal_xmit(
502502
rtw_issue_addbareq_cmd(padapter, pxmitframe);
503503
}
504504

505+
spin_lock_bh(&pxmitpriv->lock);
505506
err = rtw_xmitframe_enqueue(padapter, pxmitframe);
507+
spin_unlock_bh(&pxmitpriv->lock);
506508
if (err != _SUCCESS) {
507509
rtw_free_xmitframe(pxmitpriv, pxmitframe);
508510

0 commit comments

Comments
 (0)