Patch "net/sched: Fix mirred deadlock on device recursion" should probably be ported to 5.4, 5.10 and 5.15 LTS.
Hi,
We noticed that the patch 0f022d32c3ec should be probably ported to 6.1 and 6.6 LTS according to the bug introducing commit. Also, it can be applied to the latest version of these two LTS branches without conflicts. Its bug introducing commit is 3bcb846ca4cf. According to our manual analysis, the vulnerability is a deadlock caused by recursive locking of the qdisc lock (`sch->q.lock`) when packets are redirected in a loop (e.g., mirroring or redirecting packets to the same device). This happens because the same qdisc lock is attempted to be acquired multiple times by the same CPU, leading to a deadlock. The commit 3bcb846ca4cf removes the `spin_trylock()` in `net_tx_action()` and replaces it with `spin_lock()`. By doing so, it eliminates the non-blocking lock attempt (`spin_trylock()`), which would fail if the lock was already held, preventing recursive locking. The `spin_lock()` will block (wait) if the lock is already held, allowing for the possibility of the same CPU attempting to acquire the same lock recursively, leading to a deadlock. The patch adds an `owner` field to the `Qdisc` structure to track the CPU that currently owns the qdisc. Before enqueueing a packet to the qdisc, it checks if the current CPU is the owner. If so, it drops the packet to prevent the recursive locking. This effectively prevents the deadlock by ensuring that the same CPU doesn't attempt to acquire the lock recursively.
Sorry, the title is wrong. It should be ported to 6.1 and 6.6 LTS as mentioned in the email contents.
On Tue, Jan 21, 2025 at 11:10 PM Xingyu Li xli399@ucr.edu wrote:
Hi,
We noticed that the patch 0f022d32c3ec should be probably ported to 6.1 and 6.6 LTS according to the bug introducing commit. Also, it can be applied to the latest version of these two LTS branches without conflicts. Its bug introducing commit is 3bcb846ca4cf. According to our manual analysis, the vulnerability is a deadlock caused by recursive locking of the qdisc lock (`sch->q.lock`) when packets are redirected in a loop (e.g., mirroring or redirecting packets to the same device). This happens because the same qdisc lock is attempted to be acquired multiple times by the same CPU, leading to a deadlock. The commit 3bcb846ca4cf removes the `spin_trylock()` in `net_tx_action()` and replaces it with `spin_lock()`. By doing so, it eliminates the non-blocking lock attempt (`spin_trylock()`), which would fail if the lock was already held, preventing recursive locking. The `spin_lock()` will block (wait) if the lock is already held, allowing for the possibility of the same CPU attempting to acquire the same lock recursively, leading to a deadlock. The patch adds an `owner` field to the `Qdisc` structure to track the CPU that currently owns the qdisc. Before enqueueing a packet to the qdisc, it checks if the current CPU is the owner. If so, it drops the packet to prevent the recursive locking. This effectively prevents the deadlock by ensuring that the same CPU doesn't attempt to acquire the lock recursively. -- Yours sincerely, Xingyu
-- Yours sincerely, Xingyu
On Tue, Jan 21, 2025 at 11:10:23PM -0800, Xingyu Li wrote:
Hi,
We noticed that the patch 0f022d32c3ec should be probably ported to 6.1 and 6.6 LTS according to the bug introducing commit. Also, it can be applied to the latest version of these two LTS branches without conflicts. Its bug introducing commit is 3bcb846ca4cf. According to our manual analysis, the vulnerability is a deadlock caused by recursive locking of the qdisc lock (`sch->q.lock`) when packets are redirected in a loop (e.g., mirroring or redirecting packets to the same device). This happens because the same qdisc lock is attempted to be acquired multiple times by the same CPU, leading to a deadlock. The commit 3bcb846ca4cf removes the `spin_trylock()` in `net_tx_action()` and replaces it with `spin_lock()`. By doing so, it eliminates the non-blocking lock attempt (`spin_trylock()`), which would fail if the lock was already held, preventing recursive locking. The `spin_lock()` will block (wait) if the lock is already held, allowing for the possibility of the same CPU attempting to acquire the same lock recursively, leading to a deadlock. The patch adds an `owner` field to the `Qdisc` structure to track the CPU that currently owns the qdisc. Before enqueueing a packet to the qdisc, it checks if the current CPU is the owner. If so, it drops the packet to prevent the recursive locking. This effectively prevents the deadlock by ensuring that the same CPU doesn't attempt to acquire the lock recursively.
This commit also breaks the build on the 6.1.y and 6.6.y branches which perhaps it is why it was not applied there.
thanks,
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Xingyu Li