linux-kernel - [PATCH] sched_ext: Fix potential deadlock in destroy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20250116115101.549175-1-arighi@nvidia.com>
Date: Thu, 16 Jan 2025 12:51:01 +0100
From: Andrea Righi <arighi@...dia.com>
To: Tejun Heo <tj@...nel.org>,
	David Vernet <void@...ifault.com>,
	Changwoo Min <changwoo@...lia.com>
Cc: linux-kernel@...r.kernel.org
Subject: [PATCH] sched_ext: Fix potential deadlock in destroy_dsq()

When creating and destroying DSQs concurrently, a potential deadlock can
occur due to a circular locking dependency between the locks involved in
the operations:

 - create_dsq():

  rhashtable_bucket --> rq->lock --> dsq->lock

 - destroy_dsq():

  dsq->lock -> rhashtable_bucket

This circular dependency is also shown by the following lockdep splat:

[   85.874899] ======================================================
[   85.881304] WARNING: possible circular locking dependency detected
[   85.887710] 6.13.0-rc7-00043-g619f0b6fad52 #3 Not tainted
[   85.893298] ------------------------------------------------------
[   85.899699] sched_ext_ops_h/2060 is trying to acquire lock:
[   85.905467] ffff000080029838 (rhashtable_bucket){....}-{0:0}, at: destroy_dsq+0x1b4/0x7f8
[   85.913960]
               but task is already holding lock:
[   85.919996] ffff0000a126fed8 (&dsq->lock){-.-.}-{2:2}, at: destroy_dsq+0x6c/0x7f8
[   85.927753]
               which lock already depends on the new lock.
...
               other info that might help us debug this:

[   86.494385] Chain exists of:
                 rhashtable_bucket --> &rq->__lock --> &dsq->lock

[   86.510168]  Possible unsafe locking scenario:

[   86.519784]        CPU0                    CPU1
[   86.526184]        ----                    ----
[   86.532560]   lock(&dsq->lock);
[   86.537487]                                lock(&rq->__lock);
[   86.545154]                                lock(&dsq->lock);
[   86.552680]   lock(rhashtable_bucket);
[   86.558244]
                *** DEADLOCK ***

Fix by avoiding the acquisition of the rhashtable lock while dsq->lock
is held in destroy_dsq().

Fixes: f0e1a0643a59 ("sched_ext: Implement BPF extensible scheduler class")
Signed-off-by: Andrea Righi <arighi@...dia.com>
---
 kernel/sched/ext.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
index 8535b46fa4c3..f1bc7639e730 100644
--- a/kernel/sched/ext.c
+++ b/kernel/sched/ext.c
@@ -4521,9 +4521,6 @@ static void destroy_dsq(u64 dsq_id)
 		goto out_unlock_dsq;
 	}
 
-	if (rhashtable_remove_fast(&dsq_hash, &dsq->hash_node, dsq_hash_params))
-		goto out_unlock_dsq;
-
 	/*
 	 * Mark dead by invalidating ->id to prevent dispatch_enqueue() from
 	 * queueing more tasks. As this function can be called from anywhere,
@@ -4531,6 +4528,19 @@ static void destroy_dsq(u64 dsq_id)
 	 * operations inside scheduler locks.
 	 */
 	dsq->id = SCX_DSQ_INVALID;
+
+	raw_spin_unlock_irqrestore(&dsq->lock, flags);
+
+	/*
+	 * If removing the DSQ from the rhashtable fails, it means that a
+	 * concurrent destroy_dsq() has already removed it. In this case,
+	 * avoid triggering the free via the irq work.
+	 */
+	if (rhashtable_remove_fast(&dsq_hash, &dsq->hash_node, dsq_hash_params))
+		goto out_unlock_rcu;
+
+	raw_spin_lock_irqsave(&dsq->lock, flags);
+
 	llist_add(&dsq->free_node, &dsqs_to_free);
 	irq_work_queue(&free_dsq_irq_work);
 
-- 
2.48.0