[PATCH V2 0/2] ublk_drv: make sure that correct flags(features) returned to userspace

[PATCH V2 0/2] ublk_drv: make sure that correct flags(features) returned to userspace

[Ming Lei]

Hello Jens,

The 1st patch cleans ublk_add_dev a bit, meantime fix one potential
free un-allocated buffer issue.

The 2nd one makes sure that driver supported flags returned to
userspace, this way is important for maintaining compatibility.



Ming Lei (2):
  ublk_drv: fix error handling of ublk_add_dev
  ublk_drv: make sure that correct flags(features) returned to userspace

 drivers/block/ublk_drv.c      | 43 +++++++++++++++++++++++++++--------
 include/uapi/linux/ublk_cmd.h | 11 +++++++--
 2 files changed, 43 insertions(+), 11 deletions(-)

-- 
2.31.1

[PATCH V2 1/2] ublk_drv: fix error handling of ublk_add_dev

[Ming Lei]

__ublk_destroy_dev() is called for handling error in ublk_add_dev(),
but either tagset isn't allocated or mutex isn't initialized.

So fix the issue by letting ublk_add_dev cleanup its own allocation,
and simply call kfree(ub) outside of ublk_add_dev which is named
as ublk_add_tagset(), meantime ublk_add_chdev() is moved out too.

Now the error handling in ublk_ctrl_add_dev() becomes more readable.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index f058f40b639c..a427f020527d 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1093,7 +1093,7 @@ static void ublk_align_max_io_size(struct ublk_device *ub)
 }
 
 /* add tag_set & cdev, cleanup everything in case of failure */
-static int ublk_add_dev(struct ublk_device *ub)
+static int ublk_add_tagset(struct ublk_device *ub)
 {
 	int err = -ENOMEM;
 
@@ -1108,7 +1108,7 @@ static int ublk_add_dev(struct ublk_device *ub)
 	INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work);
 
 	if (ublk_init_queues(ub))
-		goto out_destroy_dev;
+		return err;
 
 	ub->tag_set.ops = &ublk_mq_ops;
 	ub->tag_set.nr_hw_queues = ub->dev_info.nr_hw_queues;
@@ -1125,13 +1125,10 @@ static int ublk_add_dev(struct ublk_device *ub)
 	mutex_init(&ub->mutex);
 	spin_lock_init(&ub->mm_lock);
 
-	/* add char dev so that ublksrv daemon can be setup */
-	return ublk_add_chdev(ub);
+	return 0;
 
 out_deinit_queues:
 	ublk_deinit_queues(ub);
-out_destroy_dev:
-	__ublk_destroy_dev(ub);
 	return err;
 }
 
@@ -1330,7 +1327,18 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
 	/* update device id */
 	ub->dev_info.dev_id = ub->ub_number;
 
-	ret = ublk_add_dev(ub);
+	ret = ublk_add_tagset(ub);
+	if (ret) {
+		kfree(ub);
+		goto out_unlock;
+	}
+
+	/*
+	 * add char dev so that ublksrv daemon can be setup
+	 *
+	 * ublk_add_chdev() will cleanup everything if it fails.
+	 */
+	ret = ublk_add_chdev(ub);
 	if (ret)
 		goto out_unlock;
 
-- 
2.31.1

[PATCH V2 2/2] ublk_drv: make sure that correct flags(features) returned to userspace

[Ming Lei]

Userspace may support more features or new added flags, but the driver
side can be old, so make sure correct flags(features) returned to
userpsace, then userspace can work as expected.

Reviewed-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c      | 21 +++++++++++++++++++--
 include/uapi/linux/ublk_cmd.h | 11 +++++++++--
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index a427f020527d..a3ba642a5e43 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1092,13 +1092,28 @@ static void ublk_align_max_io_size(struct ublk_device *ub)
 		round_down(max_rq_bytes, PAGE_SIZE) >> ub->bs_shift;
 }
 
+static void ublk_negotiate_features(struct ublk_device *ub)
+{
+	unsigned long *map = (unsigned long *)&ub->dev_info.flags[0];
+
+	/* We are not ready to support zero copy */
+	ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
+
+	/*
+	 * 128bit flags will be copied back to userspace as feature
+	 * negotiation result, so have to clear flags which driver
+	 * doesn't support yet, then userspace can get correct flags
+	 * (features) to handle.
+	 */
+	bitmap_clear(map, __UBLK_F_NR_BITS, 128 - __UBLK_F_NR_BITS);
+}
+
 /* add tag_set & cdev, cleanup everything in case of failure */
 static int ublk_add_tagset(struct ublk_device *ub)
 {
 	int err = -ENOMEM;
 
-	/* We are not ready to support zero copy */
-	ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
+	ublk_negotiate_features(ub);
 
 	ub->bs_shift = ilog2(ub->dev_info.block_size);
 	ub->dev_info.nr_hw_queues = min_t(unsigned int,
@@ -1499,6 +1514,8 @@ static int __init ublk_init(void)
 {
 	int ret;
 
+	BUILD_BUG_ON(__UBLK_F_NR_BITS > 128);
+
 	init_waitqueue_head(&ublk_idr_wq);
 
 	ret = misc_register(&ublk_misc);
diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
index 917580b34198..49e4950a9181 100644
--- a/include/uapi/linux/ublk_cmd.h
+++ b/include/uapi/linux/ublk_cmd.h
@@ -42,17 +42,24 @@
 /* tag bit is 12bit, so at most 4096 IOs for each queue */
 #define UBLK_MAX_QUEUE_DEPTH	4096
 
+
+enum ublk_flag_bits {
+	__UBLK_F_SUPPORT_ZERO_COPY,
+	__UBLK_F_URING_CMD_COMP_IN_TASK,
+	__UBLK_F_NR_BITS,
+};
+
 /*
  * zero copy requires 4k block size, and can remap ublk driver's io
  * request into ublksrv's vm space
  */
-#define UBLK_F_SUPPORT_ZERO_COPY	(1UL << 0)
+#define UBLK_F_SUPPORT_ZERO_COPY	(1ULL << __UBLK_F_SUPPORT_ZERO_COPY)
 
 /*
  * Force to complete io cmd via io_uring_cmd_complete_in_task so that
  * performance comparison is done easily with using task_work_add
  */
-#define UBLK_F_URING_CMD_COMP_IN_TASK	(1UL << 1)
+#define UBLK_F_URING_CMD_COMP_IN_TASK	(1ULL << __UBLK_F_URING_CMD_COMP_IN_TASK)
 
 /* device state */
 #define UBLK_S_DEV_DEAD	0
-- 
2.31.1

Re: [PATCH V2 1/2] ublk_drv: fix error handling of ublk_add_dev

[Christoph Hellwig]

I think __ublk_destroy_dev just needs to go away in that form.
Also I'd much rather do the copy_to_user before the ublk_add_chdev
as that means we never remove a devic already marked life due to a
failure.  Something like the patch below, which will need testing first
before I'd dare to submit it:

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index f058f40b639c3..64e63bad0919d 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1005,7 +1005,7 @@ static int ublk_init_queues(struct ublk_device *ub)
 	return ret;
 }
 
-static int __ublk_alloc_dev_number(struct ublk_device *ub, int idx)
+static int ublk_alloc_dev_number(struct ublk_device *ub, int idx)
 {
 	int i = idx;
 	int err;
@@ -1027,16 +1027,12 @@ static int __ublk_alloc_dev_number(struct ublk_device *ub, int idx)
 	return err;
 }
 
-static void __ublk_destroy_dev(struct ublk_device *ub)
+static void ublk_free_dev_number(struct ublk_device *ub)
 {
 	spin_lock(&ublk_idr_lock);
 	idr_remove(&ublk_index_idr, ub->ub_number);
 	wake_up_all(&ublk_idr_wq);
 	spin_unlock(&ublk_idr_lock);
-
-	mutex_destroy(&ub->mutex);
-
-	kfree(ub);
 }
 
 static void ublk_cdev_rel(struct device *dev)
@@ -1045,8 +1041,9 @@ static void ublk_cdev_rel(struct device *dev)
 
 	blk_mq_free_tag_set(&ub->tag_set);
 	ublk_deinit_queues(ub);
-
-	__ublk_destroy_dev(ub);
+	ublk_free_dev_number(ub);
+	mutex_destroy(&ub->mutex);
+	kfree(ub);
 }
 
 static int ublk_add_chdev(struct ublk_device *ub)
@@ -1092,23 +1089,13 @@ static void ublk_align_max_io_size(struct ublk_device *ub)
 		round_down(max_rq_bytes, PAGE_SIZE) >> ub->bs_shift;
 }
 
-/* add tag_set & cdev, cleanup everything in case of failure */
-static int ublk_add_dev(struct ublk_device *ub)
+static int ublk_add_tag_set(struct ublk_device *ub)
 {
-	int err = -ENOMEM;
-
-	/* We are not ready to support zero copy */
-	ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
-
-	ub->bs_shift = ilog2(ub->dev_info.block_size);
-	ub->dev_info.nr_hw_queues = min_t(unsigned int,
-			ub->dev_info.nr_hw_queues, nr_cpu_ids);
-
-	INIT_WORK(&ub->stop_work, ublk_stop_work_fn);
-	INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work);
+	int err;
 
-	if (ublk_init_queues(ub))
-		goto out_destroy_dev;
+	err = ublk_init_queues(ub);
+	if (err)
+		return err;
 
 	ub->tag_set.ops = &ublk_mq_ops;
 	ub->tag_set.nr_hw_queues = ub->dev_info.nr_hw_queues;
@@ -1119,19 +1106,7 @@ static int ublk_add_dev(struct ublk_device *ub)
 	ub->tag_set.driver_data = ub;
 	err = blk_mq_alloc_tag_set(&ub->tag_set);
 	if (err)
-		goto out_deinit_queues;
-
-	ublk_align_max_io_size(ub);
-	mutex_init(&ub->mutex);
-	spin_lock_init(&ub->mm_lock);
-
-	/* add char dev so that ublksrv daemon can be setup */
-	return ublk_add_chdev(ub);
-
-out_deinit_queues:
-	ublk_deinit_queues(ub);
-out_destroy_dev:
-	__ublk_destroy_dev(ub);
+		ublk_deinit_queues(ub);
 	return err;
 }
 
@@ -1318,26 +1293,52 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
 	ub = kzalloc(sizeof(*ub), GFP_KERNEL);
 	if (!ub)
 		goto out_unlock;
+	mutex_init(&ub->mutex);
+	spin_lock_init(&ub->mm_lock);
+	INIT_WORK(&ub->stop_work, ublk_stop_work_fn);
+	INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work);
 
-	ret = __ublk_alloc_dev_number(ub, header->dev_id);
-	if (ret < 0) {
-		kfree(ub);
-		goto out_unlock;
-	}
+	ret = ublk_alloc_dev_number(ub, header->dev_id);
+	if (ret < 0)
+		goto out_free_ub;
 
 	memcpy(&ub->dev_info, &info, sizeof(info));
 
 	/* update device id */
 	ub->dev_info.dev_id = ub->ub_number;
 
-	ret = ublk_add_dev(ub);
+	/* We are not ready to support zero copy */
+	ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
+
+	ub->bs_shift = ilog2(ub->dev_info.block_size);
+	ub->dev_info.nr_hw_queues = min_t(unsigned int,
+			ub->dev_info.nr_hw_queues, nr_cpu_ids);
+	ublk_align_max_io_size(ub);
+
+	ret = ublk_add_tag_set(ub);
 	if (ret)
-		goto out_unlock;
+		goto out_free_dev_number;
 
-	if (copy_to_user(argp, &ub->dev_info, sizeof(info))) {
-		ublk_remove(ub);
-		ret = -EFAULT;
-	}
+	ret = -EFAULT;
+	if (copy_to_user(argp, &ub->dev_info, sizeof(info)))
+		goto out_free_tag_set;
+
+	/*
+	 * Add the char dev so that ublksrv daemon can be setup
+	 *
+	 * ublk_add_chdev() will cleanup everything if it fails.
+	 */
+	ret = ublk_add_chdev(ub);
+	goto out_unlock;
+
+out_free_tag_set:
+	blk_mq_free_tag_set(&ub->tag_set);
+	ublk_deinit_queues(ub);
+out_free_dev_number:
+	ublk_free_dev_number(ub);
+out_free_ub:
+	mutex_destroy(&ub->mutex);
+	kfree(ub);
 out_unlock:
 	mutex_unlock(&ublk_ctl_mutex);
 	return ret;

Re: [PATCH V2 2/2] ublk_drv: make sure that correct flags(features) returned to userspace

[Christoph Hellwig]

On Fri, Jul 22, 2022 at 01:09:30PM +0800, Ming Lei wrote:
> +	unsigned long *map = (unsigned long *)&ub->dev_info.flags[0];
> +
> +	/* We are not ready to support zero copy */
> +	ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
> +
> +	/*
> +	 * 128bit flags will be copied back to userspace as feature
> +	 * negotiation result, so have to clear flags which driver
> +	 * doesn't support yet, then userspace can get correct flags
> +	 * (features) to handle.
> +	 */
> +	bitmap_clear(map, __UBLK_F_NR_BITS, 128 - __UBLK_F_NR_BITS);

Please don't do the cast and bitmap ops.  In fact I think the current
ABI is rather nasty.  To make everyones life easier just use a single
u64 flags, an mark the second one reserved so that we can extent into
it with extra flags or something else.  That way normal C operator
leve bitops just work.

> +enum ublk_flag_bits {
> +	__UBLK_F_SUPPORT_ZERO_COPY,
> +	__UBLK_F_URING_CMD_COMP_IN_TASK,
> +	__UBLK_F_NR_BITS,
> +};

Please make these #defines so that userspace can detect if they
exist in a header using #ifdef.

Re: [PATCH V2 1/2] ublk_drv: fix error handling of ublk_add_dev

[Ziyang Zhang]

On 2022/7/22 13:09, Ming Lei wrote:
> __ublk_destroy_dev() is called for handling error in ublk_add_dev(),
> but either tagset isn't allocated or mutex isn't initialized.
> 
> So fix the issue by letting ublk_add_dev cleanup its own allocation,
> and simply call kfree(ub) outside of ublk_add_dev which is named
> as ublk_add_tagset(), meantime ublk_add_chdev() is moved out too.
> 
> Now the error handling in ublk_ctrl_add_dev() becomes more readable.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
Reviewed-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>

Re: [PATCH V2 2/2] ublk_drv: make sure that correct flags(features) returned to userspace

[Ziyang Zhang]

On 2022/7/22 13:09, Ming Lei wrote:
> Userspace may support more features or new added flags, but the driver
> side can be old, so make sure correct flags(features) returned to
> userpsace, then userspace can work as expected.
> 
> Reviewed-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
Reviewed-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>

Re: [PATCH V2 1/2] ublk_drv: fix error handling of ublk_add_dev

[Christoph Hellwig]

On Thu, Jul 21, 2022 at 10:48:07PM -0700, Christoph Hellwig wrote:
> I think __ublk_destroy_dev just needs to go away in that form.
> Also I'd much rather do the copy_to_user before the ublk_add_chdev
> as that means we never remove a devic already marked life due to a
> failure.  Something like the patch below, which will need testing first
> before I'd dare to submit it:

Improved and tested version:

---
From 49ba6d0c5788ea9d3a6ef88d910b702152f5d75a Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 22 Jul 2022 07:38:59 +0200
Subject: ublk_drv: fix error handling of ublk_add_dev

__ublk_destroy_dev() is called for handling error in ublk_add_dev(),
but either tagset isn't allocated or mutex isn't initialized.

So fix the issue by letting replacing ublk_add_dev with a
ublk_add_tag_set function that is much more limited in scope and
instead unwind every single step directly in ublk_ctrl_add_dev.
To allow for this refactor the device freeing so that there is
a helper for freeing the device number instead of coupling that
with freeing the mutex and the memory.

Note that this now copies the dev_info to userspace before adding
the character device.  This not only simplifies the erro handling
in ublk_ctrl_add_dev, but also means that the character device
can only be seen by userspace if the device addition succeeded.

Based on a patch from Ming Lei.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/ublk_drv.c | 100 +++++++++++++++++++--------------------
 1 file changed, 48 insertions(+), 52 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index f058f40b639c3..67f91a80a7aba 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1005,7 +1005,7 @@ static int ublk_init_queues(struct ublk_device *ub)
 	return ret;
 }
 
-static int __ublk_alloc_dev_number(struct ublk_device *ub, int idx)
+static int ublk_alloc_dev_number(struct ublk_device *ub, int idx)
 {
 	int i = idx;
 	int err;
@@ -1027,16 +1027,12 @@ static int __ublk_alloc_dev_number(struct ublk_device *ub, int idx)
 	return err;
 }
 
-static void __ublk_destroy_dev(struct ublk_device *ub)
+static void ublk_free_dev_number(struct ublk_device *ub)
 {
 	spin_lock(&ublk_idr_lock);
 	idr_remove(&ublk_index_idr, ub->ub_number);
 	wake_up_all(&ublk_idr_wq);
 	spin_unlock(&ublk_idr_lock);
-
-	mutex_destroy(&ub->mutex);
-
-	kfree(ub);
 }
 
 static void ublk_cdev_rel(struct device *dev)
@@ -1045,8 +1041,9 @@ static void ublk_cdev_rel(struct device *dev)
 
 	blk_mq_free_tag_set(&ub->tag_set);
 	ublk_deinit_queues(ub);
-
-	__ublk_destroy_dev(ub);
+	ublk_free_dev_number(ub);
+	mutex_destroy(&ub->mutex);
+	kfree(ub);
 }
 
 static int ublk_add_chdev(struct ublk_device *ub)
@@ -1092,24 +1089,8 @@ static void ublk_align_max_io_size(struct ublk_device *ub)
 		round_down(max_rq_bytes, PAGE_SIZE) >> ub->bs_shift;
 }
 
-/* add tag_set & cdev, cleanup everything in case of failure */
-static int ublk_add_dev(struct ublk_device *ub)
+static int ublk_add_tag_set(struct ublk_device *ub)
 {
-	int err = -ENOMEM;
-
-	/* We are not ready to support zero copy */
-	ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
-
-	ub->bs_shift = ilog2(ub->dev_info.block_size);
-	ub->dev_info.nr_hw_queues = min_t(unsigned int,
-			ub->dev_info.nr_hw_queues, nr_cpu_ids);
-
-	INIT_WORK(&ub->stop_work, ublk_stop_work_fn);
-	INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work);
-
-	if (ublk_init_queues(ub))
-		goto out_destroy_dev;
-
 	ub->tag_set.ops = &ublk_mq_ops;
 	ub->tag_set.nr_hw_queues = ub->dev_info.nr_hw_queues;
 	ub->tag_set.queue_depth = ub->dev_info.queue_depth;
@@ -1117,22 +1098,7 @@ static int ublk_add_dev(struct ublk_device *ub)
 	ub->tag_set.cmd_size = sizeof(struct ublk_rq_data);
 	ub->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
 	ub->tag_set.driver_data = ub;
-	err = blk_mq_alloc_tag_set(&ub->tag_set);
-	if (err)
-		goto out_deinit_queues;
-
-	ublk_align_max_io_size(ub);
-	mutex_init(&ub->mutex);
-	spin_lock_init(&ub->mm_lock);
-
-	/* add char dev so that ublksrv daemon can be setup */
-	return ublk_add_chdev(ub);
-
-out_deinit_queues:
-	ublk_deinit_queues(ub);
-out_destroy_dev:
-	__ublk_destroy_dev(ub);
-	return err;
+	return blk_mq_alloc_tag_set(&ub->tag_set);
 }
 
 static void ublk_remove(struct ublk_device *ub)
@@ -1318,26 +1284,56 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
 	ub = kzalloc(sizeof(*ub), GFP_KERNEL);
 	if (!ub)
 		goto out_unlock;
+	mutex_init(&ub->mutex);
+	spin_lock_init(&ub->mm_lock);
+	INIT_WORK(&ub->stop_work, ublk_stop_work_fn);
+	INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work);
 
-	ret = __ublk_alloc_dev_number(ub, header->dev_id);
-	if (ret < 0) {
-		kfree(ub);
-		goto out_unlock;
-	}
+	ret = ublk_alloc_dev_number(ub, header->dev_id);
+	if (ret < 0)
+		goto out_free_ub;
 
 	memcpy(&ub->dev_info, &info, sizeof(info));
 
 	/* update device id */
 	ub->dev_info.dev_id = ub->ub_number;
 
-	ret = ublk_add_dev(ub);
+	/* We are not ready to support zero copy */
+	ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
+
+	ub->bs_shift = ilog2(ub->dev_info.block_size);
+	ub->dev_info.nr_hw_queues = min_t(unsigned int,
+			ub->dev_info.nr_hw_queues, nr_cpu_ids);
+	ublk_align_max_io_size(ub);
+
+	ret = ublk_init_queues(ub);
 	if (ret)
-		goto out_unlock;
+		goto out_free_dev_number;
 
-	if (copy_to_user(argp, &ub->dev_info, sizeof(info))) {
-		ublk_remove(ub);
-		ret = -EFAULT;
-	}
+	ret = ublk_add_tag_set(ub);
+	if (ret)
+		goto out_deinit_queues;
+
+	ret = -EFAULT;
+	if (copy_to_user(argp, &ub->dev_info, sizeof(info)))
+		goto out_free_tag_set;
+
+	/*
+	 * Add the char dev so that ublksrv daemon can be setup.
+	 * ublk_add_chdev() will cleanup everything if it fails.
+	 */
+	ret = ublk_add_chdev(ub);
+	goto out_unlock;
+
+out_free_tag_set:
+	blk_mq_free_tag_set(&ub->tag_set);
+out_deinit_queues:
+	ublk_deinit_queues(ub);
+out_free_dev_number:
+	ublk_free_dev_number(ub);
+out_free_ub:
+	mutex_destroy(&ub->mutex);
+	kfree(ub);
 out_unlock:
 	mutex_unlock(&ublk_ctl_mutex);
 	return ret;
-- 
2.30.2

Re: [PATCH V2 2/2] ublk_drv: make sure that correct flags(features) returned to userspace

[Ming Lei]

On Thu, Jul 21, 2022 at 10:50:43PM -0700, Christoph Hellwig wrote:
> On Fri, Jul 22, 2022 at 01:09:30PM +0800, Ming Lei wrote:
> > +	unsigned long *map = (unsigned long *)&ub->dev_info.flags[0];
> > +
> > +	/* We are not ready to support zero copy */
> > +	ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
> > +
> > +	/*
> > +	 * 128bit flags will be copied back to userspace as feature
> > +	 * negotiation result, so have to clear flags which driver
> > +	 * doesn't support yet, then userspace can get correct flags
> > +	 * (features) to handle.
> > +	 */
> > +	bitmap_clear(map, __UBLK_F_NR_BITS, 128 - __UBLK_F_NR_BITS);
> 
> Please don't do the cast and bitmap ops.  In fact I think the current
> ABI is rather nasty.  To make everyones life easier just use a single
> u64 flags, an mark the second one reserved so that we can extent into
> it with extra flags or something else.  That way normal C operator
> leve bitops just work.

OK.

> 
> > +enum ublk_flag_bits {
> > +	__UBLK_F_SUPPORT_ZERO_COPY,
> > +	__UBLK_F_URING_CMD_COMP_IN_TASK,
> > +	__UBLK_F_NR_BITS,
> > +};
> 
> Please make these #defines so that userspace can detect if they
> exist in a header using #ifdef.

userspace is supposed to only use UBLK_F_* instead of __UBLK_F_*, one
benefit of using enum is that UBLK_F_NR_BITS can be figured out
automatically, otherwise how can we figure out the max bits?


thanks,
Ming

Re: [PATCH V2 1/2] ublk_drv: fix error handling of ublk_add_dev

[Ziyang Zhang]

On 2022/7/22 14:17, Christoph Hellwig wrote:
> On Thu, Jul 21, 2022 at 10:48:07PM -0700, Christoph Hellwig wrote:
>> I think __ublk_destroy_dev just needs to go away in that form.
>> Also I'd much rather do the copy_to_user before the ublk_add_chdev
>> as that means we never remove a devic already marked life due to a
>> failure.  Something like the patch below, which will need testing first
>> before I'd dare to submit it:
> 
> Improved and tested version:
> 
> ---
> From 49ba6d0c5788ea9d3a6ef88d910b702152f5d75a Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 22 Jul 2022 07:38:59 +0200
> Subject: ublk_drv: fix error handling of ublk_add_dev
> 
> __ublk_destroy_dev() is called for handling error in ublk_add_dev(),
> but either tagset isn't allocated or mutex isn't initialized.
> 
> So fix the issue by letting replacing ublk_add_dev with a
> ublk_add_tag_set function that is much more limited in scope and
> instead unwind every single step directly in ublk_ctrl_add_dev.
> To allow for this refactor the device freeing so that there is
> a helper for freeing the device number instead of coupling that
> with freeing the mutex and the memory.
> 
> Note that this now copies the dev_info to userspace before adding
> the character device.  This not only simplifies the erro handling
> in ublk_ctrl_add_dev, but also means that the character device
> can only be seen by userspace if the device addition succeeded.
> 
> Based on a patch from Ming Lei.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Reviewed-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>

Re: [PATCH V2 2/2] ublk_drv: make sure that correct flags(features) returned to userspace

[Christoph Hellwig]

On Fri, Jul 22, 2022 at 03:30:17PM +0800, Ming Lei wrote:
> > > +enum ublk_flag_bits {
> > > +	__UBLK_F_SUPPORT_ZERO_COPY,
> > > +	__UBLK_F_URING_CMD_COMP_IN_TASK,
> > > +	__UBLK_F_NR_BITS,
> > > +};
> > 
> > Please make these #defines so that userspace can detect if they
> > exist in a header using #ifdef.
> 
> userspace is supposed to only use UBLK_F_* instead of __UBLK_F_*, one
> benefit of using enum is that UBLK_F_NR_BITS can be figured out
> automatically, otherwise how can we figure out the max bits?

Oh, indeed.  But I don't think the automatic max index is really
that useful.  Just use a manually maintained in-kernel UBLK_F_ALL,
that is just as little overhead to maintain as the extra enum.
And if you forget to add a flag that code will never see it as it
gets cleared before the device is added.