[PATCH 0/2] ublk: don't let one stuck device block control of all ublk devices

[PATCH 0/2] ublk: don't let one stuck device block control of all ublk devices

[Mark Korenberg]

If the filesystem on a ublk device is stuck, removing that device
blocks in del_gendisk(), which syncs the filesystem. DEL_DEV holds the
global ublk_ctl_mutex across the removal, so ADD_DEV and DEL_DEV of
every other ublk device hang too, until reboot. It doesn't matter
whether the server is alive: when it dies, the release work does the
same syncing del_gendisk() under ub->mutex, and DEL_DEV waits for
ub->mutex holding ublk_ctl_mutex.

I hit it with btrfs on a ublk zoned device whose writeback deadlocked
on its own (a btrfs bug, handled separately): killing the server as a
last resort didn't help, and no new ublk device could be created on the
machine afterwards.

Patch 2 removes the global part: DEL_DEV doesn't take ublk_ctl_mutex.
It relies on patch 1, which fixes an older race the mutex partly hid:
ADD_DEV makes a new device findable by its id before its char device is
added, and if adding it fails, a DEL_DEV that found the device removes
a char device that was never added. With a failure injected into
ublk_add_chdev() this oopses on master in kill_device() from
cdev_device_del(); with patch 1, DEL_DEV gets -ENODEV.

Reproducer, without btrfs or zoned: XFS (or ext4) on a ublk loop
device, its external log (journal) on dm-linear over null_blk:

  dmsetup suspend --nolockfs --noflush <log>
  write a file; syncfs                  # blocks, as expected
  kill -9 <ublk server>                 # optional
  DEL_DEV 0                             # directly, as libublk does
  ADD_DEV 1                             # null target
  dmsetup resume <log>                  # everything unwinds

           DEL_DEV 0                         ADD_DEV 1
  master   D, survives SIGKILL               hangs on ublk_ctl_mutex
  series   D, survives SIGKILL (see below)   ok

The stuck device itself still can't be stopped or deleted until its
filesystem moves: its STOP_DEV and DEL_DEV sleep uninterruptibly, in
del_gendisk() or on ub->mutex. When the server is gone, nothing can be
written anyway, so that removal should be a surprise one done without
ub->mutex, but marking the disk dead isn't enough (the filesystem's
->shutdown() and the writeback flush in bdi_unregister() can block the
same way), and doing the removal without ub->mutex races with the
release work, recovery and a new server attaching. I'll send that part
separately as an RFC.

Tested in a VM with lockdep on top of master c339f4010dea, XFS and ext4,
with and without the server killed; block for-next has no ublk_drv.c
changes on top of it. The ublk selftests give the same results with and
without the series (integrity_01/03 fail on both, no
CONFIG_BLK_DEV_INTEGRITY here); a stress of DEL_DEV racing ADD_DEV of
the same id shows no warnings, also with KASAN and KCSAN.

This series was developed with the help of an AI coding assistant
(Claude); I reviewed and tested the changes and take responsibility
for them.

---
Mark Korenberg (2):
      ublk: publish a new device by its id only once it is set up
      ublk: don't take ublk_ctl_mutex in DEL_DEV

 drivers/block/ublk_drv.c | 60 +++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 23 deletions(-)
---
base-commit: c339f4010dea8b0d754f84ace70cd1b089878ef8
change-id: 20261104-ublk-del-gendisk-hang-bb33f56625f5

Best regards,
-- 
Mark Korenberg <socketpair@gmail.com>

[PATCH 0/2] ublk: don't let one stuck device block control of all ublk devices

[Mark Korenberg via B4 Relay]

If the filesystem on a ublk device is stuck, removing that device
blocks in del_gendisk(), which syncs the filesystem. DEL_DEV holds the
global ublk_ctl_mutex across the removal, so ADD_DEV and DEL_DEV of
every other ublk device hang too, until reboot. It doesn't matter
whether the server is alive: when it dies, the release work does the
same syncing del_gendisk() under ub->mutex, and DEL_DEV waits for
ub->mutex holding ublk_ctl_mutex.

I hit it with btrfs on a ublk zoned device whose writeback deadlocked
on its own (a btrfs bug, handled separately): killing the server as a
last resort didn't help, and no new ublk device could be created on the
machine afterwards.

Patch 2 removes the global part: DEL_DEV doesn't take ublk_ctl_mutex.
It relies on patch 1, which fixes an older race the mutex partly hid:
ADD_DEV makes a new device findable by its id before its char device is
added, and if adding it fails, a DEL_DEV that found the device removes
a char device that was never added. With a failure injected into
ublk_add_chdev() this oopses on master in kill_device() from
cdev_device_del(); with patch 1, DEL_DEV gets -ENODEV.

Reproducer, without btrfs or zoned: XFS (or ext4) on a ublk loop
device, its external log (journal) on dm-linear over null_blk:

  dmsetup suspend --nolockfs --noflush <log>
  write a file; syncfs                  # blocks, as expected
  kill -9 <ublk server>                 # optional
  DEL_DEV 0                             # directly, as libublk does
  ADD_DEV 1                             # null target
  dmsetup resume <log>                  # everything unwinds

           DEL_DEV 0                         ADD_DEV 1
  master   D, survives SIGKILL               hangs on ublk_ctl_mutex
  series   D, survives SIGKILL (see below)   ok

The stuck device itself still can't be stopped or deleted until its
filesystem moves: its STOP_DEV and DEL_DEV sleep uninterruptibly, in
del_gendisk() or on ub->mutex. When the server is gone, nothing can be
written anyway, so that removal should be a surprise one done without
ub->mutex, but marking the disk dead isn't enough (the filesystem's
->shutdown() and the writeback flush in bdi_unregister() can block the
same way), and doing the removal without ub->mutex races with the
release work, recovery and a new server attaching. I'll send that part
separately as an RFC.

Tested in a VM with lockdep on top of master c339f4010dea, XFS and ext4,
with and without the server killed; block for-next has no ublk_drv.c
changes on top of it. The ublk selftests give the same results with and
without the series (integrity_01/03 fail on both, no
CONFIG_BLK_DEV_INTEGRITY here); a stress of DEL_DEV racing ADD_DEV of
the same id shows no warnings, also with KASAN and KCSAN.

This series was developed with the help of an AI coding assistant
(Claude); I reviewed and tested the changes and take responsibility
for them.

---
Mark Korenberg (2):
      ublk: publish a new device by its id only once it is set up
      ublk: don't take ublk_ctl_mutex in DEL_DEV

 drivers/block/ublk_drv.c | 60 +++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 23 deletions(-)
---
base-commit: c339f4010dea8b0d754f84ace70cd1b089878ef8
change-id: 20261104-ublk-del-gendisk-hang-bb33f56625f5

Best regards,
-- 
Mark Korenberg <socketpair@gmail.com>

[PATCH 1/2] ublk: publish a new device by its id only once it is set up

[Mark Korenberg]

ADD_DEV stores the device in ublk_index_idr when it allocates the id,
and control commands find it there as soon as ublk_add_chdev() has
initialized its struct device, before the char device is added. If
dev_set_name() or cdev_device_add() then fails, ublk_add_chdev() drops
the initial reference to free the device. A DEL_DEV that found the
device meanwhile waits for ublk_ctl_mutex, then calls ublk_remove():
cdev_device_del() of a device that was never added, and a second put of
the initial reference, which frees the device under the callers still
holding references.

Reserve the id with a NULL entry, which idr_find() and the idr iterators
skip, and store the device once ublk_add_chdev() succeeded.

Fixes: 71f28f3136af ("ublk_drv: add io_uring based userspace block driver")
Signed-off-by: Mark Korenberg <socketpair@gmail.com>
---
 drivers/block/ublk_drv.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 72e9c435208c..14d2823a92e5 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -4405,14 +4405,18 @@ static int ublk_alloc_dev_number(struct ublk_device *ub, int idx)
 	int i = idx;
 	int err;
 
+	/*
+	 * Only reserve the id: the device is published by
+	 * ublk_publish_dev_number() once it is fully set up.
+	 */
 	spin_lock(&ublk_idr_lock);
 	/* allocate id, if @id >= 0, we're requesting that specific id */
 	if (i >= 0) {
-		err = idr_alloc(&ublk_index_idr, ub, i, i + 1, GFP_NOWAIT);
+		err = idr_alloc(&ublk_index_idr, NULL, i, i + 1, GFP_NOWAIT);
 		if (err == -ENOSPC)
 			err = -EEXIST;
 	} else {
-		err = idr_alloc(&ublk_index_idr, ub, 0, UBLK_MAX_UBLKS,
+		err = idr_alloc(&ublk_index_idr, NULL, 0, UBLK_MAX_UBLKS,
 				GFP_NOWAIT);
 	}
 	spin_unlock(&ublk_idr_lock);
@@ -4423,6 +4427,19 @@ static int ublk_alloc_dev_number(struct ublk_device *ub, int idx)
 	return err;
 }
 
+/*
+ * Make the device visible to control commands by its id. Before its char
+ * device is added, DEL_DEV could find it and remove a char device that was
+ * never added, and drop the reference that ublk_add_chdev() drops on
+ * failure.
+ */
+static void ublk_publish_dev_number(struct ublk_device *ub)
+{
+	spin_lock(&ublk_idr_lock);
+	WARN_ON_ONCE(idr_replace(&ublk_index_idr, ub, ub->ub_number) != NULL);
+	spin_unlock(&ublk_idr_lock);
+}
+
 static void ublk_free_dev_number(struct ublk_device *ub)
 {
 	spin_lock(&ublk_idr_lock);
@@ -4991,6 +5008,8 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header)
 	 * ublk_add_chdev() will cleanup everything if it fails.
 	 */
 	ret = ublk_add_chdev(ub);
+	if (!ret)
+		ublk_publish_dev_number(ub);
 	goto out_unlock;
 
 out_deinit_queues:

-- 
2.56.0

[PATCH 1/2] ublk: publish a new device by its id only once it is set up

[Mark Korenberg via B4 Relay]

From: Mark Korenberg <socketpair@gmail.com>

ADD_DEV stores the device in ublk_index_idr when it allocates the id,
and control commands find it there as soon as ublk_add_chdev() has
initialized its struct device, before the char device is added. If
dev_set_name() or cdev_device_add() then fails, ublk_add_chdev() drops
the initial reference to free the device. A DEL_DEV that found the
device meanwhile waits for ublk_ctl_mutex, then calls ublk_remove():
cdev_device_del() of a device that was never added, and a second put of
the initial reference, which frees the device under the callers still
holding references.

Reserve the id with a NULL entry, which idr_find() and the idr iterators
skip, and store the device once ublk_add_chdev() succeeded.

Fixes: 71f28f3136af ("ublk_drv: add io_uring based userspace block driver")
Signed-off-by: Mark Korenberg <socketpair@gmail.com>
---
 drivers/block/ublk_drv.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 72e9c435208c..14d2823a92e5 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -4405,14 +4405,18 @@ static int ublk_alloc_dev_number(struct ublk_device *ub, int idx)
 	int i = idx;
 	int err;
 
+	/*
+	 * Only reserve the id: the device is published by
+	 * ublk_publish_dev_number() once it is fully set up.
+	 */
 	spin_lock(&ublk_idr_lock);
 	/* allocate id, if @id >= 0, we're requesting that specific id */
 	if (i >= 0) {
-		err = idr_alloc(&ublk_index_idr, ub, i, i + 1, GFP_NOWAIT);
+		err = idr_alloc(&ublk_index_idr, NULL, i, i + 1, GFP_NOWAIT);
 		if (err == -ENOSPC)
 			err = -EEXIST;
 	} else {
-		err = idr_alloc(&ublk_index_idr, ub, 0, UBLK_MAX_UBLKS,
+		err = idr_alloc(&ublk_index_idr, NULL, 0, UBLK_MAX_UBLKS,
 				GFP_NOWAIT);
 	}
 	spin_unlock(&ublk_idr_lock);
@@ -4423,6 +4427,19 @@ static int ublk_alloc_dev_number(struct ublk_device *ub, int idx)
 	return err;
 }
 
+/*
+ * Make the device visible to control commands by its id. Before its char
+ * device is added, DEL_DEV could find it and remove a char device that was
+ * never added, and drop the reference that ublk_add_chdev() drops on
+ * failure.
+ */
+static void ublk_publish_dev_number(struct ublk_device *ub)
+{
+	spin_lock(&ublk_idr_lock);
+	WARN_ON_ONCE(idr_replace(&ublk_index_idr, ub, ub->ub_number) != NULL);
+	spin_unlock(&ublk_idr_lock);
+}
+
 static void ublk_free_dev_number(struct ublk_device *ub)
 {
 	spin_lock(&ublk_idr_lock);
@@ -4991,6 +5008,8 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header)
 	 * ublk_add_chdev() will cleanup everything if it fails.
 	 */
 	ret = ublk_add_chdev(ub);
+	if (!ret)
+		ublk_publish_dev_number(ub);
 	goto out_unlock;
 
 out_deinit_queues:

-- 
2.56.0

[PATCH 2/2] ublk: don't take ublk_ctl_mutex in DEL_DEV

[Mark Korenberg]

ublk_ctrl_del_dev() holds the global ublk_ctl_mutex across
ublk_remove(). Stopping the device calls del_gendisk(), which syncs the
filesystem on the disk. If that filesystem is stuck, DEL_DEV never
returns, and ADD_DEV and DEL_DEV of every other ublk device wait for
ublk_ctl_mutex behind it, until reboot. This happens with the server
alive, and also when it dies: the release work then hangs in the same
del_gendisk() with ub->mutex held, and DEL_DEV sleeps on ub->mutex
uninterruptibly, holding ublk_ctl_mutex.

DEL_DEV doesn't need the mutex. ADD_DEV is done with the device when
DEL_DEV finds it: the device is found by its id only once it is set up,
since the previous patch. Claiming UB_STATE_DELETED with
test_and_set_bit() lets only one DEL_DEV remove the device, and the
device number is freed under ublk_idr_lock on release. The mutex also
protected unprivileged_ublks_added: make it atomic. ADD_DEV still
checks and increments it under the mutex, so a decrement without it
can only lower the count, and the limit holds.

A second DEL_DEV of the same device used to wait for the first one on
ublk_ctl_mutex. Now it sees UB_STATE_DELETED and, unless it is
DEL_DEV_ASYNC, waits interruptibly for the device number to be freed.
The counter is decremented once the device is removed, so at the limit
ADD_DEV of an unprivileged device fails with -EACCES instead of waiting
for the removal, and the limit is never exceeded, even if the removal
never completes.

Reproducer: XFS on a ublk loop device with an external log on
dm-linear; "dmsetup suspend --nolockfs --noflush" the log device, write
a file and syncfs, SIGKILL the ublk server or not, DEL_DEV the device,
ADD_DEV another one. Before, ADD_DEV hangs on ublk_ctl_mutex until the
log device is resumed. After, it completes; only the DEL_DEV of the
device with the stuck filesystem waits.

Signed-off-by: Mark Korenberg <socketpair@gmail.com>
---
 drivers/block/ublk_drv.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 14d2823a92e5..40f521077bc9 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -881,7 +881,8 @@ static ssize_t ublk_batch_copy_io_tags(struct ublk_batch_fetch_cmd *fcmd,
  * by cgroup.
  */
 static unsigned int unprivileged_ublks_max = 64;
-static unsigned int unprivileged_ublks_added; /* protected by ublk_ctl_mutex */
+/* checked and incremented under ublk_ctl_mutex, decremented without it */
+static atomic_t unprivileged_ublks_added = ATOMIC_INIT(0);
 
 static struct miscdevice ublk_misc;
 
@@ -4486,7 +4487,7 @@ static int ublk_add_chdev(struct ublk_device *ub)
 		goto fail;
 
 	if (ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV)
-		unprivileged_ublks_added++;
+		atomic_inc(&unprivileged_ublks_added);
 	return 0;
  fail:
 	put_device(dev);
@@ -4518,15 +4519,9 @@ static int ublk_add_tag_set(struct ublk_device *ub)
 
 static void ublk_remove(struct ublk_device *ub)
 {
-	bool unprivileged;
-
 	ublk_stop_dev(ub);
 	cdev_device_del(&ub->cdev, &ub->cdev_dev);
-	unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV;
 	ublk_put_device(ub);
-
-	if (unprivileged)
-		unprivileged_ublks_added--;
 }
 
 static struct ublk_device *ublk_get_device_from_id(int idx)
@@ -4916,7 +4911,7 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header)
 
 	ret = -EACCES;
 	if ((info.flags & UBLK_F_UNPRIVILEGED_DEV) &&
-	    unprivileged_ublks_added >= unprivileged_ublks_max)
+	    atomic_read(&unprivileged_ublks_added) >= unprivileged_ublks_max)
 		goto out_unlock;
 
 	ret = -ENOMEM;
@@ -5042,23 +5037,23 @@ static int ublk_ctrl_del_dev(struct ublk_device **p_ub, bool wait)
 {
 	struct ublk_device *ub = *p_ub;
 	int idx = ub->ub_number;
-	int ret;
-
-	ret = mutex_lock_killable(&ublk_ctl_mutex);
-	if (ret)
-		return ret;
+	bool unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV;
 
-	if (!test_bit(UB_STATE_DELETED, &ub->state)) {
+	/*
+	 * Don't take ublk_ctl_mutex: stopping the device may wait for the
+	 * filesystem on its disk indefinitely, and ADD_DEV and DEL_DEV of all
+	 * other ublk devices would wait for that too. ADD_DEV is done with the
+	 * device, it is found by its id only then.
+	 */
+	if (!test_and_set_bit(UB_STATE_DELETED, &ub->state)) {
 		ublk_remove(ub);
-		set_bit(UB_STATE_DELETED, &ub->state);
+		if (unprivileged)
+			atomic_dec(&unprivileged_ublks_added);
 	}
 
-	mutex_unlock(&ublk_ctl_mutex);
-
 	/*
-	 * Drop the reference outside ublk_ctl_mutex: if it is the last one,
-	 * the release frees the tag set, which waits for an SRCU grace period,
-	 * and holding the mutex would serialize concurrent deletions on it.
+	 * If this is the last reference, the release frees the tag set, which
+	 * waits for an SRCU grace period.
 	 */
 	*p_ub = NULL;
 	ublk_put_device(ub);

-- 
2.56.0

[PATCH 2/2] ublk: don't take ublk_ctl_mutex in DEL_DEV

[Mark Korenberg via B4 Relay]

From: Mark Korenberg <socketpair@gmail.com>

ublk_ctrl_del_dev() holds the global ublk_ctl_mutex across
ublk_remove(). Stopping the device calls del_gendisk(), which syncs the
filesystem on the disk. If that filesystem is stuck, DEL_DEV never
returns, and ADD_DEV and DEL_DEV of every other ublk device wait for
ublk_ctl_mutex behind it, until reboot. This happens with the server
alive, and also when it dies: the release work then hangs in the same
del_gendisk() with ub->mutex held, and DEL_DEV sleeps on ub->mutex
uninterruptibly, holding ublk_ctl_mutex.

DEL_DEV doesn't need the mutex. ADD_DEV is done with the device when
DEL_DEV finds it: the device is found by its id only once it is set up,
since the previous patch. Claiming UB_STATE_DELETED with
test_and_set_bit() lets only one DEL_DEV remove the device, and the
device number is freed under ublk_idr_lock on release. The mutex also
protected unprivileged_ublks_added: make it atomic. ADD_DEV still
checks and increments it under the mutex, so a decrement without it
can only lower the count, and the limit holds.

A second DEL_DEV of the same device used to wait for the first one on
ublk_ctl_mutex. Now it sees UB_STATE_DELETED and, unless it is
DEL_DEV_ASYNC, waits interruptibly for the device number to be freed.
The counter is decremented once the device is removed, so at the limit
ADD_DEV of an unprivileged device fails with -EACCES instead of waiting
for the removal, and the limit is never exceeded, even if the removal
never completes.

Reproducer: XFS on a ublk loop device with an external log on
dm-linear; "dmsetup suspend --nolockfs --noflush" the log device, write
a file and syncfs, SIGKILL the ublk server or not, DEL_DEV the device,
ADD_DEV another one. Before, ADD_DEV hangs on ublk_ctl_mutex until the
log device is resumed. After, it completes; only the DEL_DEV of the
device with the stuck filesystem waits.

Signed-off-by: Mark Korenberg <socketpair@gmail.com>
---
 drivers/block/ublk_drv.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 14d2823a92e5..40f521077bc9 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -881,7 +881,8 @@ static ssize_t ublk_batch_copy_io_tags(struct ublk_batch_fetch_cmd *fcmd,
  * by cgroup.
  */
 static unsigned int unprivileged_ublks_max = 64;
-static unsigned int unprivileged_ublks_added; /* protected by ublk_ctl_mutex */
+/* checked and incremented under ublk_ctl_mutex, decremented without it */
+static atomic_t unprivileged_ublks_added = ATOMIC_INIT(0);
 
 static struct miscdevice ublk_misc;
 
@@ -4486,7 +4487,7 @@ static int ublk_add_chdev(struct ublk_device *ub)
 		goto fail;
 
 	if (ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV)
-		unprivileged_ublks_added++;
+		atomic_inc(&unprivileged_ublks_added);
 	return 0;
  fail:
 	put_device(dev);
@@ -4518,15 +4519,9 @@ static int ublk_add_tag_set(struct ublk_device *ub)
 
 static void ublk_remove(struct ublk_device *ub)
 {
-	bool unprivileged;
-
 	ublk_stop_dev(ub);
 	cdev_device_del(&ub->cdev, &ub->cdev_dev);
-	unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV;
 	ublk_put_device(ub);
-
-	if (unprivileged)
-		unprivileged_ublks_added--;
 }
 
 static struct ublk_device *ublk_get_device_from_id(int idx)
@@ -4916,7 +4911,7 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header)
 
 	ret = -EACCES;
 	if ((info.flags & UBLK_F_UNPRIVILEGED_DEV) &&
-	    unprivileged_ublks_added >= unprivileged_ublks_max)
+	    atomic_read(&unprivileged_ublks_added) >= unprivileged_ublks_max)
 		goto out_unlock;
 
 	ret = -ENOMEM;
@@ -5042,23 +5037,23 @@ static int ublk_ctrl_del_dev(struct ublk_device **p_ub, bool wait)
 {
 	struct ublk_device *ub = *p_ub;
 	int idx = ub->ub_number;
-	int ret;
-
-	ret = mutex_lock_killable(&ublk_ctl_mutex);
-	if (ret)
-		return ret;
+	bool unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV;
 
-	if (!test_bit(UB_STATE_DELETED, &ub->state)) {
+	/*
+	 * Don't take ublk_ctl_mutex: stopping the device may wait for the
+	 * filesystem on its disk indefinitely, and ADD_DEV and DEL_DEV of all
+	 * other ublk devices would wait for that too. ADD_DEV is done with the
+	 * device, it is found by its id only then.
+	 */
+	if (!test_and_set_bit(UB_STATE_DELETED, &ub->state)) {
 		ublk_remove(ub);
-		set_bit(UB_STATE_DELETED, &ub->state);
+		if (unprivileged)
+			atomic_dec(&unprivileged_ublks_added);
 	}
 
-	mutex_unlock(&ublk_ctl_mutex);
-
 	/*
-	 * Drop the reference outside ublk_ctl_mutex: if it is the last one,
-	 * the release frees the tag set, which waits for an SRCU grace period,
-	 * and holding the mutex would serialize concurrent deletions on it.
+	 * If this is the last reference, the release frees the tag set, which
+	 * waits for an SRCU grace period.
 	 */
 	*p_ub = NULL;
 	ublk_put_device(ub);

-- 
2.56.0

Re: [PATCH 1/2] ublk: publish a new device by its id only once it is set up

[Ming Lei]

On Thu, Nov 05, 2026 at 12:25:05AM +0500, Mark Korenberg via B4 Relay wrote:
> From: Mark Korenberg <socketpair@gmail.com>
> 
> ADD_DEV stores the device in ublk_index_idr when it allocates the id,
> and control commands find it there as soon as ublk_add_chdev() has
> initialized its struct device, before the char device is added. If
> dev_set_name() or cdev_device_add() then fails, ublk_add_chdev() drops
> the initial reference to free the device. A DEL_DEV that found the
> device meanwhile waits for ublk_ctl_mutex, then calls ublk_remove():
> cdev_device_del() of a device that was never added, and a second put of
> the initial reference, which frees the device under the callers still
> holding references.
> 
> Reserve the id with a NULL entry, which idr_find() and the idr iterators
> skip, and store the device once ublk_add_chdev() succeeded.
> 
> Fixes: 71f28f3136af ("ublk_drv: add io_uring based userspace block driver")
> Signed-off-by: Mark Korenberg <socketpair@gmail.com>

Reviewed-by: Ming Lei <tom.leiming@gmail.com>

Thanks,
Ming

Re: [PATCH 2/2] ublk: don't take ublk_ctl_mutex in DEL_DEV

[Ming Lei]

On Thu, Nov 05, 2026 at 12:25:06AM +0500, Mark Korenberg via B4 Relay wrote:
> From: Mark Korenberg <socketpair@gmail.com>
> 
> ublk_ctrl_del_dev() holds the global ublk_ctl_mutex across
> ublk_remove(). Stopping the device calls del_gendisk(), which syncs the
> filesystem on the disk. If that filesystem is stuck, DEL_DEV never
> returns, and ADD_DEV and DEL_DEV of every other ublk device wait for
> ublk_ctl_mutex behind it, until reboot. This happens with the server
> alive, and also when it dies: the release work then hangs in the same
> del_gendisk() with ub->mutex held, and DEL_DEV sleeps on ub->mutex
> uninterruptibly, holding ublk_ctl_mutex.
> 
> DEL_DEV doesn't need the mutex. ADD_DEV is done with the device when
> DEL_DEV finds it: the device is found by its id only once it is set up,
> since the previous patch. Claiming UB_STATE_DELETED with
> test_and_set_bit() lets only one DEL_DEV remove the device, and the
> device number is freed under ublk_idr_lock on release. The mutex also
> protected unprivileged_ublks_added: make it atomic. ADD_DEV still
> checks and increments it under the mutex, so a decrement without it
> can only lower the count, and the limit holds.
> 
> A second DEL_DEV of the same device used to wait for the first one on
> ublk_ctl_mutex. Now it sees UB_STATE_DELETED and, unless it is
> DEL_DEV_ASYNC, waits interruptibly for the device number to be freed.
> The counter is decremented once the device is removed, so at the limit
> ADD_DEV of an unprivileged device fails with -EACCES instead of waiting
> for the removal, and the limit is never exceeded, even if the removal
> never completes.
> 
> Reproducer: XFS on a ublk loop device with an external log on
> dm-linear; "dmsetup suspend --nolockfs --noflush" the log device, write
> a file and syncfs, SIGKILL the ublk server or not, DEL_DEV the device,
> ADD_DEV another one. Before, ADD_DEV hangs on ublk_ctl_mutex until the
> log device is resumed. After, it completes; only the DEL_DEV of the
> device with the stuck filesystem waits.
> 
> Signed-off-by: Mark Korenberg <socketpair@gmail.com>

Reviewed-by: Ming Lei <tom.leiming@gmail.com>



Thanks,
Ming

Re: [PATCH 0/2] ublk: don't let one stuck device block control of all ublk devices

[Jens Axboe]

On Thu, 05 Nov 2026 00:25:04 +0500, Mark Korenberg wrote:
> If the filesystem on a ublk device is stuck, removing that device
> blocks in del_gendisk(), which syncs the filesystem. DEL_DEV holds the
> global ublk_ctl_mutex across the removal, so ADD_DEV and DEL_DEV of
> every other ublk device hang too, until reboot. It doesn't matter
> whether the server is alive: when it dies, the release work does the
> same syncing del_gendisk() under ub->mutex, and DEL_DEV waits for
> ub->mutex holding ublk_ctl_mutex.
> 
> [...]

Applied, thanks!

[1/2] ublk: publish a new device by its id only once it is set up
      commit: e5c463e5074a940c644d54d8fc29aba06709aab0
[2/2] ublk: don't take ublk_ctl_mutex in DEL_DEV
      commit: 8067050e7226844fb32062b5cea947e5897d3d7e

Best regards,
-- 
Jens Axboe