patches/driver/bes2600/queue-pending-record-lock-bh-danctnix: mirror besser#18 fix #30
+121
@@ -0,0 +1,121 @@
|
||||
From d95453c98e31d7a47bc227aef5d0b426ac9e334b Mon Sep 17 00:00:00 2001
|
||||
From: Markus Fritsche <fritsche.markus@gmail.com>
|
||||
Date: Mon, 18 May 2026 16:58:49 +0200
|
||||
Subject: [PATCH] =?UTF-8?q?bes2600:=20take=20pending=5Frecord=5Flock=20wit?=
|
||||
=?UTF-8?q?h=20=5Fbh()=20to=20fix=20SOFTIRQ-safe=20=E2=86=92=20-unsafe=20i?=
|
||||
=?UTF-8?q?nversion=20(besser#18)?=
|
||||
MIME-Version: 1.0
|
||||
Content-Type: text/plain; charset=UTF-8
|
||||
Content-Transfer-Encoding: 8bit
|
||||
|
||||
PROVE_LOCKING reports:
|
||||
|
||||
WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected
|
||||
kworker/u16:1 is trying to acquire:
|
||||
&hw_priv->tx_loop.pending_record_lock at bes2600_queue_clear+0x80
|
||||
and this task is already holding:
|
||||
&queue->lock at bes2600_queue_clear+0x60
|
||||
|
||||
which would create a new lock dependency:
|
||||
(&queue->lock){+.-.} -> (&hw_priv->tx_loop.pending_record_lock){+.+.}
|
||||
|
||||
but this new dependency connects a SOFTIRQ-irq-safe lock:
|
||||
(&queue->lock){+.-.}
|
||||
... which became SOFTIRQ-irq-safe at:
|
||||
bes2600_tx -> ieee80211_handle_wake_tx_queue -> tasklet_action
|
||||
to a SOFTIRQ-irq-unsafe lock:
|
||||
(&hw_priv->tx_loop.pending_record_lock){+.+.}
|
||||
... which became SOFTIRQ-irq-unsafe at:
|
||||
bes2600_queue_get_skb -> bes2600_join_work -> process_one_work
|
||||
|
||||
queue->lock is taken consistently with spin_lock_bh() at 22 sites;
|
||||
the nested acquisition of pending_record_lock at queue.c:289 (inside
|
||||
the outer queue->lock_bh held at line 285) had it implicitly BH-safe
|
||||
via the outer scope. But pending_record_lock is ALSO taken from
|
||||
non-BH-disabled contexts:
|
||||
|
||||
bes2600_queue_get_skb (queue.c:832) — process context via
|
||||
bes2600_join_work (workqueue), no outer queue->lock held
|
||||
bes2600_tx_loop_item_pending_check (tx_loop.c:112)
|
||||
— TX-loop context, no outer
|
||||
queue->lock held
|
||||
|
||||
When CPU0 holds pending_record_lock from one of those non-BH paths
|
||||
and a softirq fires that wants queue->lock, and CPU1 in softirq has
|
||||
queue->lock and is about to acquire pending_record_lock — classic AB-BA
|
||||
SOFTIRQ deadlock.
|
||||
|
||||
The fix is the conservative one: take pending_record_lock with _bh()
|
||||
at every site that's not already inside a queue->lock_bh-held scope.
|
||||
That makes the lock consistently SOFTIRQ-safe, eliminating the
|
||||
inversion. queue.c:289/295 stays as plain spin_lock because BH is
|
||||
already disabled by the outer queue->lock_bh acquired at queue.c:285.
|
||||
|
||||
Five sites converted:
|
||||
bes2600/queue.c:832 -- spin_lock -> spin_lock_bh
|
||||
bes2600/queue.c:839 -- spin_unlock -> spin_unlock_bh
|
||||
bes2600/queue.c:844 -- spin_unlock -> spin_unlock_bh
|
||||
bes2600/tx_loop.c:112 -- spin_lock -> spin_lock_bh
|
||||
bes2600/tx_loop.c:114 -- spin_unlock -> spin_unlock_bh
|
||||
|
||||
Contract:
|
||||
- Documentation/locking/locktypes.rst spelling: spin_lock_bh() is
|
||||
the canonical way to make a non-IRQ spinlock safe against
|
||||
softirq preemption that might re-enter the same lock.
|
||||
- Same shape as queue->lock in this driver and as is_drv->lock
|
||||
in the cw1200 ancestor.
|
||||
|
||||
Closes: besser#18
|
||||
Fixes: <bes2600 base import>
|
||||
Signed-off-by: Markus Fritsche <fritsche.markus@gmail.com>
|
||||
---
|
||||
bes2600/queue.c | 6 +++---
|
||||
bes2600/tx_loop.c | 4 ++--
|
||||
2 files changed, 5 insertions(+), 5 deletions(-)
|
||||
|
||||
diff --git a/drivers/staging/bes2600/queue.c b/drivers/staging/bes2600/queue.c
|
||||
index cc606c1..4016b76 100644
|
||||
--- a/drivers/staging/bes2600/queue.c
|
||||
+++ b/drivers/staging/bes2600/queue.c
|
||||
@@ -829,19 +829,19 @@ int bes2600_queue_get_skb(struct bes2600_queue *queue, u32 packetID,
|
||||
bes2600_queue_parse_id(packetID, &queue_generation, &queue_id,
|
||||
&item_generation, &item_id, &if_id, &link_id);
|
||||
|
||||
- spin_lock(&queue->stats->hw_priv->tx_loop.pending_record_lock);
|
||||
+ spin_lock_bh(&queue->stats->hw_priv->tx_loop.pending_record_lock);
|
||||
if (!list_empty(&queue->stats->hw_priv->tx_loop.pending_record_list)) {
|
||||
list_for_each_entry_safe(record_item, temp_record_item, &queue->stats->hw_priv->tx_loop.pending_record_list, head) {
|
||||
if (record_item->packetID == packetID) {
|
||||
list_del(&record_item->head);
|
||||
dev_kfree_skb(record_item->skb);
|
||||
kfree(record_item);
|
||||
- spin_unlock(&queue->stats->hw_priv->tx_loop.pending_record_lock);
|
||||
+ spin_unlock_bh(&queue->stats->hw_priv->tx_loop.pending_record_lock);
|
||||
return -EINVAL;
|
||||
}
|
||||
}
|
||||
}
|
||||
- spin_unlock(&queue->stats->hw_priv->tx_loop.pending_record_lock);
|
||||
+ spin_unlock_bh(&queue->stats->hw_priv->tx_loop.pending_record_lock);
|
||||
|
||||
item = &queue->pool[item_id];
|
||||
|
||||
diff --git a/drivers/staging/bes2600/tx_loop.c b/drivers/staging/bes2600/tx_loop.c
|
||||
index e6cf072..0cf7ce1 100644
|
||||
--- a/drivers/staging/bes2600/tx_loop.c
|
||||
+++ b/drivers/staging/bes2600/tx_loop.c
|
||||
@@ -109,9 +109,9 @@ void bes2600_tx_loop_set_enable(struct bes2600_common *hw_priv, bool need_warn)
|
||||
bes2600_queue_iterate_pending_packet(&hw_priv->tx_queue[i],
|
||||
bes2600_tx_loop_item_pending_item);
|
||||
}
|
||||
- spin_lock(&hw_priv->tx_loop.pending_record_lock);
|
||||
+ spin_lock_bh(&hw_priv->tx_loop.pending_record_lock);
|
||||
bes2600_queue_iterate_record_pending_packet(hw_priv, bes2600_tx_loop_item_pending_item);
|
||||
- spin_unlock(&hw_priv->tx_loop.pending_record_lock);
|
||||
+ spin_unlock_bh(&hw_priv->tx_loop.pending_record_lock);
|
||||
|
||||
if (atomic_read(&hw_priv->bh_rx) > 0)
|
||||
wake_up(&hw_priv->bh_wq);
|
||||
--
|
||||
2.54.0
|
||||
|
||||
@@ -0,0 +1,19 @@
|
||||
# queue-pending-record-lock-bh-danctnix — close besser#18
|
||||
|
||||
Converts `pending_record_lock` to `spin_lock_bh()` at the 5 sites
|
||||
where it is taken in non-BH-disabled contexts (`bes2600_queue_get_skb`
|
||||
called from `bes2600_join_work`, and `bes2600_tx_loop_item_pending_check`).
|
||||
|
||||
Eliminates the PROVE_LOCKING SOFTIRQ-safe → SOFTIRQ-unsafe warning
|
||||
reported in besser#18: `&queue->lock` (taken with `_bh` everywhere,
|
||||
including the nested acquisition at `queue.c:289` that holds
|
||||
`pending_record_lock` as inner) was registered SOFTIRQ-irq-safe by
|
||||
lockdep, but `pending_record_lock` was sometimes taken without BH
|
||||
disable, creating an AB-BA deadlock window.
|
||||
|
||||
Provenance:
|
||||
- Source-of-truth commit on `marfrit/bes2600-dkms` branch
|
||||
`bes2600/queue-pending-record-lock-bh-fix`, commit `d95453c`.
|
||||
- This file is the same commit's `git format-patch` output with
|
||||
the DKMS-style `bes2600/foo.c` paths rewritten to in-tree
|
||||
`drivers/staging/bes2600/foo.c` paths via sed.
|
||||
Reference in New Issue
Block a user