From 0a26f327e46c203229e72c823dfec71a2b405ec5 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Thu, 5 Jan 2023 12:51:45 -0800 Subject: block: make BLK_DEF_MAX_SECTORS unsigned This is used as an unsigned value, so define it that way to avoid having to cast it. Suggested-by: Christoph Hellwig Signed-off-by: Keith Busch Reviewed-by: Christoph Hellwig Reviewed-by: Bart Van Assche Reviewed-by: Martin K. Petersen Link: https://lore.kernel.org/r/20230105205146.3610282-2-kbusch@meta.com Signed-off-by: Jens Axboe --- drivers/block/null_blk/main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 7d28e3aa406c..4c601ca9552a 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -2123,8 +2123,7 @@ static int null_add_dev(struct nullb_device *dev) blk_queue_physical_block_size(nullb->q, dev->blocksize); if (!dev->max_sectors) dev->max_sectors = queue_max_hw_sectors(nullb->q); - dev->max_sectors = min_t(unsigned int, dev->max_sectors, - BLK_DEF_MAX_SECTORS); + dev->max_sectors = min(dev->max_sectors, BLK_DEF_MAX_SECTORS); blk_queue_max_hw_sectors(nullb->q, dev->max_sectors); if (dev->virt_boundary) -- cgit v1.2.3 From 887b98c74fdf9ab44e93ad9166977cbbb766d2c2 Mon Sep 17 00:00:00 2001 From: Christoph Böhmwalder Date: Fri, 13 Jan 2023 13:35:04 +0100 Subject: drbd: split off drbd_buildtag into separate file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To be more similar to what we do in the out-of-tree module and ease the upstreaming process. Signed-off-by: Christoph Böhmwalder Reviewed-by: Joel Colledge Link: https://lore.kernel.org/r/20230113123506.144082-2-christoph.boehmwalder@linbit.com Signed-off-by: Jens Axboe --- drivers/block/drbd/Makefile | 2 +- drivers/block/drbd/drbd_buildtag.c | 22 ++++++++++++++++++++++ drivers/block/drbd/drbd_main.c | 18 ------------------ 3 files changed, 23 insertions(+), 19 deletions(-) create mode 100644 drivers/block/drbd/drbd_buildtag.c (limited to 'drivers/block') diff --git a/drivers/block/drbd/Makefile b/drivers/block/drbd/Makefile index c93e462130ff..67a8b352a1d5 100644 --- a/drivers/block/drbd/Makefile +++ b/drivers/block/drbd/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0-only -drbd-y := drbd_bitmap.o drbd_proc.o +drbd-y := drbd_buildtag.o drbd_bitmap.o drbd_proc.o drbd-y += drbd_worker.o drbd_receiver.o drbd_req.o drbd_actlog.o drbd-y += drbd_main.o drbd_strings.o drbd_nl.o drbd-y += drbd_interval.o drbd_state.o diff --git a/drivers/block/drbd/drbd_buildtag.c b/drivers/block/drbd/drbd_buildtag.c new file mode 100644 index 000000000000..956a4d5c339b --- /dev/null +++ b/drivers/block/drbd/drbd_buildtag.c @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include +#include + +const char *drbd_buildtag(void) +{ + /* DRBD built from external sources has here a reference to the + * git hash of the source code. + */ + + static char buildtag[38] = "\0uilt-in"; + + if (buildtag[0] == 0) { +#ifdef MODULE + sprintf(buildtag, "srcversion: %-24s", THIS_MODULE->srcversion); +#else + buildtag[0] = 'b'; +#endif + } + + return buildtag; +} diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c index e43dfb9eb6ad..af9309175637 100644 --- a/drivers/block/drbd/drbd_main.c +++ b/drivers/block/drbd/drbd_main.c @@ -3776,24 +3776,6 @@ _drbd_insert_fault(struct drbd_device *device, unsigned int type) } #endif -const char *drbd_buildtag(void) -{ - /* DRBD built from external sources has here a reference to the - git hash of the source code. */ - - static char buildtag[38] = "\0uilt-in"; - - if (buildtag[0] == 0) { -#ifdef MODULE - sprintf(buildtag, "srcversion: %-24s", THIS_MODULE->srcversion); -#else - buildtag[0] = 'b'; -#endif - } - - return buildtag; -} - module_init(drbd_init) module_exit(drbd_cleanup) -- cgit v1.2.3 From 4e2da933b9f19d8098374515ee0984a20202e674 Mon Sep 17 00:00:00 2001 From: Christoph Böhmwalder Date: Fri, 13 Jan 2023 13:35:05 +0100 Subject: drbd: drop API_VERSION define MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use the genetlink api version as defined in drbd_genl_api.h. Signed-off-by: Christoph Böhmwalder Reviewed-by: Joel Colledge Link: https://lore.kernel.org/r/20230113123506.144082-3-christoph.boehmwalder@linbit.com Signed-off-by: Jens Axboe --- drivers/block/drbd/drbd_debugfs.c | 2 +- drivers/block/drbd/drbd_main.c | 2 +- drivers/block/drbd/drbd_proc.c | 2 +- include/linux/drbd.h | 1 - include/linux/drbd_genl_api.h | 2 +- 5 files changed, 4 insertions(+), 5 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/drbd/drbd_debugfs.c b/drivers/block/drbd/drbd_debugfs.c index a72c096aa5b1..12460b584bcb 100644 --- a/drivers/block/drbd/drbd_debugfs.c +++ b/drivers/block/drbd/drbd_debugfs.c @@ -844,7 +844,7 @@ static int drbd_version_show(struct seq_file *m, void *ignored) { seq_printf(m, "# %s\n", drbd_buildtag()); seq_printf(m, "VERSION=%s\n", REL_VERSION); - seq_printf(m, "API_VERSION=%u\n", API_VERSION); + seq_printf(m, "API_VERSION=%u\n", GENL_MAGIC_VERSION); seq_printf(m, "PRO_VERSION_MIN=%u\n", PRO_VERSION_MIN); seq_printf(m, "PRO_VERSION_MAX=%u\n", PRO_VERSION_MAX); return 0; diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c index af9309175637..2c764f7ee4a7 100644 --- a/drivers/block/drbd/drbd_main.c +++ b/drivers/block/drbd/drbd_main.c @@ -2899,7 +2899,7 @@ static int __init drbd_init(void) pr_info("initialized. " "Version: " REL_VERSION " (api:%d/proto:%d-%d)\n", - API_VERSION, PRO_VERSION_MIN, PRO_VERSION_MAX); + GENL_MAGIC_VERSION, PRO_VERSION_MIN, PRO_VERSION_MAX); pr_info("%s\n", drbd_buildtag()); pr_info("registered as block device major %d\n", DRBD_MAJOR); return 0; /* Success! */ diff --git a/drivers/block/drbd/drbd_proc.c b/drivers/block/drbd/drbd_proc.c index 2227fb0db1ce..1d0feafceadc 100644 --- a/drivers/block/drbd/drbd_proc.c +++ b/drivers/block/drbd/drbd_proc.c @@ -228,7 +228,7 @@ int drbd_seq_show(struct seq_file *seq, void *v) }; seq_printf(seq, "version: " REL_VERSION " (api:%d/proto:%d-%d)\n%s\n", - API_VERSION, PRO_VERSION_MIN, PRO_VERSION_MAX, drbd_buildtag()); + GENL_MAGIC_VERSION, PRO_VERSION_MIN, PRO_VERSION_MAX, drbd_buildtag()); /* cs .. connection state diff --git a/include/linux/drbd.h b/include/linux/drbd.h index 5755537b51b1..df65a8f5228a 100644 --- a/include/linux/drbd.h +++ b/include/linux/drbd.h @@ -40,7 +40,6 @@ extern const char *drbd_buildtag(void); #define REL_VERSION "8.4.11" -#define API_VERSION 1 #define PRO_VERSION_MIN 86 #define PRO_VERSION_MAX 101 diff --git a/include/linux/drbd_genl_api.h b/include/linux/drbd_genl_api.h index bd62efc29002..70682c058027 100644 --- a/include/linux/drbd_genl_api.h +++ b/include/linux/drbd_genl_api.h @@ -47,7 +47,7 @@ enum drbd_state_info_bcast_reason { #undef linux #include -#define GENL_MAGIC_VERSION API_VERSION +#define GENL_MAGIC_VERSION 1 #define GENL_MAGIC_FAMILY drbd #define GENL_MAGIC_FAMILY_HDRSZ sizeof(struct drbd_genlmsghdr) #define GENL_MAGIC_INCLUDE_FILE -- cgit v1.2.3 From 20f2a34a421b1716b96d1e34d4f4948bf4b4ba1e Mon Sep 17 00:00:00 2001 From: Christoph Böhmwalder Date: Fri, 13 Jan 2023 13:35:06 +0100 Subject: drbd: split off drbd_config into separate file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To be more similar to what we do in the out-of-tree module and ease the upstreaming process. Signed-off-by: Christoph Böhmwalder Reviewed-by: Joel Colledge Link: https://lore.kernel.org/r/20230113123506.144082-4-christoph.boehmwalder@linbit.com Signed-off-by: Jens Axboe --- drivers/block/drbd/drbd_buildtag.c | 2 +- drivers/block/drbd/drbd_int.h | 1 + include/linux/drbd.h | 6 ------ include/linux/drbd_config.h | 16 ++++++++++++++++ 4 files changed, 18 insertions(+), 7 deletions(-) create mode 100644 include/linux/drbd_config.h (limited to 'drivers/block') diff --git a/drivers/block/drbd/drbd_buildtag.c b/drivers/block/drbd/drbd_buildtag.c index 956a4d5c339b..cb1aa66d7d5d 100644 --- a/drivers/block/drbd/drbd_buildtag.c +++ b/drivers/block/drbd/drbd_buildtag.c @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL-2.0-only -#include +#include #include const char *drbd_buildtag(void) diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h index ae713338aa46..67b4e86634ec 100644 --- a/drivers/block/drbd/drbd_int.h +++ b/drivers/block/drbd/drbd_int.h @@ -34,6 +34,7 @@ #include #include #include +#include #include "drbd_strings.h" #include "drbd_state.h" #include "drbd_protocol.h" diff --git a/include/linux/drbd.h b/include/linux/drbd.h index df65a8f5228a..5468a2399d48 100644 --- a/include/linux/drbd.h +++ b/include/linux/drbd.h @@ -38,12 +38,6 @@ #endif -extern const char *drbd_buildtag(void); -#define REL_VERSION "8.4.11" -#define PRO_VERSION_MIN 86 -#define PRO_VERSION_MAX 101 - - enum drbd_io_error_p { EP_PASS_ON, /* FIXME should the better be named "Ignore"? */ EP_CALL_HELPER, diff --git a/include/linux/drbd_config.h b/include/linux/drbd_config.h new file mode 100644 index 000000000000..d215365c6bb1 --- /dev/null +++ b/include/linux/drbd_config.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * drbd_config.h + * DRBD's compile time configuration. + */ + +#ifndef DRBD_CONFIG_H +#define DRBD_CONFIG_H + +extern const char *drbd_buildtag(void); + +#define REL_VERSION "8.4.11" +#define PRO_VERSION_MIN 86 +#define PRO_VERSION_MAX 101 + +#endif -- cgit v1.2.3 From 069182007d1ad05b6aaadd9f3864c33b279e2685 Mon Sep 17 00:00:00 2001 From: Christoph Böhmwalder Date: Fri, 13 Jan 2023 13:35:34 +0100 Subject: drbd: remove unnecessary assignment in vli_encode_bits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Christoph Böhmwalder Reviewed-by: Joel Colledge Link: https://lore.kernel.org/r/20230113123538.144276-5-christoph.boehmwalder@linbit.com Signed-off-by: Jens Axboe --- drivers/block/drbd/drbd_vli.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/block') diff --git a/drivers/block/drbd/drbd_vli.h b/drivers/block/drbd/drbd_vli.h index 1ee81e3c2152..941c511cc4da 100644 --- a/drivers/block/drbd/drbd_vli.h +++ b/drivers/block/drbd/drbd_vli.h @@ -327,7 +327,7 @@ static inline int bitstream_get_bits(struct bitstream *bs, u64 *out, int bits) */ static inline int vli_encode_bits(struct bitstream *bs, u64 in) { - u64 code = code; + u64 code; int bits = __vli_encode_bits(&code, in); if (bits <= 0) -- cgit v1.2.3 From 9cf766a457995a95d7f66d78cf749d05067d68a4 Mon Sep 17 00:00:00 2001 From: Christoph Böhmwalder Date: Fri, 13 Jan 2023 13:35:35 +0100 Subject: drbd: remove macros using require_context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This require_context attribute originated in a proposed sparse patch by Philipp Reisner back in 2008. Johannes Berg had a different solution to a similar problem, and that patch "won" in the end; so the require_context thing never got merged. The whole history can be read at [0]. DRBD kept using these annotations anyway for a while. Nowadays, on a modern unmodified sparse, they obviously do nothing, and they are hardly used anymore anyway. So, just remove the definitions of these macros. [0] https://www.spinics.net/lists/linux-sparse/msg01150.html Signed-off-by: Christoph Böhmwalder Reviewed-by: Joel Colledge Link: https://lore.kernel.org/r/20230113123538.144276-6-christoph.boehmwalder@linbit.com Signed-off-by: Jens Axboe --- drivers/block/drbd/drbd_int.h | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h index 67b4e86634ec..d89b7d03d4c8 100644 --- a/drivers/block/drbd/drbd_int.h +++ b/drivers/block/drbd/drbd_int.h @@ -40,16 +40,6 @@ #include "drbd_protocol.h" #include "drbd_polymorph_printk.h" -#ifdef __CHECKER__ -# define __protected_by(x) __attribute__((require_context(x,1,999,"rdwr"))) -# define __protected_read_by(x) __attribute__((require_context(x,1,999,"read"))) -# define __protected_write_by(x) __attribute__((require_context(x,1,999,"write"))) -#else -# define __protected_by(x) -# define __protected_read_by(x) -# define __protected_write_by(x) -#endif - /* shared module parameters, defined in drbd_main.c */ #ifdef CONFIG_DRBD_FAULT_INJECTION extern int drbd_enable_faults; @@ -775,7 +765,7 @@ struct drbd_device { unsigned long flags; /* configured by drbdsetup */ - struct drbd_backing_dev *ldev __protected_by(local); + struct drbd_backing_dev *ldev; sector_t p_size; /* partner's disk size */ struct request_queue *rq_queue; -- cgit v1.2.3 From 2990ca29f36171e052ea42d8464ec2e21cf4485a Mon Sep 17 00:00:00 2001 From: Lars Ellenberg Date: Fri, 13 Jan 2023 13:35:37 +0100 Subject: drbd: interval tree: make removing an "empty" interval a no-op MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trying to remove an "empty" (just initialized, or "cleared") interval from the tree, this results in an endless loop. As we typically protect the tree with a spinlock_irq, the result is a hung system. Be nice to error cleanup code paths, ignore removal of empty intervals. Signed-off-by: Lars Ellenberg Signed-off-by: Christoph Böhmwalder Link: https://lore.kernel.org/r/20230113123538.144276-8-christoph.boehmwalder@linbit.com Signed-off-by: Jens Axboe --- drivers/block/drbd/drbd_interval.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'drivers/block') diff --git a/drivers/block/drbd/drbd_interval.c b/drivers/block/drbd/drbd_interval.c index 5024ffd6143d..b6aaf0d4d85b 100644 --- a/drivers/block/drbd/drbd_interval.c +++ b/drivers/block/drbd/drbd_interval.c @@ -95,6 +95,10 @@ drbd_contains_interval(struct rb_root *root, sector_t sector, void drbd_remove_interval(struct rb_root *root, struct drbd_interval *this) { + /* avoid endless loop */ + if (drbd_interval_empty(this)) + return; + rb_erase_augmented(&this->rb, root, &augment_callbacks); } -- cgit v1.2.3 From 2bb34fa6ff4183b42c397866ec2443ab5eabc280 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Fri, 13 Jan 2023 13:35:38 +0100 Subject: drbd: drbd_insert_interval(): Clarify comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Andreas Gruenbacher Signed-off-by: Christoph Böhmwalder Link: https://lore.kernel.org/r/20230113123538.144276-9-christoph.boehmwalder@linbit.com Signed-off-by: Jens Axboe --- drivers/block/drbd/drbd_interval.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/block') diff --git a/drivers/block/drbd/drbd_interval.c b/drivers/block/drbd/drbd_interval.c index b6aaf0d4d85b..873beda6de24 100644 --- a/drivers/block/drbd/drbd_interval.c +++ b/drivers/block/drbd/drbd_interval.c @@ -58,7 +58,7 @@ drbd_insert_interval(struct rb_root *root, struct drbd_interval *this) * drbd_contains_interval - check if a tree contains a given interval * @root: red black tree root * @sector: start sector of @interval - * @interval: may not be a valid pointer + * @interval: may be an invalid pointer * * Returns if the tree contains the node @interval with start sector @start. * Does not dereference @interval until @interval is known to be a valid object -- cgit v1.2.3 From ed878d1c1c641c4a6bd366658fc8e6bc842b80d1 Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Fri, 6 Jan 2023 12:17:06 +0800 Subject: ublk_drv: remove nr_aborted_queues from ublk_device No one uses 'nr_aborted_queues' any more, so remove it. Reviewed-by: ZiyangZhang Signed-off-by: Ming Lei Link: https://lore.kernel.org/r/20230106041711.914434-2-ming.lei@redhat.com Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 1 - 1 file changed, 1 deletion(-) (limited to 'drivers/block') diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index e54693204630..4232089e3723 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -159,7 +159,6 @@ struct ublk_device { struct completion completion; unsigned int nr_queues_ready; - atomic_t nr_aborted_queues; /* * Our ubq->daemon may be killed without any notification, so -- cgit v1.2.3 From 73a166d9749230d598320fdae3b687cdc0e2e205 Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Fri, 6 Jan 2023 12:17:07 +0800 Subject: ublk_drv: don't probe partitions if the ubq daemon isn't trusted If any ubq daemon is unprivileged, the ublk char device is allowed for unprivileged user actually, and we can't trust the current user, so not probe partitions. Fixes: 71f28f3136af ("ublk_drv: add io_uring based userspace block driver") Reviewed-by: ZiyangZhang Signed-off-by: Ming Lei Link: https://lore.kernel.org/r/20230106041711.914434-3-ming.lei@redhat.com Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'drivers/block') diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 4232089e3723..8a6f38cc62db 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -159,6 +159,7 @@ struct ublk_device { struct completion completion; unsigned int nr_queues_ready; + unsigned int nr_privileged_daemon; /* * Our ubq->daemon may be killed without any notification, so @@ -1178,6 +1179,9 @@ static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq) ubq->ubq_daemon = current; get_task_struct(ubq->ubq_daemon); ub->nr_queues_ready++; + + if (capable(CAP_SYS_ADMIN)) + ub->nr_privileged_daemon++; } if (ub->nr_queues_ready == ub->dev_info.nr_hw_queues) complete_all(&ub->completion); @@ -1534,6 +1538,10 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd) if (ret) goto out_put_disk; + /* don't probe partitions if any one ubq daemon is un-trusted */ + if (ub->nr_privileged_daemon != ub->nr_queues_ready) + set_bit(GD_SUPPRESS_PART_SCAN, &disk->state); + get_device(&ub->cdev_dev); ret = add_disk(disk); if (ret) { @@ -1935,6 +1943,7 @@ static int ublk_ctrl_start_recovery(struct io_uring_cmd *cmd) /* set to NULL, otherwise new ubq_daemon cannot mmap the io_cmd_buf */ ub->mm = NULL; ub->nr_queues_ready = 0; + ub->nr_privileged_daemon = 0; init_completion(&ub->completion); ret = 0; out_unlock: -- cgit v1.2.3 From bfbcef036396a73fbf4b3fee385cc670159df5ad Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Fri, 6 Jan 2023 12:17:08 +0800 Subject: ublk_drv: move ublk_get_device_from_id into ublk_ctrl_uring_cmd It is annoying for each control command handler to get/put ublk device and deal with failure. Control command handler is simplified a lot by moving ublk_get_device_from_id into ublk_ctrl_uring_cmd(). Reviewed-by: ZiyangZhang Signed-off-by: Ming Lei Link: https://lore.kernel.org/r/20230106041711.914434-4-ming.lei@redhat.com Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 138 +++++++++++++++++------------------------------ 1 file changed, 49 insertions(+), 89 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 8a6f38cc62db..b015e46b59bb 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -1496,21 +1496,16 @@ static struct ublk_device *ublk_get_device_from_id(int idx) return ub; } -static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd) +static int ublk_ctrl_start_dev(struct ublk_device *ub, struct io_uring_cmd *cmd) { struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; int ublksrv_pid = (int)header->data[0]; - struct ublk_device *ub; struct gendisk *disk; int ret = -EINVAL; if (ublksrv_pid <= 0) return -EINVAL; - ub = ublk_get_device_from_id(header->dev_id); - if (!ub) - return -EINVAL; - wait_for_completion_interruptible(&ub->completion); schedule_delayed_work(&ub->monitor_work, UBLK_DAEMON_MONITOR_PERIOD); @@ -1559,21 +1554,20 @@ out_put_disk: put_disk(disk); out_unlock: mutex_unlock(&ub->mutex); - ublk_put_device(ub); return ret; } -static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd) +static int ublk_ctrl_get_queue_affinity(struct ublk_device *ub, + struct io_uring_cmd *cmd) { struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; void __user *argp = (void __user *)(unsigned long)header->addr; - struct ublk_device *ub; cpumask_var_t cpumask; unsigned long queue; unsigned int retlen; unsigned int i; - int ret = -EINVAL; - + int ret; + if (header->len * BITS_PER_BYTE < nr_cpu_ids) return -EINVAL; if (header->len & (sizeof(unsigned long)-1)) @@ -1581,17 +1575,12 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd) if (!header->addr) return -EINVAL; - ub = ublk_get_device_from_id(header->dev_id); - if (!ub) - return -EINVAL; - queue = header->data[0]; if (queue >= ub->dev_info.nr_hw_queues) - goto out_put_device; + return -EINVAL; - ret = -ENOMEM; if (!zalloc_cpumask_var(&cpumask, GFP_KERNEL)) - goto out_put_device; + return -ENOMEM; for_each_possible_cpu(i) { if (ub->tag_set.map[HCTX_TYPE_DEFAULT].mq_map[i] == queue) @@ -1609,8 +1598,6 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd) ret = 0; out_free_cpumask: free_cpumask_var(cpumask); -out_put_device: - ublk_put_device(ub); return ret; } @@ -1731,30 +1718,27 @@ static inline bool ublk_idr_freed(int id) return ptr == NULL; } -static int ublk_ctrl_del_dev(int idx) +static int ublk_ctrl_del_dev(struct ublk_device **p_ub) { - struct ublk_device *ub; + struct ublk_device *ub = *p_ub; + int idx = ub->ub_number; int ret; ret = mutex_lock_killable(&ublk_ctl_mutex); if (ret) return ret; - ub = ublk_get_device_from_id(idx); - if (ub) { - ublk_remove(ub); - ublk_put_device(ub); - ret = 0; - } else { - ret = -ENODEV; - } + ublk_remove(ub); + + /* Mark the reference as consumed */ + *p_ub = NULL; + ublk_put_device(ub); /* * Wait until the idr is removed, then it can be reused after * DEL_DEV command is returned. */ - if (!ret) - wait_event(ublk_idr_wq, ublk_idr_freed(idx)); + wait_event(ublk_idr_wq, ublk_idr_freed(idx)); mutex_unlock(&ublk_ctl_mutex); return ret; @@ -1769,50 +1753,36 @@ static inline void ublk_ctrl_cmd_dump(struct io_uring_cmd *cmd) header->data[0], header->addr, header->len); } -static int ublk_ctrl_stop_dev(struct io_uring_cmd *cmd) +static int ublk_ctrl_stop_dev(struct ublk_device *ub) { - struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; - struct ublk_device *ub; - - ub = ublk_get_device_from_id(header->dev_id); - if (!ub) - return -EINVAL; - ublk_stop_dev(ub); cancel_work_sync(&ub->stop_work); cancel_work_sync(&ub->quiesce_work); - ublk_put_device(ub); return 0; } -static int ublk_ctrl_get_dev_info(struct io_uring_cmd *cmd) +static int ublk_ctrl_get_dev_info(struct ublk_device *ub, + struct io_uring_cmd *cmd) { struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; void __user *argp = (void __user *)(unsigned long)header->addr; - struct ublk_device *ub; - int ret = 0; if (header->len < sizeof(struct ublksrv_ctrl_dev_info) || !header->addr) return -EINVAL; - ub = ublk_get_device_from_id(header->dev_id); - if (!ub) - return -EINVAL; - if (copy_to_user(argp, &ub->dev_info, sizeof(ub->dev_info))) - ret = -EFAULT; - ublk_put_device(ub); + return -EFAULT; - return ret; + return 0; } -static int ublk_ctrl_get_params(struct io_uring_cmd *cmd) +static int ublk_ctrl_get_params(struct ublk_device *ub, + struct io_uring_cmd *cmd) { struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; void __user *argp = (void __user *)(unsigned long)header->addr; struct ublk_params_header ph; - struct ublk_device *ub; int ret; if (header->len <= sizeof(ph) || !header->addr) @@ -1827,10 +1797,6 @@ static int ublk_ctrl_get_params(struct io_uring_cmd *cmd) if (ph.len > sizeof(struct ublk_params)) ph.len = sizeof(struct ublk_params); - ub = ublk_get_device_from_id(header->dev_id); - if (!ub) - return -EINVAL; - mutex_lock(&ub->mutex); if (copy_to_user(argp, &ub->params, ph.len)) ret = -EFAULT; @@ -1838,16 +1804,15 @@ static int ublk_ctrl_get_params(struct io_uring_cmd *cmd) ret = 0; mutex_unlock(&ub->mutex); - ublk_put_device(ub); return ret; } -static int ublk_ctrl_set_params(struct io_uring_cmd *cmd) +static int ublk_ctrl_set_params(struct ublk_device *ub, + struct io_uring_cmd *cmd) { struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; void __user *argp = (void __user *)(unsigned long)header->addr; struct ublk_params_header ph; - struct ublk_device *ub; int ret = -EFAULT; if (header->len <= sizeof(ph) || !header->addr) @@ -1862,10 +1827,6 @@ static int ublk_ctrl_set_params(struct io_uring_cmd *cmd) if (ph.len > sizeof(struct ublk_params)) ph.len = sizeof(struct ublk_params); - ub = ublk_get_device_from_id(header->dev_id); - if (!ub) - return -EINVAL; - /* parameters can only be changed when device isn't live */ mutex_lock(&ub->mutex); if (ub->dev_info.state == UBLK_S_DEV_LIVE) { @@ -1878,7 +1839,6 @@ static int ublk_ctrl_set_params(struct io_uring_cmd *cmd) ret = ublk_validate_params(ub); } mutex_unlock(&ub->mutex); - ublk_put_device(ub); return ret; } @@ -1905,17 +1865,13 @@ static void ublk_queue_reinit(struct ublk_device *ub, struct ublk_queue *ubq) } } -static int ublk_ctrl_start_recovery(struct io_uring_cmd *cmd) +static int ublk_ctrl_start_recovery(struct ublk_device *ub, + struct io_uring_cmd *cmd) { struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; - struct ublk_device *ub; int ret = -EINVAL; int i; - ub = ublk_get_device_from_id(header->dev_id); - if (!ub) - return ret; - mutex_lock(&ub->mutex); if (!ublk_can_use_recovery(ub)) goto out_unlock; @@ -1948,21 +1904,16 @@ static int ublk_ctrl_start_recovery(struct io_uring_cmd *cmd) ret = 0; out_unlock: mutex_unlock(&ub->mutex); - ublk_put_device(ub); return ret; } -static int ublk_ctrl_end_recovery(struct io_uring_cmd *cmd) +static int ublk_ctrl_end_recovery(struct ublk_device *ub, + struct io_uring_cmd *cmd) { struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; int ublksrv_pid = (int)header->data[0]; - struct ublk_device *ub; int ret = -EINVAL; - ub = ublk_get_device_from_id(header->dev_id); - if (!ub) - return ret; - pr_devel("%s: Waiting for new ubq_daemons(nr: %d) are ready, dev id %d...\n", __func__, ub->dev_info.nr_hw_queues, header->dev_id); /* wait until new ubq_daemon sending all FETCH_REQ */ @@ -1990,7 +1941,6 @@ static int ublk_ctrl_end_recovery(struct io_uring_cmd *cmd) ret = 0; out_unlock: mutex_unlock(&ub->mutex); - ublk_put_device(ub); return ret; } @@ -1998,6 +1948,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) { struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; + struct ublk_device *ub = NULL; int ret = -EINVAL; if (issue_flags & IO_URING_F_NONBLOCK) @@ -2012,41 +1963,50 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, if (!capable(CAP_SYS_ADMIN)) goto out; - ret = -ENODEV; + if (cmd->cmd_op != UBLK_CMD_ADD_DEV) { + ret = -ENODEV; + ub = ublk_get_device_from_id(header->dev_id); + if (!ub) + goto out; + } + switch (cmd->cmd_op) { case UBLK_CMD_START_DEV: - ret = ublk_ctrl_start_dev(cmd); + ret = ublk_ctrl_start_dev(ub, cmd); break; case UBLK_CMD_STOP_DEV: - ret = ublk_ctrl_stop_dev(cmd); + ret = ublk_ctrl_stop_dev(ub); break; case UBLK_CMD_GET_DEV_INFO: - ret = ublk_ctrl_get_dev_info(cmd); + ret = ublk_ctrl_get_dev_info(ub, cmd); break; case UBLK_CMD_ADD_DEV: ret = ublk_ctrl_add_dev(cmd); break; case UBLK_CMD_DEL_DEV: - ret = ublk_ctrl_del_dev(header->dev_id); + ret = ublk_ctrl_del_dev(&ub); break; case UBLK_CMD_GET_QUEUE_AFFINITY: - ret = ublk_ctrl_get_queue_affinity(cmd); + ret = ublk_ctrl_get_queue_affinity(ub, cmd); break; case UBLK_CMD_GET_PARAMS: - ret = ublk_ctrl_get_params(cmd); + ret = ublk_ctrl_get_params(ub, cmd); break; case UBLK_CMD_SET_PARAMS: - ret = ublk_ctrl_set_params(cmd); + ret = ublk_ctrl_set_params(ub, cmd); break; case UBLK_CMD_START_USER_RECOVERY: - ret = ublk_ctrl_start_recovery(cmd); + ret = ublk_ctrl_start_recovery(ub, cmd); break; case UBLK_CMD_END_USER_RECOVERY: - ret = ublk_ctrl_end_recovery(cmd); + ret = ublk_ctrl_end_recovery(ub, cmd); break; default: + ret = -ENOTSUPP; break; } + if (ub) + ublk_put_device(ub); out: io_uring_cmd_done(cmd, ret, 0); pr_devel("%s: cmd done ret %d cmd_op %x, dev id %d qid %d\n", -- cgit v1.2.3 From abb864d380854b5427b6b070beb2ebc291ce4d1e Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Fri, 6 Jan 2023 12:17:09 +0800 Subject: ublk_drv: add device parameter UBLK_PARAM_TYPE_DEVT Userspace side only knows device ID, but the associated path of ublkc* and ublkb* could be changed by udev, and that depends on userspace's policy, so add parameter of UBLK_PARAM_TYPE_DEVT for retrieving major/minor of the ublkc* and ublkb*, then user may figure out major/minor of the ublk disks he/she owns. With major/minor, it is easy to find the device node path. Signed-off-by: Ming Lei Link: https://lore.kernel.org/r/20230106041711.914434-5-ming.lei@redhat.com Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 24 +++++++++++++++++++++++- include/uapi/linux/ublk_cmd.h | 13 +++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) (limited to 'drivers/block') diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index b015e46b59bb..75033304b900 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -54,7 +54,8 @@ | UBLK_F_USER_RECOVERY_REISSUE) /* All UBLK_PARAM_TYPE_* should be included here */ -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD) +#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | \ + UBLK_PARAM_TYPE_DISCARD | UBLK_PARAM_TYPE_DEVT) struct ublk_rq_data { struct llist_node node; @@ -255,6 +256,10 @@ static int ublk_validate_params(const struct ublk_device *ub) return -EINVAL; } + /* dev_t is read-only */ + if (ub->params.types & UBLK_PARAM_TYPE_DEVT) + return -EINVAL; + return 0; } @@ -1777,6 +1782,22 @@ static int ublk_ctrl_get_dev_info(struct ublk_device *ub, return 0; } +/* TYPE_DEVT is readonly, so fill it up before returning to userspace */ +static void ublk_ctrl_fill_params_devt(struct ublk_device *ub) +{ + ub->params.devt.char_major = MAJOR(ub->cdev_dev.devt); + ub->params.devt.char_minor = MINOR(ub->cdev_dev.devt); + + if (ub->ub_disk) { + ub->params.devt.disk_major = MAJOR(disk_devt(ub->ub_disk)); + ub->params.devt.disk_minor = MINOR(disk_devt(ub->ub_disk)); + } else { + ub->params.devt.disk_major = 0; + ub->params.devt.disk_minor = 0; + } + ub->params.types |= UBLK_PARAM_TYPE_DEVT; +} + static int ublk_ctrl_get_params(struct ublk_device *ub, struct io_uring_cmd *cmd) { @@ -1798,6 +1819,7 @@ static int ublk_ctrl_get_params(struct ublk_device *ub, ph.len = sizeof(struct ublk_params); mutex_lock(&ub->mutex); + ublk_ctrl_fill_params_devt(ub); if (copy_to_user(argp, &ub->params, ph.len)) ret = -EFAULT; else diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h index 8f88e3a29998..4e38b9aa0293 100644 --- a/include/uapi/linux/ublk_cmd.h +++ b/include/uapi/linux/ublk_cmd.h @@ -214,6 +214,17 @@ struct ublk_param_discard { __u16 reserved0; }; +/* + * read-only, can't set via UBLK_CMD_SET_PARAMS, disk_devt is available + * after device is started + */ +struct ublk_param_devt { + __u32 char_major; + __u32 char_minor; + __u32 disk_major; + __u32 disk_minor; +}; + struct ublk_params { /* * Total length of parameters, userspace has to set 'len' for both @@ -224,10 +235,12 @@ struct ublk_params { __u32 len; #define UBLK_PARAM_TYPE_BASIC (1 << 0) #define UBLK_PARAM_TYPE_DISCARD (1 << 1) +#define UBLK_PARAM_TYPE_DEVT (1 << 2) __u32 types; /* types of parameter included */ struct ublk_param_basic basic; struct ublk_param_discard discard; + struct ublk_param_devt devt; }; #endif -- cgit v1.2.3 From 403ebc877832752da9fc851284fa00ceca7b2fae Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Fri, 6 Jan 2023 12:17:10 +0800 Subject: ublk_drv: add module parameter of ublks_max for limiting max allowed ublk dev Prepare for supporting unprivileged ublk device by limiting max number ublk devices added. Otherwise too many ublk devices could be added by un-trusted user, which can be thought as one DoS. Reviewed-by: ZiyangZhang Signed-off-by: Ming Lei Link: https://lore.kernel.org/r/20230106041711.914434-6-ming.lei@redhat.com Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) (limited to 'drivers/block') diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 75033304b900..883f7974d105 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -186,6 +186,15 @@ static wait_queue_head_t ublk_idr_wq; /* wait until one idr is freed */ static DEFINE_MUTEX(ublk_ctl_mutex); +/* + * Max ublk devices allowed to add + * + * It can be extended to one per-user limit in future or even controlled + * by cgroup. + */ +static unsigned int ublks_max = 64; +static unsigned int ublks_added; /* protected by ublk_ctl_mutex */ + static struct miscdevice ublk_misc; static void ublk_dev_param_basic_apply(struct ublk_device *ub) @@ -1441,6 +1450,8 @@ static int ublk_add_chdev(struct ublk_device *ub) ret = cdev_device_add(&ub->cdev, dev); if (ret) goto fail; + + ublks_added++; return 0; fail: put_device(dev); @@ -1483,6 +1494,7 @@ static void ublk_remove(struct ublk_device *ub) cancel_work_sync(&ub->quiesce_work); cdev_device_del(&ub->cdev, &ub->cdev_dev); put_device(&ub->cdev_dev); + ublks_added--; } static struct ublk_device *ublk_get_device_from_id(int idx) @@ -1642,6 +1654,10 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) if (ret) return ret; + ret = -EACCES; + if (ublks_added >= ublks_max) + goto out_unlock; + ret = -ENOMEM; ub = kzalloc(sizeof(*ub), GFP_KERNEL); if (!ub) @@ -2095,5 +2111,8 @@ static void __exit ublk_exit(void) module_init(ublk_init); module_exit(ublk_exit); +module_param(ublks_max, int, 0444); +MODULE_PARM_DESC(ublks_max, "max number of ublk devices allowed to add(default: 64)"); + MODULE_AUTHOR("Ming Lei "); MODULE_LICENSE("GPL"); -- cgit v1.2.3 From 4093cb5a06343ea3936ae46664d132c82576b153 Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Fri, 6 Jan 2023 12:17:11 +0800 Subject: ublk_drv: add mechanism for supporting unprivileged ublk device unprivileged ublk device is helpful for container use case, such as: ublk device created in one unprivileged container can be controlled and accessed by this container only. Implement this feature by adding flag of UBLK_F_UNPRIVILEGED_DEV, and if this flag isn't set, any control command has been run from privileged user. Otherwise, any control command can be sent from any unprivileged user, but the user has to be permitted to access the ublk char device to be controlled. In case of UBLK_F_UNPRIVILEGED_DEV: 1) for command UBLK_CMD_ADD_DEV, it is always allowed, and user needs to provide owner's uid/gid in this command, so that udev can set correct ownership for the created ublk device, since the device owner uid/gid can be queried via command of UBLK_CMD_GET_DEV_INFO. 2) for other control commands, they can only be run successfully if the current user is allowed to access the specified ublk char device, for running the permission check, path of the ublk char device has to be provided by these commands. Also add one control of command UBLK_CMD_GET_DEV_INFO2 which always include the char dev path in payload since userspace may not have knowledge if this device is created in unprivileged mode. For applying this mechanism, system administrator needs to take the following policies: 1) chmod 0666 /dev/ublk-control 2) change ownership of ublkcN & ublkbN - chown owner_uid:owner_gid /dev/ublkcN - chown owner_uid:owner_gid /dev/ublkbN Both can be done via one simple udev rule. Userspace: https://github.com/ming1/ubdsrv/tree/unprivileged-ublk 'ublk add -t $TYPE --un_privileged=1' is for creating one un-privileged ublk device if the user is un-privileged. Link: https://lore.kernel.org/linux-block/YoOr6jBfgVm8GvWg@stefanha-x1.localdomain/ Suggested-by: Stefan Hajnoczi Signed-off-by: Ming Lei Link: https://lore.kernel.org/r/20230106041711.914434-7-ming.lei@redhat.com Signed-off-by: Jens Axboe --- Documentation/block/ublk.rst | 49 +++++++++++--- drivers/block/ublk_drv.c | 152 ++++++++++++++++++++++++++++++++++++++++-- include/uapi/linux/ublk_cmd.h | 36 +++++++++- 3 files changed, 220 insertions(+), 17 deletions(-) (limited to 'drivers/block') diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst index ba45c46cc0da..2916fcf3ab44 100644 --- a/Documentation/block/ublk.rst +++ b/Documentation/block/ublk.rst @@ -144,6 +144,37 @@ managing and controlling ublk devices with help of several control commands: For retrieving device info via ``ublksrv_ctrl_dev_info``. It is the server's responsibility to save IO target specific info in userspace. +- ``UBLK_CMD_GET_DEV_INFO2`` + Same purpose with ``UBLK_CMD_GET_DEV_INFO``, but ublk server has to + provide path of the char device of ``/dev/ublkc*`` for kernel to run + permission check, and this command is added for supporting unprivileged + ublk device, and introduced with ``UBLK_F_UNPRIVILEGED_DEV`` together. + Only the user owning the requested device can retrieve the device info. + + How to deal with userspace/kernel compatibility: + + 1) if kernel is capable of handling ``UBLK_F_UNPRIVILEGED_DEV`` + If ublk server supports ``UBLK_F_UNPRIVILEGED_DEV``: + ublk server should send ``UBLK_CMD_GET_DEV_INFO2``, given anytime + unprivileged application needs to query devices the current user owns, + when the application has no idea if ``UBLK_F_UNPRIVILEGED_DEV`` is set + given the capability info is stateless, and application should always + retrieve it via ``UBLK_CMD_GET_DEV_INFO2`` + + If ublk server doesn't support ``UBLK_F_UNPRIVILEGED_DEV``: + ``UBLK_CMD_GET_DEV_INFO`` is always sent to kernel, and the feature of + UBLK_F_UNPRIVILEGED_DEV isn't available for user + + 2) if kernel isn't capable of handling ``UBLK_F_UNPRIVILEGED_DEV`` + If ublk server supports ``UBLK_F_UNPRIVILEGED_DEV``: + ``UBLK_CMD_GET_DEV_INFO2`` is tried first, and will be failed, then + ``UBLK_CMD_GET_DEV_INFO`` needs to be retried given + ``UBLK_F_UNPRIVILEGED_DEV`` can't be set + + If ublk server doesn't support ``UBLK_F_UNPRIVILEGED_DEV``: + ``UBLK_CMD_GET_DEV_INFO`` is always sent to kernel, and the feature of + ``UBLK_F_UNPRIVILEGED_DEV`` isn't available for user + - ``UBLK_CMD_START_USER_RECOVERY`` This command is valid if ``UBLK_F_USER_RECOVERY`` feature is enabled. This @@ -180,6 +211,15 @@ managing and controlling ublk devices with help of several control commands: double-write since the driver may issue the same I/O request twice. It might be useful to a read-only FS or a VM backend. +Unprivileged ublk device is supported by passing ``UBLK_F_UNPRIVILEGED_DEV``. +Once the flag is set, all control commands can be sent by unprivileged +user. Except for command of ``UBLK_CMD_ADD_DEV``, permission check on +the specified char device(``/dev/ublkc*``) is done for all other control +commands by ublk driver, for doing that, path of the char device has to +be provided in these commands' payload from ublk server. With this way, +ublk device becomes container-ware, and device created in one container +can be controlled/accessed just inside this container. + Data plane ---------- @@ -254,15 +294,6 @@ with specified IO tag in the command data: Future development ================== -Container-aware ublk deivice ----------------------------- - -ublk driver doesn't handle any IO logic. Its function is well defined -for now and very limited userspace interfaces are needed, which is also -well defined too. It is possible to make ublk devices container-aware block -devices in future as Stefan Hajnoczi suggested [#stefan]_, by removing -ADMIN privilege. - Zero copy --------- diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 883f7974d105..a725a236a38f 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -42,6 +42,7 @@ #include #include #include +#include #include #define UBLK_MINORS (1U << MINORBITS) @@ -51,7 +52,8 @@ | UBLK_F_URING_CMD_COMP_IN_TASK \ | UBLK_F_NEED_GET_DATA \ | UBLK_F_USER_RECOVERY \ - | UBLK_F_USER_RECOVERY_REISSUE) + | UBLK_F_USER_RECOVERY_REISSUE \ + | UBLK_F_UNPRIVILEGED_DEV) /* All UBLK_PARAM_TYPE_* should be included here */ #define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | \ @@ -1618,6 +1620,17 @@ out_free_cpumask: return ret; } +static void ublk_store_owner_uid_gid(struct ublksrv_ctrl_dev_info *info) +{ + kuid_t uid; + kgid_t gid; + + current_uid_gid(&uid, &gid); + + info->owner_uid = from_kuid(&init_user_ns, uid); + info->owner_gid = from_kgid(&init_user_ns, gid); +} + static inline void ublk_dump_dev_info(struct ublksrv_ctrl_dev_info *info) { pr_devel("%s: dev id %d flags %llx\n", __func__, @@ -1641,15 +1654,26 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) __func__, header->queue_id); return -EINVAL; } + if (copy_from_user(&info, argp, sizeof(info))) return -EFAULT; - ublk_dump_dev_info(&info); + + if (capable(CAP_SYS_ADMIN)) + info.flags &= ~UBLK_F_UNPRIVILEGED_DEV; + else if (!(info.flags & UBLK_F_UNPRIVILEGED_DEV)) + return -EPERM; + + /* the created device is always owned by current user */ + ublk_store_owner_uid_gid(&info); + if (header->dev_id != info.dev_id) { pr_warn("%s: dev id not match %u %u\n", __func__, header->dev_id, info.dev_id); return -EINVAL; } + ublk_dump_dev_info(&info); + ret = mutex_lock_killable(&ublk_ctl_mutex); if (ret) return ret; @@ -1982,6 +2006,115 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub, return ret; } +/* + * All control commands are sent via /dev/ublk-control, so we have to check + * the destination device's permission + */ +static int ublk_char_dev_permission(struct ublk_device *ub, + const char *dev_path, int mask) +{ + int err; + struct path path; + struct kstat stat; + + err = kern_path(dev_path, LOOKUP_FOLLOW, &path); + if (err) + return err; + + err = vfs_getattr(&path, &stat, STATX_TYPE, AT_STATX_SYNC_AS_STAT); + if (err) + goto exit; + + err = -EPERM; + if (stat.rdev != ub->cdev_dev.devt || !S_ISCHR(stat.mode)) + goto exit; + + err = inode_permission(&init_user_ns, + d_backing_inode(path.dentry), mask); +exit: + path_put(&path); + return err; +} + +static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub, + struct io_uring_cmd *cmd) +{ + struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd; + bool unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV; + void __user *argp = (void __user *)(unsigned long)header->addr; + char *dev_path = NULL; + int ret = 0; + int mask; + + if (!unprivileged) { + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + /* + * The new added command of UBLK_CMD_GET_DEV_INFO2 includes + * char_dev_path in payload too, since userspace may not + * know if the specified device is created as unprivileged + * mode. + */ + if (cmd->cmd_op != UBLK_CMD_GET_DEV_INFO2) + return 0; + } + + /* + * User has to provide the char device path for unprivileged ublk + * + * header->addr always points to the dev path buffer, and + * header->dev_path_len records length of dev path buffer. + */ + if (!header->dev_path_len || header->dev_path_len > PATH_MAX) + return -EINVAL; + + if (header->len < header->dev_path_len) + return -EINVAL; + + dev_path = kmalloc(header->dev_path_len + 1, GFP_KERNEL); + if (!dev_path) + return -ENOMEM; + + ret = -EFAULT; + if (copy_from_user(dev_path, argp, header->dev_path_len)) + goto exit; + dev_path[header->dev_path_len] = 0; + + ret = -EINVAL; + switch (cmd->cmd_op) { + case UBLK_CMD_GET_DEV_INFO: + case UBLK_CMD_GET_DEV_INFO2: + case UBLK_CMD_GET_QUEUE_AFFINITY: + case UBLK_CMD_GET_PARAMS: + mask = MAY_READ; + break; + case UBLK_CMD_START_DEV: + case UBLK_CMD_STOP_DEV: + case UBLK_CMD_ADD_DEV: + case UBLK_CMD_DEL_DEV: + case UBLK_CMD_SET_PARAMS: + case UBLK_CMD_START_USER_RECOVERY: + case UBLK_CMD_END_USER_RECOVERY: + mask = MAY_READ | MAY_WRITE; + break; + default: + goto exit; + } + + ret = ublk_char_dev_permission(ub, dev_path, mask); + if (!ret) { + header->len -= header->dev_path_len; + header->addr += header->dev_path_len; + } + pr_devel("%s: dev id %d cmd_op %x uid %d gid %d path %s ret %d\n", + __func__, ub->ub_number, cmd->cmd_op, + ub->dev_info.owner_uid, ub->dev_info.owner_gid, + dev_path, ret); +exit: + kfree(dev_path); + return ret; +} + static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) { @@ -1997,17 +2130,21 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, if (!(issue_flags & IO_URING_F_SQE128)) goto out; - ret = -EPERM; - if (!capable(CAP_SYS_ADMIN)) - goto out; - if (cmd->cmd_op != UBLK_CMD_ADD_DEV) { ret = -ENODEV; ub = ublk_get_device_from_id(header->dev_id); if (!ub) goto out; + + ret = ublk_ctrl_uring_cmd_permission(ub, cmd); + } else { + /* ADD_DEV permission check is done in command handler */ + ret = 0; } + if (ret) + goto put_dev; + switch (cmd->cmd_op) { case UBLK_CMD_START_DEV: ret = ublk_ctrl_start_dev(ub, cmd); @@ -2016,6 +2153,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, ret = ublk_ctrl_stop_dev(ub); break; case UBLK_CMD_GET_DEV_INFO: + case UBLK_CMD_GET_DEV_INFO2: ret = ublk_ctrl_get_dev_info(ub, cmd); break; case UBLK_CMD_ADD_DEV: @@ -2043,6 +2181,8 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd, ret = -ENOTSUPP; break; } + + put_dev: if (ub) ublk_put_device(ub); out: diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h index 4e38b9aa0293..f6238ccc7800 100644 --- a/include/uapi/linux/ublk_cmd.h +++ b/include/uapi/linux/ublk_cmd.h @@ -19,6 +19,8 @@ #define UBLK_CMD_GET_PARAMS 0x09 #define UBLK_CMD_START_USER_RECOVERY 0x10 #define UBLK_CMD_END_USER_RECOVERY 0x11 +#define UBLK_CMD_GET_DEV_INFO2 0x12 + /* * IO commands, issued by ublk server, and handled by ublk driver. * @@ -79,6 +81,27 @@ #define UBLK_F_USER_RECOVERY_REISSUE (1UL << 4) +/* + * Unprivileged user can create /dev/ublkcN and /dev/ublkbN. + * + * /dev/ublk-control needs to be available for unprivileged user, and it + * can be done via udev rule to make all control commands available to + * unprivileged user. Except for the command of UBLK_CMD_ADD_DEV, all + * other commands are only allowed for the owner of the specified device. + * + * When userspace sends UBLK_CMD_ADD_DEV, the device pair's owner_uid and + * owner_gid are stored to ublksrv_ctrl_dev_info by kernel, so far only + * the current user's uid/gid is stored, that said owner of the created + * device is always the current user. + * + * We still need udev rule to apply OWNER/GROUP with the stored owner_uid + * and owner_gid. + * + * Then ublk server can be run as unprivileged user, and /dev/ublkbN can + * be accessed and managed by its owner represented by owner_uid/owner_gid. + */ +#define UBLK_F_UNPRIVILEGED_DEV (1UL << 5) + /* device state */ #define UBLK_S_DEV_DEAD 0 #define UBLK_S_DEV_LIVE 1 @@ -98,7 +121,15 @@ struct ublksrv_ctrl_cmd { __u64 addr; /* inline data */ - __u64 data[2]; + __u64 data[1]; + + /* + * Used for UBLK_F_UNPRIVILEGED_DEV and UBLK_CMD_GET_DEV_INFO2 + * only, include null char + */ + __u16 dev_path_len; + __u16 pad; + __u32 reserved; }; struct ublksrv_ctrl_dev_info { @@ -118,7 +149,8 @@ struct ublksrv_ctrl_dev_info { /* For ublksrv internal use, invisible to ublk driver */ __u64 ublksrv_flags; - __u64 reserved0; + __u32 owner_uid; /* store by kernel */ + __u32 owner_gid; /* store by kernel */ __u64 reserved1; __u64 reserved2; }; -- cgit v1.2.3 From 1bf7a749efdc5183e83239e030596ef74b9c8b13 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Mon, 23 Jan 2023 08:47:18 +0100 Subject: ps3vram: remove bio splitting ps3vram iterates over the bio one segment, that is page aligned and max page sized chunk, a time. Because of that there is no point in calling bio_split_to_limits, or explicitly setting the default limits that are only used by bio_split_to_limits. Signed-off-by: Christoph Hellwig Tested-by: Geoff Levand Link: https://lore.kernel.org/r/20230123074718.57951-1-hch@lst.de Signed-off-by: Jens Axboe --- drivers/block/ps3vram.c | 7 ------- 1 file changed, 7 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/ps3vram.c b/drivers/block/ps3vram.c index 574e470b220b..38d42af01b25 100644 --- a/drivers/block/ps3vram.c +++ b/drivers/block/ps3vram.c @@ -586,10 +586,6 @@ static void ps3vram_submit_bio(struct bio *bio) dev_dbg(&dev->core, "%s\n", __func__); - bio = bio_split_to_limits(bio); - if (!bio) - return; - spin_lock_irq(&priv->lock); busy = !bio_list_empty(&priv->list); bio_list_add(&priv->list, bio); @@ -749,9 +745,6 @@ static int ps3vram_probe(struct ps3_system_bus_device *dev) gendisk->private_data = dev; strscpy(gendisk->disk_name, DEVICE_NAME, sizeof(gendisk->disk_name)); set_capacity(gendisk, priv->size >> 9); - blk_queue_max_segments(gendisk->queue, BLK_MAX_SEGMENTS); - blk_queue_max_segment_size(gendisk->queue, BLK_MAX_SEGMENT_SIZE); - blk_queue_max_hw_sectors(gendisk->queue, BLK_SAFE_MAX_SECTORS); dev_info(&dev->core, "%s: Using %llu MiB of GPU memory\n", gendisk->disk_name, get_capacity(gendisk) >> 11); -- cgit v1.2.3 From 48a9051980242128f844defe44c7e83217f79872 Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Tue, 31 Jan 2023 12:04:46 +0800 Subject: ublk_drv: only allow owner to open unprivileged disk Owner of one unprivileged ublk device could be one evil user, which can grant this disk's privilege to other users deliberately, and this way could be like making one trap and waiting for other users to be caught. So only owner to open unprivileged disk even though the owner grants disk privilege to other user. This way is reasonable too given anyone can create ublk disk, and no need other's grant. Reported-by: Stefan Hajnoczi Fixes: 4093cb5a0634 ("ublk_drv: add mechanism for supporting unprivileged ublk device") Signed-off-by: Ming Lei Link: https://lore.kernel.org/r/20230131040446.214583-1-ming.lei@redhat.com Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 55 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 12 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index a725a236a38f..c932e9ea5a0f 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -377,8 +377,50 @@ static void ublk_free_disk(struct gendisk *disk) put_device(&ub->cdev_dev); } +static void ublk_store_owner_uid_gid(unsigned int *owner_uid, + unsigned int *owner_gid) +{ + kuid_t uid; + kgid_t gid; + + current_uid_gid(&uid, &gid); + + *owner_uid = from_kuid(&init_user_ns, uid); + *owner_gid = from_kgid(&init_user_ns, gid); +} + +static int ublk_open(struct block_device *bdev, fmode_t mode) +{ + struct ublk_device *ub = bdev->bd_disk->private_data; + + if (capable(CAP_SYS_ADMIN)) + return 0; + + /* + * If it is one unprivileged device, only owner can open + * the disk. Otherwise it could be one trap made by one + * evil user who grants this disk's privileges to other + * users deliberately. + * + * This way is reasonable too given anyone can create + * unprivileged device, and no need other's grant. + */ + if (ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV) { + unsigned int curr_uid, curr_gid; + + ublk_store_owner_uid_gid(&curr_uid, &curr_gid); + + if (curr_uid != ub->dev_info.owner_uid || curr_gid != + ub->dev_info.owner_gid) + return -EPERM; + } + + return 0; +} + static const struct block_device_operations ub_fops = { .owner = THIS_MODULE, + .open = ublk_open, .free_disk = ublk_free_disk, }; @@ -1620,17 +1662,6 @@ out_free_cpumask: return ret; } -static void ublk_store_owner_uid_gid(struct ublksrv_ctrl_dev_info *info) -{ - kuid_t uid; - kgid_t gid; - - current_uid_gid(&uid, &gid); - - info->owner_uid = from_kuid(&init_user_ns, uid); - info->owner_gid = from_kgid(&init_user_ns, gid); -} - static inline void ublk_dump_dev_info(struct ublksrv_ctrl_dev_info *info) { pr_devel("%s: dev id %d flags %llx\n", __func__, @@ -1664,7 +1695,7 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) return -EPERM; /* the created device is always owned by current user */ - ublk_store_owner_uid_gid(&info); + ublk_store_owner_uid_gid(&info.owner_uid, &info.owner_gid); if (header->dev_id != info.dev_id) { pr_warn("%s: dev id not match %u %u\n", -- cgit v1.2.3 From e152a05fa054170c05f1d5e04e93e2e75ea11405 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Mon, 30 Jan 2023 13:13:47 -0800 Subject: loop: Improve the hw_queue_depth kernel module parameter implementation Make the following minor changes which were reported by colleagues while reviewing this code: - Remove the parentheses from around the LOOP_DEFAULT_HW_Q_DEPTH definition since these are superfluous. - Accept other number formats than decimal, e.g. hexadecimal. - Do not set hw_queue_depth to an out-of-range value, even if that value won't be used. - Use the LOOP_DEFAULT_HW_Q_DEPTH macro in the kernel module parameter description to prevent that the description gets out of sync. This patch has been tested as follows: # modprobe -r loop # modprobe loop hw_queue_depth=-1 modprobe: ERROR: could not insert 'loop': Invalid argument # modprobe loop hw_queue_depth=0 modprobe: ERROR: could not insert 'loop': Invalid argument # modprobe loop hw_queue_depth=1; cat /sys/module/loop/parameters/hw_queue_depth 1 # modprobe -r loop; modprobe loop; cat /sys/module/loop/parameters/hw_queue_depth hw_queue_depth=0x10 16 # modprobe -r loop; modprobe loop; cat /sys/module/loop/parameters/hw_queue_depth hw_queue_depth=128 128 # modprobe -r loop; modprobe loop hw_queue_depth=129; cat /sys/module/loop/parameters/hw_queue_depth 129 # modprobe -r loop; modprobe loop hw_queue_depth=$((1<<32)) modprobe: ERROR: could not insert 'loop': Numerical result out of range See also commit ef44c50837ab ("loop: allow user to set the queue depth"). Cc: Chaitanya Kulkarni Cc: Himanshu Madhani Signed-off-by: Bart Van Assche Reviewed-by: Chaitanya Kulkarni Link: https://lore.kernel.org/r/20230130211347.832110-1-bvanassche@acm.org Signed-off-by: Jens Axboe --- drivers/block/loop.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 1518a6423279..5f04235e4ff7 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -90,7 +90,7 @@ struct loop_cmd { }; #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ) -#define LOOP_DEFAULT_HW_Q_DEPTH (128) +#define LOOP_DEFAULT_HW_Q_DEPTH 128 static DEFINE_IDR(loop_index_idr); static DEFINE_MUTEX(loop_ctl_mutex); @@ -1792,9 +1792,15 @@ static int hw_queue_depth = LOOP_DEFAULT_HW_Q_DEPTH; static int loop_set_hw_queue_depth(const char *s, const struct kernel_param *p) { - int ret = kstrtoint(s, 10, &hw_queue_depth); + int qd, ret; - return (ret || (hw_queue_depth < 1)) ? -EINVAL : 0; + ret = kstrtoint(s, 0, &qd); + if (ret < 0) + return ret; + if (qd < 1) + return -EINVAL; + hw_queue_depth = qd; + return 0; } static const struct kernel_param_ops loop_hw_qdepth_param_ops = { @@ -1803,7 +1809,7 @@ static const struct kernel_param_ops loop_hw_qdepth_param_ops = { }; device_param_cb(hw_queue_depth, &loop_hw_qdepth_param_ops, &hw_queue_depth, 0444); -MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 128"); +MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: " __stringify(LOOP_DEFAULT_HW_Q_DEPTH)); MODULE_LICENSE("GPL"); MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR); -- cgit v1.2.3 From 7df2af0bb4912cf360045d065f88fe4ed2f702ca Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 3 Feb 2023 16:06:19 +0100 Subject: rbd: use bvec_set_page to initialize the copy up bvec Use the bvec_set_page helper to initialize the copy up bvec. Signed-off-by: Christoph Hellwig Reviewed-by: Ilya Dryomov Link: https://lore.kernel.org/r/20230203150634.3199647-9-hch@lst.de Signed-off-by: Jens Axboe --- drivers/block/rbd.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 04453f4a319c..1faca7e07a4d 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -3068,13 +3068,12 @@ static int setup_copyup_bvecs(struct rbd_obj_request *obj_req, u64 obj_overlap) for (i = 0; i < obj_req->copyup_bvec_count; i++) { unsigned int len = min(obj_overlap, (u64)PAGE_SIZE); + struct page *page = alloc_page(GFP_NOIO); - obj_req->copyup_bvecs[i].bv_page = alloc_page(GFP_NOIO); - if (!obj_req->copyup_bvecs[i].bv_page) + if (!page) return -ENOMEM; - obj_req->copyup_bvecs[i].bv_offset = 0; - obj_req->copyup_bvecs[i].bv_len = len; + bvec_set_page(&obj_req->copyup_bvecs[i], page, len, 0); obj_overlap -= len; } -- cgit v1.2.3 From b831f3a1031664ae2443bab63d35c416ed30c91d Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 3 Feb 2023 16:06:20 +0100 Subject: virtio_blk: use bvec_set_virt to initialize special_vec Use the bvec_set_virt helper to initialize the special_vec. Signed-off-by: Christoph Hellwig Reviewed-by: Chaitanya Kulkarni Acked-by: Michael S. Tsirkin Acked-by: Jason Wang Link: https://lore.kernel.org/r/20230203150634.3199647-10-hch@lst.de Signed-off-by: Jens Axboe --- drivers/block/virtio_blk.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 6a77fa917428..dc6e9b989910 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -170,9 +170,7 @@ static int virtblk_setup_discard_write_zeroes_erase(struct request *req, bool un WARN_ON_ONCE(n != segments); - req->special_vec.bv_page = virt_to_page(range); - req->special_vec.bv_offset = offset_in_page(range); - req->special_vec.bv_len = sizeof(*range) * segments; + bvec_set_virt(&req->special_vec, range, sizeof(*range) * segments); req->rq_flags |= RQF_SPECIAL_PAYLOAD; return 0; -- cgit v1.2.3 From 13ae4db0c05107814db4e774856aa83e72e8bf04 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 3 Feb 2023 16:06:21 +0100 Subject: zram: use bvec_set_page to initialize bvecs Use the bvec_set_page helper to initialize bvecs. Signed-off-by: Christoph Hellwig Reviewed-by: Sergey Senozhatsky Reviewed-by: Johannes Thumshirn Link: https://lore.kernel.org/r/20230203150634.3199647-11-hch@lst.de Signed-off-by: Jens Axboe --- drivers/block/zram/zram_drv.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index e290d6d97047..bd8ae4822dc3 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -703,9 +703,7 @@ static ssize_t writeback_store(struct device *dev, for (; nr_pages != 0; index++, nr_pages--) { struct bio_vec bvec; - bvec.bv_page = page; - bvec.bv_len = PAGE_SIZE; - bvec.bv_offset = 0; + bvec_set_page(&bvec, page, PAGE_SIZE, 0); spin_lock(&zram->wb_limit_lock); if (zram->wb_limit_enable && !zram->bd_wb_limit) { @@ -1380,12 +1378,9 @@ out: static int zram_bvec_read_from_bdev(struct zram *zram, struct page *page, u32 index, struct bio *bio, bool partial_io) { - struct bio_vec bvec = { - .bv_page = page, - .bv_len = PAGE_SIZE, - .bv_offset = 0, - }; + struct bio_vec bvec; + bvec_set_page(&bvec, page, PAGE_SIZE, 0); return read_from_bdev(zram, &bvec, zram_get_element(zram, index), bio, partial_io); } @@ -1652,9 +1647,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, memcpy_from_bvec(dst + offset, bvec); kunmap_atomic(dst); - vec.bv_page = page; - vec.bv_len = PAGE_SIZE; - vec.bv_offset = 0; + bvec_set_page(&vec, page, PAGE_SIZE, 0); } ret = __zram_bvec_write(zram, &vec, index, bio); -- cgit v1.2.3 From 731e208d7b4b38d2bac4b7c53403c8abbf306d01 Mon Sep 17 00:00:00 2001 From: Ziyang Zhang Date: Tue, 7 Feb 2023 15:08:37 +0800 Subject: ublk: remove unnecessary NULL check in ublk_rq_has_data() bio_has_data() allows a NULL bio so the NULL check in ublk_rq_has_data() is unnecessary. Signed-off-by: Ziyang Zhang Reviewed-by: Ming Lei Link: https://lore.kernel.org/r/20230207070839.370817-2-ZiyangZhang@linux.alibaba.com Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/block') diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index c932e9ea5a0f..55fccce68a9c 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -322,7 +322,7 @@ static inline struct ublk_queue *ublk_get_queue(struct ublk_device *dev, static inline bool ublk_rq_has_data(const struct request *rq) { - return rq->bio && bio_has_data(rq->bio); + return bio_has_data(rq->bio); } static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq, -- cgit v1.2.3 From b352389e7ba34bdb5bcf4254fa1e85319ba76352 Mon Sep 17 00:00:00 2001 From: Ziyang Zhang Date: Tue, 7 Feb 2023 15:08:38 +0800 Subject: ublk: mention WRITE_ZEROES in comment of ublk_complete_rq() WRITE_ZEROES won't return bytes returned just like FLUSH and DISCARD, and we can end it directly. Add missing comment for it in ublk_complete_rq(). Signed-off-by: Ziyang Zhang Reviewed-by: Ming Lei Link: https://lore.kernel.org/r/20230207070839.370817-3-ZiyangZhang@linux.alibaba.com Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/block') diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 55fccce68a9c..06eddefdf02a 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -665,7 +665,7 @@ static void ublk_complete_rq(struct request *req) } /* - * FLUSH or DISCARD usually won't return bytes returned, so end them + * FLUSH, DISCARD or WRITE_ZEROES usually won't return bytes returned, so end them * directly. * * Both the two needn't unmap. -- cgit v1.2.3 From 1972d038a5401781377d3ce2d901bf7763a43589 Mon Sep 17 00:00:00 2001 From: Ziyang Zhang Date: Tue, 7 Feb 2023 15:08:39 +0800 Subject: ublk: pass NULL to blk_mq_alloc_disk() as queuedata queuedata is not referenced in ublk_drv and we can use driver_data instead. Pass NULL to blk_mq_alloc_disk() as queuedata while allocating ublk's gendisk. Signed-off-by: Ziyang Zhang Reviewed-by: Ming Lei Link: https://lore.kernel.org/r/20230207070839.370817-4-ZiyangZhang@linux.alibaba.com Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/block') diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 06eddefdf02a..d83fe2c2b3ba 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -1578,7 +1578,7 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub, struct io_uring_cmd *cmd) goto out_unlock; } - disk = blk_mq_alloc_disk(&ub->tag_set, ub); + disk = blk_mq_alloc_disk(&ub->tag_set, NULL); if (IS_ERR(disk)) { ret = PTR_ERR(disk); goto out_unlock; -- cgit v1.2.3 From 0abe39dec065133e3f92a52219c3728fe7d7617f Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Tue, 7 Feb 2023 23:07:00 +0800 Subject: block: ublk: improve handling device deletion Inside ublk_ctrl_del_dev(), when the device is removed, we wait until the device number is freed with holding global lock of ublk_ctl_mutex, this way isn't friendly from user viewpoint: 1) if device is in-use, the current delete command hangs in ublk_ctrl_del_dev(), and user can't break from the handling because wait_event() is used 2) global lock is held, so any new device can't be added and other old devices can't be removed. Improve the deleting handling by the following way, suggested by Nadav: 1) wait without holding the global lock 2) replace wait_event() with wait_event_interruptible() Reported-by: Nadav Amit Suggested-by: Nadav Amit Signed-off-by: Ming Lei Link: https://lore.kernel.org/r/20230207150700.545530-1-ming.lei@redhat.com Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index d83fe2c2b3ba..e6eceee44366 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -150,6 +150,7 @@ struct ublk_device { #define UB_STATE_OPEN 0 #define UB_STATE_USED 1 +#define UB_STATE_DELETED 2 unsigned long state; int ub_number; @@ -1804,20 +1805,33 @@ static int ublk_ctrl_del_dev(struct ublk_device **p_ub) if (ret) return ret; - ublk_remove(ub); + if (!test_bit(UB_STATE_DELETED, &ub->state)) { + ublk_remove(ub); + set_bit(UB_STATE_DELETED, &ub->state); + } /* Mark the reference as consumed */ *p_ub = NULL; ublk_put_device(ub); + mutex_unlock(&ublk_ctl_mutex); /* * Wait until the idr is removed, then it can be reused after * DEL_DEV command is returned. + * + * If we returns because of user interrupt, future delete command + * may come: + * + * - the device number isn't freed, this device won't or needn't + * be deleted again, since UB_STATE_DELETED is set, and device + * will be released after the last reference is dropped + * + * - the device number is freed already, we will not find this + * device via ublk_get_device_from_id() */ - wait_event(ublk_idr_wq, ublk_idr_freed(idx)); - mutex_unlock(&ublk_ctl_mutex); + wait_event_interruptible(ublk_idr_wq, ublk_idr_freed(idx)); - return ret; + return 0; } static inline void ublk_ctrl_cmd_dump(struct io_uring_cmd *cmd) -- cgit v1.2.3 From 2f1e07dda1e1310873647abc40bbc49eaf3b10e3 Mon Sep 17 00:00:00 2001 From: Liu Xiaodong Date: Fri, 10 Feb 2023 09:13:56 -0500 Subject: block: ublk: check IO buffer based on flag need_get_data Currently, uring_cmd with UBLK_IO_FETCH_REQ or UBLK_IO_COMMIT_AND_FETCH_REQ is always checked whether userspace server has provided IO buffer even flag UBLK_F_NEED_GET_DATA is configured. This is a excessive check. If UBLK_F_NEED_GET_DATA is configured, FETCH_RQ doesn't need to provide IO buffer; COMMIT_AND_FETCH_REQ also doesn't need to do that if the IO type is not READ. Check ub_cmd->addr together with ublk_need_get_data() and IO type in ublk_ch_uring_cmd(). With this fix, userspace server doesn't need to preserve buffers for every ublk_io when flag UBLK_F_NEED_GET_DATA is configured, in order to save memory. Signed-off-by: Liu Xiaodong Fixes: c86019ff75c1 ("ublk_drv: add support for UBLK_IO_NEED_GET_DATA") Reviewed-by: Ming Lei Link: https://lore.kernel.org/r/20230210141356.112321-1-xiaodong.liu@intel.com Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index e6eceee44366..f48d213fb65e 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -1265,6 +1265,7 @@ static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) u32 cmd_op = cmd->cmd_op; unsigned tag = ub_cmd->tag; int ret = -EINVAL; + struct request *req; pr_devel("%s: received: cmd op %d queue %d tag %d result %d\n", __func__, cmd->cmd_op, ub_cmd->q_id, tag, @@ -1315,8 +1316,8 @@ static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) */ if (io->flags & UBLK_IO_FLAG_OWNED_BY_SRV) goto out; - /* FETCH_RQ has to provide IO buffer */ - if (!ub_cmd->addr) + /* FETCH_RQ has to provide IO buffer if NEED GET DATA is not enabled */ + if (!ub_cmd->addr && !ublk_need_get_data(ubq)) goto out; io->cmd = cmd; io->flags |= UBLK_IO_FLAG_ACTIVE; @@ -1325,8 +1326,12 @@ static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) ublk_mark_io_ready(ub, ubq); break; case UBLK_IO_COMMIT_AND_FETCH_REQ: - /* FETCH_RQ has to provide IO buffer */ - if (!ub_cmd->addr) + req = blk_mq_tag_to_rq(ub->tag_set.tags[ub_cmd->q_id], tag); + /* + * COMMIT_AND_FETCH_REQ has to provide IO buffer if NEED GET DATA is + * not enabled or it is Read IO. + */ + if (!ub_cmd->addr && (!ublk_need_get_data(ubq) || req_op(req) == REQ_OP_READ)) goto out; if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV)) goto out; -- cgit v1.2.3 From db0ccc44a20b4bb3039c0f6885a1f9c3323c7673 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 16 Feb 2023 07:57:32 -0700 Subject: brd: return 0/-error from brd_insert_page() It currently returns a page, but callers just check for NULL/page to gauge success. Clean this up and return the appropriate error directly instead. Cc: stable@vger.kernel.org # 5.10+ Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/block/brd.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/brd.c b/drivers/block/brd.c index 20acc4a1fd6d..15a148d5aad9 100644 --- a/drivers/block/brd.c +++ b/drivers/block/brd.c @@ -78,11 +78,9 @@ static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector) } /* - * Look up and return a brd's page for a given sector. - * If one does not exist, allocate an empty page, and insert that. Then - * return it. + * Insert a new page for a given sector, if one does not already exist. */ -static struct page *brd_insert_page(struct brd_device *brd, sector_t sector) +static int brd_insert_page(struct brd_device *brd, sector_t sector) { pgoff_t idx; struct page *page; @@ -90,7 +88,7 @@ static struct page *brd_insert_page(struct brd_device *brd, sector_t sector) page = brd_lookup_page(brd, sector); if (page) - return page; + return 0; /* * Must use NOIO because we don't want to recurse back into the @@ -99,11 +97,11 @@ static struct page *brd_insert_page(struct brd_device *brd, sector_t sector) gfp_flags = GFP_NOIO | __GFP_ZERO | __GFP_HIGHMEM; page = alloc_page(gfp_flags); if (!page) - return NULL; + return -ENOMEM; if (radix_tree_preload(GFP_NOIO)) { __free_page(page); - return NULL; + return -ENOMEM; } spin_lock(&brd->brd_lock); @@ -120,8 +118,7 @@ static struct page *brd_insert_page(struct brd_device *brd, sector_t sector) spin_unlock(&brd->brd_lock); radix_tree_preload_end(); - - return page; + return 0; } /* @@ -174,16 +171,17 @@ static int copy_to_brd_setup(struct brd_device *brd, sector_t sector, size_t n) { unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT; size_t copy; + int ret; copy = min_t(size_t, n, PAGE_SIZE - offset); - if (!brd_insert_page(brd, sector)) - return -ENOSPC; + ret = brd_insert_page(brd, sector); + if (ret) + return ret; if (copy < n) { sector += copy >> SECTOR_SHIFT; - if (!brd_insert_page(brd, sector)) - return -ENOSPC; + ret = brd_insert_page(brd, sector); } - return 0; + return ret; } /* -- cgit v1.2.3 From 6ded703c56c21bfb259725d4f1831a5feb563e9b Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 16 Feb 2023 08:01:08 -0700 Subject: brd: check for REQ_NOWAIT and set correct page allocation mask If REQ_NOWAIT is set, then do a non-blocking allocation if the operation is a write and we need to insert a new page. Currently REQ_NOWAIT cannot be set as the queue isn't marked as supporting nowait, this change is in preparation for allowing that. radix_tree_preload() warns on attempting to call it with an allocation mask that doesn't allow blocking. While that warning could arguably be removed, we need to handle radix insertion failures anyway as they are more likely if we cannot block to get memory. Remove legacy BUG_ON()'s and turn them into proper errors instead, one for the allocation failure and one for finding a page that doesn't match the correct index. Cc: stable@vger.kernel.org # 5.10+ Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/block/brd.c | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/brd.c b/drivers/block/brd.c index 15a148d5aad9..00f3c5b51a01 100644 --- a/drivers/block/brd.c +++ b/drivers/block/brd.c @@ -80,26 +80,21 @@ static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector) /* * Insert a new page for a given sector, if one does not already exist. */ -static int brd_insert_page(struct brd_device *brd, sector_t sector) +static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp) { pgoff_t idx; struct page *page; - gfp_t gfp_flags; + int ret = 0; page = brd_lookup_page(brd, sector); if (page) return 0; - /* - * Must use NOIO because we don't want to recurse back into the - * block or filesystem layers from page reclaim. - */ - gfp_flags = GFP_NOIO | __GFP_ZERO | __GFP_HIGHMEM; - page = alloc_page(gfp_flags); + page = alloc_page(gfp | __GFP_ZERO | __GFP_HIGHMEM); if (!page) return -ENOMEM; - if (radix_tree_preload(GFP_NOIO)) { + if (gfpflags_allow_blocking(gfp) && radix_tree_preload(gfp)) { __free_page(page); return -ENOMEM; } @@ -110,15 +105,17 @@ static int brd_insert_page(struct brd_device *brd, sector_t sector) if (radix_tree_insert(&brd->brd_pages, idx, page)) { __free_page(page); page = radix_tree_lookup(&brd->brd_pages, idx); - BUG_ON(!page); - BUG_ON(page->index != idx); + if (!page) + ret = -ENOMEM; + else if (page->index != idx) + ret = -EIO; } else { brd->brd_nr_pages++; } spin_unlock(&brd->brd_lock); radix_tree_preload_end(); - return 0; + return ret; } /* @@ -167,19 +164,20 @@ static void brd_free_pages(struct brd_device *brd) /* * copy_to_brd_setup must be called before copy_to_brd. It may sleep. */ -static int copy_to_brd_setup(struct brd_device *brd, sector_t sector, size_t n) +static int copy_to_brd_setup(struct brd_device *brd, sector_t sector, size_t n, + gfp_t gfp) { unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT; size_t copy; int ret; copy = min_t(size_t, n, PAGE_SIZE - offset); - ret = brd_insert_page(brd, sector); + ret = brd_insert_page(brd, sector, gfp); if (ret) return ret; if (copy < n) { sector += copy >> SECTOR_SHIFT; - ret = brd_insert_page(brd, sector); + ret = brd_insert_page(brd, sector, gfp); } return ret; } @@ -254,20 +252,26 @@ static void copy_from_brd(void *dst, struct brd_device *brd, * Process a single bvec of a bio. */ static int brd_do_bvec(struct brd_device *brd, struct page *page, - unsigned int len, unsigned int off, enum req_op op, + unsigned int len, unsigned int off, blk_opf_t opf, sector_t sector) { void *mem; int err = 0; - if (op_is_write(op)) { - err = copy_to_brd_setup(brd, sector, len); + if (op_is_write(opf)) { + /* + * Must use NOIO because we don't want to recurse back into the + * block or filesystem layers from page reclaim. + */ + gfp_t gfp = opf & REQ_NOWAIT ? GFP_NOWAIT : GFP_NOIO; + + err = copy_to_brd_setup(brd, sector, len, gfp); if (err) goto out; } mem = kmap_atomic(page); - if (!op_is_write(op)) { + if (!op_is_write(opf)) { copy_from_brd(mem + off, brd, sector, len); flush_dcache_page(page); } else { @@ -296,8 +300,12 @@ static void brd_submit_bio(struct bio *bio) (len & (SECTOR_SIZE - 1))); err = brd_do_bvec(brd, bvec.bv_page, len, bvec.bv_offset, - bio_op(bio), sector); + bio->bi_opf, sector); if (err) { + if (err == -ENOMEM && bio->bi_opf & REQ_NOWAIT) { + bio_wouldblock_error(bio); + return; + } bio_io_error(bio); return; } -- cgit v1.2.3 From 67205f80be9910207481406c47f7d85e703fb2e9 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Wed, 15 Feb 2023 16:43:47 -0700 Subject: brd: mark as nowait compatible By default, non-mq drivers do not support nowait. This causes io_uring to use a slower path as the driver cannot be trust not to block. brd can safely set the nowait flag, as worst case all it does is a NOIO allocation. For io_uring, this makes a substantial difference. Before: submitter=0, tid=453, file=/dev/ram0, node=-1 polled=0, fixedbufs=1/0, register_files=1, buffered=0, QD=128 Engine=io_uring, sq_ring=128, cq_ring=128 IOPS=440.03K, BW=1718MiB/s, IOS/call=32/31 IOPS=428.96K, BW=1675MiB/s, IOS/call=32/32 IOPS=442.59K, BW=1728MiB/s, IOS/call=32/31 IOPS=419.65K, BW=1639MiB/s, IOS/call=32/32 IOPS=426.82K, BW=1667MiB/s, IOS/call=32/31 and after: submitter=0, tid=354, file=/dev/ram0, node=-1 polled=0, fixedbufs=1/0, register_files=1, buffered=0, QD=128 Engine=io_uring, sq_ring=128, cq_ring=128 IOPS=3.37M, BW=13.15GiB/s, IOS/call=32/31 IOPS=3.45M, BW=13.46GiB/s, IOS/call=32/31 IOPS=3.43M, BW=13.42GiB/s, IOS/call=32/32 IOPS=3.43M, BW=13.39GiB/s, IOS/call=32/31 IOPS=3.43M, BW=13.38GiB/s, IOS/call=32/31 or about an 8x in difference. Now that brd is prepared to deal with REQ_NOWAIT reads/writes, mark it as supporting that. Cc: stable@vger.kernel.org # 5.10+ Link: https://lore.kernel.org/linux-block/20230203103005.31290-1-p.raghav@samsung.com/ Signed-off-by: Jens Axboe --- drivers/block/brd.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/block') diff --git a/drivers/block/brd.c b/drivers/block/brd.c index 00f3c5b51a01..740631dcdd0e 100644 --- a/drivers/block/brd.c +++ b/drivers/block/brd.c @@ -418,6 +418,7 @@ static int brd_alloc(int i) /* Tell the block layer that this is not a rotational device */ blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue); blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, disk->queue); + blk_queue_flag_set(QUEUE_FLAG_NOWAIT, disk->queue); err = add_disk(disk); if (err) goto out_cleanup_disk; -- cgit v1.2.3 From 0aa2988e4fd23c0c8b33999d7b47dfbc5e6bf24b Mon Sep 17 00:00:00 2001 From: Pankaj Raghav Date: Fri, 17 Feb 2023 17:44:44 +0530 Subject: brd: use radix_tree_maybe_preload instead of radix_tree_preload Unconditionally calling radix_tree_preload_end() results in a OOPS message as the preload is only conditionally called for gfpflags_allow_blocking(). [ 20.267323] BUG: using smp_processor_id() in preemptible [00000000] code: fio/416 [ 20.267837] caller is brd_insert_page.part.0+0xbe/0x190 [brd] [ 20.269436] Call Trace: [ 20.269598] [ 20.269742] dump_stack_lvl+0x32/0x50 [ 20.269982] check_preemption_disabled+0xd1/0xe0 [ 20.270289] brd_insert_page.part.0+0xbe/0x190 [brd] [ 20.270664] brd_submit_bio+0x33f/0xf40 [brd] Use radix_tree_maybe_preload() which does preload only if gfpflags_allow_blocking() is true but also takes the lock. Therefore, unconditionally calling radix_tree_preload_end() should not create any issues and the message disappears. Fixes: 6ded703c56c2 ("brd: check for REQ_NOWAIT and set correct page allocation mask") Signed-off-by: Pankaj Raghav Link: https://lore.kernel.org/r/20230217121442.33914-1-p.raghav@samsung.com Signed-off-by: Jens Axboe --- drivers/block/brd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/block') diff --git a/drivers/block/brd.c b/drivers/block/brd.c index 740631dcdd0e..a8a77a1efe1e 100644 --- a/drivers/block/brd.c +++ b/drivers/block/brd.c @@ -94,7 +94,7 @@ static int brd_insert_page(struct brd_device *brd, sector_t sector, gfp_t gfp) if (!page) return -ENOMEM; - if (gfpflags_allow_blocking(gfp) && radix_tree_preload(gfp)) { + if (radix_tree_maybe_preload(gfp)) { __free_page(page); return -ENOMEM; } -- cgit v1.2.3