* [PATCH 0/2] Granular CAP_SYS_ADMIN [not found] <CGME20221020071338epcas5p16d72f5d4d868b889e3a98688bc454a98@epcas5p1.samsung.com> @ 2022-10-20 7:02 ` Kanchan Joshi [not found] ` <CGME20221020071343epcas5p3722073ab4822d39d6ca91606869f0379@epcas5p3.samsung.com> ` (3 more replies) 0 siblings, 4 replies; 8+ messages in thread From: Kanchan Joshi @ 2022-10-20 7:02 UTC (permalink / raw) To: hch, kbusch, sagi, axboe; +Cc: linux-nvme, gost.dev, Kanchan Joshi Hi, Patch 1 is for io-commands. It implements the shift to file-mode based policy. Patch 2 is to allow identify-namespace command. This is based on the feedback received during ALPSS. @Sagi: Since patch 1 is changed a bit (changelog below), I did not apply the reviewed-by tag. Please take a look again. Changes since v2: - Add patch 2 that allows identify-ns - Patch 1: Move nvme_cmd_allowed check further down, so that we can use CNS values for decision making in patch 2 - Patch 1: invert if condition (Sagi) Changes since v1: - Move nvme_cmd_allowed check at a place that allows using nvme_is_write helper (hch) - Keep everything into single patch (chaitanya, hch) - Comments cleanup (hch, chaitanya) - Part of cover-letter moved to commit-description Examples (after patches): *************************** #1: Two NS, one with 666 another with 600 $: ls -l /dev/ng* crw-rw-rw- 1 root root 242, 0 Oct 20 12:04 /dev/ng0n1 crw------- 1 root root 242, 1 Oct 20 12:04 /dev/ng0n2 #2: this should fail $: nvme id-ns /dev/ng0n2 /dev/ng0n2: Permission denied Usage: nvme id-ns <device> [OPTIONS] Send an Identify Namespace command to the given device, returns properties of the specified namespace in either human-readable or binary format. Can also return binary vendor-specific namespace attributes. Options: [ --namespace-id=<NUM>, -n <NUM> ] --- identifier of desired namespace [ --force ] --- Return this namespace, even if not attaced (1.2 devices only) [ --vendor-specific, -v ] --- dump binary vendor fields [ --raw-binary, -b ] --- show identify in binary format [ --output-format=<FMT>, -o <FMT> ] --- Output format: normal|json|binary [ --human-readable, -H ] --- show identify in readable format #3: this should travel $: nvme id-ns /dev/ng0n1 NVME Identify Namespace 1: nsze : 0x300000 ncap : 0x300000 nuse : 0x300000 nsfeat : 0 nlbaf : 7 flbas : 0x4 mc : 0 dpc : 0 dps : 0 nmic : 0 rescap : 0 fpi : 0 dlfeat : 9 nawun : 0 nawupf : 0 nacwu : 0 nabsn : 0 nabo : 0 nabspf : 0 noiob : 0 nvmcap : 0 mssrl : 256 mcl : 256 msrc : 127 nulbaf : 0 anagrpid: 0 nsattr : 0 nvmsetid: 0 endgid : 0 nguid : 00000000000000000000000000000000 eui64 : 0000000000000000 lbaf 0 : ms:0 lbads:9 rp:0 lbaf 1 : ms:8 lbads:9 rp:0 lbaf 2 : ms:16 lbads:9 rp:0 lbaf 3 : ms:64 lbads:9 rp:0 lbaf 4 : ms:0 lbads:12 rp:0 (in use) lbaf 5 : ms:8 lbads:12 rp:0 lbaf 6 : ms:16 lbads:12 rp:0 lbaf 7 : ms:64 lbads:12 rp:0 #4: this should not travel $: nvme id-ctrl /dev/ng0n1 identify controller: Permission denied #5: uring-passthru read on ng0n1 (should work) $: ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -cmd_type=nvme -bs=4k -numjobs=1 -size=4k -filename=/dev/ng0n1 -name=pt pt: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T) 4096B-4096B, ioengine=io_uring_cmd, iodepth=1 fio-3.32-58-gb19c-dirty Starting 1 process pt: (groupid=0, jobs=1): err= 0: pid=56582: Thu Oct 20 12:12:50 2022 read: IOPS=500, BW=2000KiB/s (2048kB/s)(4096B/2msec) slat (nsec): min=461505, max=461505, avg=461505.00, stdev= 0.00 clat (nsec): min=544742, max=544742, avg=544742.00, stdev= 0.00 lat (nsec): min=1006.2k, max=1006.2k, avg=1006247.00, stdev= 0.00 clat percentiles (usec): | 1.00th=[ 545], 5.00th=[ 545], 10.00th=[ 545], 20.00th=[ 545], | 30.00th=[ 545], 40.00th=[ 545], 50.00th=[ 545], 60.00th=[ 545], | 70.00th=[ 545], 80.00th=[ 545], 90.00th=[ 545], 95.00th=[ 545], | 99.00th=[ 545], 99.50th=[ 545], 99.90th=[ 545], 99.95th=[ 545], | 99.99th=[ 545] lat (usec) : 750=100.00% cpu : usr=0.00%, sys=200.00%, ctx=1, majf=0, minf=6 IO depths : 1=100.0%, 2=0.0%, 4=0.0%, 8=0.0%, 16=0.0%, 32=0.0%, >=64=0.0% submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0% complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0% issued rwts: total=1,0,0,0 short=0,0,0,0 dropped=0,0,0,0 latency : target=0, window=0, percentile=100.00%, depth=1 Run status group 0 (all jobs): READ: bw=2000KiB/s (2048kB/s), 2000KiB/s-2000KiB/s (2048kB/s-2048kB/s), io=4096B (4096B), run=2-2msec #6: uring-passthru read on ng0n2 (should fail) $: ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -cmd_type=nvme -bs=4k -numjobs=1 -size=4k -filename=/dev/ng0n2 -name=pt pt: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T) 4096B-4096B, ioengine=io_uring_cmd, iodepth=1 fio-3.32-58-gb19c-dirty Starting 1 process Run status group 0 (all jobs): Kanchan Joshi (2): nvme: fine-granular CAP_SYS_ADMIN for nvme io commands nvme: identify-namespace without CAP_SYS_ADMIN drivers/nvme/host/ioctl.c | 107 ++++++++++++++++++++++++++------------ include/linux/nvme.h | 1 + 2 files changed, 75 insertions(+), 33 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <CGME20221020071343epcas5p3722073ab4822d39d6ca91606869f0379@epcas5p3.samsung.com>]
* [PATCH 1/2] nvme: fine-granular CAP_SYS_ADMIN for nvme io commands [not found] ` <CGME20221020071343epcas5p3722073ab4822d39d6ca91606869f0379@epcas5p3.samsung.com> @ 2022-10-20 7:02 ` Kanchan Joshi 0 siblings, 0 replies; 8+ messages in thread From: Kanchan Joshi @ 2022-10-20 7:02 UTC (permalink / raw) To: hch, kbusch, sagi, axboe; +Cc: linux-nvme, gost.dev, Kanchan Joshi Currently both io and admin commands are kept under a coarse-granular CAP_SYS_ADMIN check, disregarding file mode completely. $ ls -l /dev/ng* crw-rw-rw- 1 root root 242, 0 Sep 9 19:20 /dev/ng0n1 crw------- 1 root root 242, 1 Sep 9 19:20 /dev/ng0n2 In the example above, ng0n1 appears as if it may allow unprivileged read/write operation but it does not and behaves same as ng0n2. This patch implements a shift from CAP_SYS_ADMIN to more fine-granular control for io-commands. If CAP_SYS_ADMIN is present, nothing else is checked as before. Otherwise, following rules are in place - any admin-cmd is not allowed - vendor-specific and fabric commmand are not allowed - io-commands that can write are allowed if matching FMODE_WRITE permission is present - io-commands that read are allowed Add a helper nvme_cmd_allowed that implements above policy. Change all the callers of CAP_SYS_ADMIN to go through nvme_cmd_allowed for any decision making. Since file open mode is counted for any approval/denial, change at various places to keep file-mode information handy. Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> --- drivers/nvme/host/ioctl.c | 97 ++++++++++++++++++++++++++------------- include/linux/nvme.h | 1 + 2 files changed, 65 insertions(+), 33 deletions(-) diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c index 81f5550b670d..9c581b1a8956 100644 --- a/drivers/nvme/host/ioctl.c +++ b/drivers/nvme/host/ioctl.c @@ -8,6 +8,29 @@ #include <linux/io_uring.h> #include "nvme.h" +bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c, fmode_t mode) +{ + u8 opcode = c->common.opcode; + + if (capable(CAP_SYS_ADMIN)) + return true; + + /* admin commands are not allowed */ + if (!ns) + return false; + + /* exclude vendor-specific io and fabrics commands */ + if (opcode >= nvme_cmd_vendor_start || opcode == nvme_fabrics_command) + return false; + + /* allow write cmds only if matching FMODE is present */ + if (nvme_is_write(c)) + return mode & FMODE_WRITE; + + /* allow read cmds as the device permission allows access */ + return true; +} + /* * Convert integer values from ioctl structures to user pointers, silently * ignoring the upper bits in the compat case to match behaviour of 32-bit @@ -261,7 +284,7 @@ static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl, } static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns, - struct nvme_passthru_cmd __user *ucmd) + struct nvme_passthru_cmd __user *ucmd, fmode_t mode) { struct nvme_passthru_cmd cmd; struct nvme_command c; @@ -269,8 +292,6 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u64 result; int status; - if (!capable(CAP_SYS_ADMIN)) - return -EACCES; if (copy_from_user(&cmd, ucmd, sizeof(cmd))) return -EFAULT; if (cmd.flags) @@ -291,6 +312,9 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns, c.common.cdw14 = cpu_to_le32(cmd.cdw14); c.common.cdw15 = cpu_to_le32(cmd.cdw15); + if (!nvme_cmd_allowed(ns, &c, mode)) + return -EACCES; + if (cmd.timeout_ms) timeout = msecs_to_jiffies(cmd.timeout_ms); @@ -308,15 +332,14 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns, } static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns, - struct nvme_passthru_cmd64 __user *ucmd, bool vec) + struct nvme_passthru_cmd64 __user *ucmd, bool vec, + fmode_t mode) { struct nvme_passthru_cmd64 cmd; struct nvme_command c; unsigned timeout = 0; int status; - if (!capable(CAP_SYS_ADMIN)) - return -EACCES; if (copy_from_user(&cmd, ucmd, sizeof(cmd))) return -EFAULT; if (cmd.flags) @@ -337,6 +360,9 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns, c.common.cdw14 = cpu_to_le32(cmd.cdw14); c.common.cdw15 = cpu_to_le32(cmd.cdw15); + if (!nvme_cmd_allowed(ns, &c, mode)) + return -EACCES; + if (cmd.timeout_ms) timeout = msecs_to_jiffies(cmd.timeout_ms); @@ -483,9 +509,6 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns, void *meta = NULL; int ret; - if (!capable(CAP_SYS_ADMIN)) - return -EACCES; - c.common.opcode = READ_ONCE(cmd->opcode); c.common.flags = READ_ONCE(cmd->flags); if (c.common.flags) @@ -507,6 +530,9 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns, c.common.cdw14 = cpu_to_le32(READ_ONCE(cmd->cdw14)); c.common.cdw15 = cpu_to_le32(READ_ONCE(cmd->cdw15)); + if (!nvme_cmd_allowed(ns, &c, ioucmd->file->f_mode)) + return -EACCES; + d.metadata = READ_ONCE(cmd->metadata); d.addr = READ_ONCE(cmd->addr); d.data_len = READ_ONCE(cmd->data_len); @@ -570,13 +596,13 @@ static bool is_ctrl_ioctl(unsigned int cmd) } static int nvme_ctrl_ioctl(struct nvme_ctrl *ctrl, unsigned int cmd, - void __user *argp) + void __user *argp, fmode_t mode) { switch (cmd) { case NVME_IOCTL_ADMIN_CMD: - return nvme_user_cmd(ctrl, NULL, argp); + return nvme_user_cmd(ctrl, NULL, argp, mode); case NVME_IOCTL_ADMIN64_CMD: - return nvme_user_cmd64(ctrl, NULL, argp, false); + return nvme_user_cmd64(ctrl, NULL, argp, false, mode); default: return sed_ioctl(ctrl->opal_dev, cmd, argp); } @@ -601,14 +627,14 @@ struct nvme_user_io32 { #endif /* COMPAT_FOR_U64_ALIGNMENT */ static int nvme_ns_ioctl(struct nvme_ns *ns, unsigned int cmd, - void __user *argp) + void __user *argp, fmode_t mode) { switch (cmd) { case NVME_IOCTL_ID: force_successful_syscall_return(); return ns->head->ns_id; case NVME_IOCTL_IO_CMD: - return nvme_user_cmd(ns->ctrl, ns, argp); + return nvme_user_cmd(ns->ctrl, ns, argp, mode); /* * struct nvme_user_io can have different padding on some 32-bit ABIs. * Just accept the compat version as all fields that are used are the @@ -620,19 +646,20 @@ static int nvme_ns_ioctl(struct nvme_ns *ns, unsigned int cmd, case NVME_IOCTL_SUBMIT_IO: return nvme_submit_io(ns, argp); case NVME_IOCTL_IO64_CMD: - return nvme_user_cmd64(ns->ctrl, ns, argp, false); + return nvme_user_cmd64(ns->ctrl, ns, argp, false, mode); case NVME_IOCTL_IO64_CMD_VEC: - return nvme_user_cmd64(ns->ctrl, ns, argp, true); + return nvme_user_cmd64(ns->ctrl, ns, argp, true, mode); default: return -ENOTTY; } } -static int __nvme_ioctl(struct nvme_ns *ns, unsigned int cmd, void __user *arg) +static int __nvme_ioctl(struct nvme_ns *ns, unsigned int cmd, void __user *arg, + fmode_t mode) { - if (is_ctrl_ioctl(cmd)) - return nvme_ctrl_ioctl(ns->ctrl, cmd, arg); - return nvme_ns_ioctl(ns, cmd, arg); + if (is_ctrl_ioctl(cmd)) + return nvme_ctrl_ioctl(ns->ctrl, cmd, arg, mode); + return nvme_ns_ioctl(ns, cmd, arg, mode); } int nvme_ioctl(struct block_device *bdev, fmode_t mode, @@ -640,7 +667,7 @@ int nvme_ioctl(struct block_device *bdev, fmode_t mode, { struct nvme_ns *ns = bdev->bd_disk->private_data; - return __nvme_ioctl(ns, cmd, (void __user *)arg); + return __nvme_ioctl(ns, cmd, (void __user *)arg, mode); } long nvme_ns_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg) @@ -648,7 +675,7 @@ long nvme_ns_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg) struct nvme_ns *ns = container_of(file_inode(file)->i_cdev, struct nvme_ns, cdev); - return __nvme_ioctl(ns, cmd, (void __user *)arg); + return __nvme_ioctl(ns, cmd, (void __user *)arg, file->f_mode); } static int nvme_uring_cmd_checks(unsigned int issue_flags) @@ -716,7 +743,8 @@ int nvme_ns_chr_uring_cmd_iopoll(struct io_uring_cmd *ioucmd, } #ifdef CONFIG_NVME_MULTIPATH static int nvme_ns_head_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd, - void __user *argp, struct nvme_ns_head *head, int srcu_idx) + void __user *argp, struct nvme_ns_head *head, int srcu_idx, + fmode_t mode) __releases(&head->srcu) { struct nvme_ctrl *ctrl = ns->ctrl; @@ -724,7 +752,7 @@ static int nvme_ns_head_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd, nvme_get_ctrl(ns->ctrl); srcu_read_unlock(&head->srcu, srcu_idx); - ret = nvme_ctrl_ioctl(ns->ctrl, cmd, argp); + ret = nvme_ctrl_ioctl(ns->ctrl, cmd, argp, mode); nvme_put_ctrl(ctrl); return ret; @@ -749,9 +777,10 @@ int nvme_ns_head_ioctl(struct block_device *bdev, fmode_t mode, * deadlock when deleting namespaces using the passthrough interface. */ if (is_ctrl_ioctl(cmd)) - return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx); + return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx, + mode); - ret = nvme_ns_ioctl(ns, cmd, argp); + ret = nvme_ns_ioctl(ns, cmd, argp, mode); out_unlock: srcu_read_unlock(&head->srcu, srcu_idx); return ret; @@ -773,9 +802,10 @@ long nvme_ns_head_chr_ioctl(struct file *file, unsigned int cmd, goto out_unlock; if (is_ctrl_ioctl(cmd)) - return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx); + return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx, + file->f_mode); - ret = nvme_ns_ioctl(ns, cmd, argp); + ret = nvme_ns_ioctl(ns, cmd, argp, file->f_mode); out_unlock: srcu_read_unlock(&head->srcu, srcu_idx); return ret; @@ -849,7 +879,8 @@ int nvme_dev_uring_cmd(struct io_uring_cmd *ioucmd, unsigned int issue_flags) return ret; } -static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp) +static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp, + fmode_t mode) { struct nvme_ns *ns; int ret; @@ -873,7 +904,7 @@ static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp) kref_get(&ns->kref); up_read(&ctrl->namespaces_rwsem); - ret = nvme_user_cmd(ctrl, ns, argp); + ret = nvme_user_cmd(ctrl, ns, argp, mode); nvme_put_ns(ns); return ret; @@ -890,11 +921,11 @@ long nvme_dev_ioctl(struct file *file, unsigned int cmd, switch (cmd) { case NVME_IOCTL_ADMIN_CMD: - return nvme_user_cmd(ctrl, NULL, argp); + return nvme_user_cmd(ctrl, NULL, argp, file->f_mode); case NVME_IOCTL_ADMIN64_CMD: - return nvme_user_cmd64(ctrl, NULL, argp, false); + return nvme_user_cmd64(ctrl, NULL, argp, false, file->f_mode); case NVME_IOCTL_IO_CMD: - return nvme_dev_user_cmd(ctrl, argp); + return nvme_dev_user_cmd(ctrl, argp, file->f_mode); case NVME_IOCTL_RESET: if (!capable(CAP_SYS_ADMIN)) return -EACCES; diff --git a/include/linux/nvme.h b/include/linux/nvme.h index 050d7d0cd81b..1d102b662e88 100644 --- a/include/linux/nvme.h +++ b/include/linux/nvme.h @@ -797,6 +797,7 @@ enum nvme_opcode { nvme_cmd_zone_mgmt_send = 0x79, nvme_cmd_zone_mgmt_recv = 0x7a, nvme_cmd_zone_append = 0x7d, + nvme_cmd_vendor_start = 0x80, }; #define nvme_opcode_name(opcode) { opcode, #opcode } -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
[parent not found: <CGME20221020071346epcas5p4c3b8da5e60f94947ad570cbd151eb38d@epcas5p4.samsung.com>]
* [PATCH 2/2] nvme: identify-namespace without CAP_SYS_ADMIN [not found] ` <CGME20221020071346epcas5p4c3b8da5e60f94947ad570cbd151eb38d@epcas5p4.samsung.com> @ 2022-10-20 7:02 ` Kanchan Joshi 2022-10-31 6:55 ` Chaitanya Kulkarni 0 siblings, 1 reply; 8+ messages in thread From: Kanchan Joshi @ 2022-10-20 7:02 UTC (permalink / raw) To: hch, kbusch, sagi, axboe; +Cc: linux-nvme, gost.dev, Kanchan Joshi Allow all identify-namespace variants (CNS 00h, 05h and 08h) without requiring CAP_SYS_ADMIN. The information (retrieved using id-ns) is needed to form IO commands for passthrough interface. Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> --- drivers/nvme/host/ioctl.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c index 9c581b1a8956..9273db147872 100644 --- a/drivers/nvme/host/ioctl.c +++ b/drivers/nvme/host/ioctl.c @@ -15,9 +15,19 @@ bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c, fmode_t mode) if (capable(CAP_SYS_ADMIN)) return true; - /* admin commands are not allowed */ - if (!ns) + /* policy for admin commands */ + if (!ns) { + if (opcode == nvme_admin_identify) { + switch (c->identify.cns) { + case NVME_ID_CNS_NS: + case NVME_ID_CNS_CS_NS: + case NVME_ID_CNS_NS_CS_INDEP: + return true; + } + } + /* other admin commands are not allowed */ return false; + } /* exclude vendor-specific io and fabrics commands */ if (opcode >= nvme_cmd_vendor_start || opcode == nvme_fabrics_command) -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] nvme: identify-namespace without CAP_SYS_ADMIN 2022-10-20 7:02 ` [PATCH 2/2] nvme: identify-namespace without CAP_SYS_ADMIN Kanchan Joshi @ 2022-10-31 6:55 ` Chaitanya Kulkarni 2022-10-31 13:47 ` Kanchan Joshi 0 siblings, 1 reply; 8+ messages in thread From: Chaitanya Kulkarni @ 2022-10-31 6:55 UTC (permalink / raw) To: Kanchan Joshi; +Cc: linux-nvme, hch, axboe, sagi, kbusch, gost.dev On 10/20/22 00:02, Kanchan Joshi wrote: > Allow all identify-namespace variants (CNS 00h, 05h and 08h) without > requiring CAP_SYS_ADMIN. The information (retrieved using id-ns) is > needed to form IO commands for passthrough interface. > > Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> > --- > drivers/nvme/host/ioctl.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c > index 9c581b1a8956..9273db147872 100644 > --- a/drivers/nvme/host/ioctl.c > +++ b/drivers/nvme/host/ioctl.c > @@ -15,9 +15,19 @@ bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c, fmode_t mode) > if (capable(CAP_SYS_ADMIN)) > return true; > > - /* admin commands are not allowed */ > - if (!ns) > + /* policy for admin commands */ above comment is not needed as it is clear from the opcode below you are dealing with admin commands only that too specific cns values .. > + if (!ns) { > + if (opcode == nvme_admin_identify) { > + switch (c->identify.cns) { > + case NVME_ID_CNS_NS: > + case NVME_ID_CNS_CS_NS: > + case NVME_ID_CNS_NS_CS_INDEP: > + return true; > + } > + } > + /* other admin commands are not allowed */ same here.. > return false; if and swicth and two returns are looking confusing, I'd use nested switch case default here.. > + } > > /* exclude vendor-specific io and fabrics commands */ > if (opcode >= nvme_cmd_vendor_start || opcode == nvme_fabrics_command) -ck ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] nvme: identify-namespace without CAP_SYS_ADMIN 2022-10-31 6:55 ` Chaitanya Kulkarni @ 2022-10-31 13:47 ` Kanchan Joshi 2022-11-01 5:20 ` Chaitanya Kulkarni 0 siblings, 1 reply; 8+ messages in thread From: Kanchan Joshi @ 2022-10-31 13:47 UTC (permalink / raw) To: Chaitanya Kulkarni; +Cc: linux-nvme, hch, axboe, sagi, kbusch, gost.dev [-- Attachment #1: Type: text/plain, Size: 2309 bytes --] On Mon, Oct 31, 2022 at 06:55:56AM +0000, Chaitanya Kulkarni wrote: >On 10/20/22 00:02, Kanchan Joshi wrote: >> Allow all identify-namespace variants (CNS 00h, 05h and 08h) without >> requiring CAP_SYS_ADMIN. The information (retrieved using id-ns) is >> needed to form IO commands for passthrough interface. >> >> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> >> --- >> drivers/nvme/host/ioctl.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c >> index 9c581b1a8956..9273db147872 100644 >> --- a/drivers/nvme/host/ioctl.c >> +++ b/drivers/nvme/host/ioctl.c >> @@ -15,9 +15,19 @@ bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c, fmode_t mode) >> if (capable(CAP_SYS_ADMIN)) >> return true; >> >> - /* admin commands are not allowed */ >> - if (!ns) >> + /* policy for admin commands */ > >above comment is not needed as it is clear from the opcode below >you are dealing with admin commands only that too specific cns >values .. > >> + if (!ns) { >> + if (opcode == nvme_admin_identify) { >> + switch (c->identify.cns) { >> + case NVME_ID_CNS_NS: >> + case NVME_ID_CNS_CS_NS: >> + case NVME_ID_CNS_NS_CS_INDEP: >> + return true; >> + } >> + } >> + /* other admin commands are not allowed */ > >same here.. All right, will kill these. > >> return false; > >if and swicth and two returns are looking confusing, I'd use >nested switch case default here.. Do you think that'll give better looking code? I did not write that because it did not seem good fit for the situtation. It involved aligning more curly braces: - /* admin commands are not allowed */ - if (!ns) + if (!ns) { + switch (opcode) { + case nvme_admin_identify: { + switch (c->identify.cns) { + case NVME_ID_CNS_NS: + case NVME_ID_CNS_CS_NS: + case NVME_ID_CNS_NS_CS_INDEP: + return true; + } + } + } return false; + } Above is without default. And with two defaults, it just gets more wordy. And future growth in above admin opcodes is not expected too. [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] nvme: identify-namespace without CAP_SYS_ADMIN 2022-10-31 13:47 ` Kanchan Joshi @ 2022-11-01 5:20 ` Chaitanya Kulkarni 0 siblings, 0 replies; 8+ messages in thread From: Chaitanya Kulkarni @ 2022-11-01 5:20 UTC (permalink / raw) To: Kanchan Joshi; +Cc: linux-nvme, hch, axboe, sagi, kbusch, gost.dev > Do you think that'll give better looking code? > I did not write that because it did not seem good fit for the > situtation. It involved aligning more curly braces: > - /* admin commands are not allowed */ > - if (!ns) > + if (!ns) { > + switch (opcode) { > + case nvme_admin_identify: { > + switch (c->identify.cns) { > + case NVME_ID_CNS_NS: > + case NVME_ID_CNS_CS_NS: > + case NVME_ID_CNS_NS_CS_INDEP: > + return true; > + } > + } > + } > return false; > + } > this is exact pattern we have used in the target when it comes to nvme_admin_identify handling, so it makes sense to keep the same, unless there is any strong objection.. > Above is without default. And with two defaults, it just gets more > wordy. > And future growth in above admin opcodes is not expected too. > -ck ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] Granular CAP_SYS_ADMIN 2022-10-20 7:02 ` [PATCH 0/2] Granular CAP_SYS_ADMIN Kanchan Joshi [not found] ` <CGME20221020071343epcas5p3722073ab4822d39d6ca91606869f0379@epcas5p3.samsung.com> [not found] ` <CGME20221020071346epcas5p4c3b8da5e60f94947ad570cbd151eb38d@epcas5p4.samsung.com> @ 2022-10-25 19:43 ` Jens Axboe 2022-10-25 20:07 ` Keith Busch 3 siblings, 0 replies; 8+ messages in thread From: Jens Axboe @ 2022-10-25 19:43 UTC (permalink / raw) To: Kanchan Joshi, hch, kbusch, sagi; +Cc: linux-nvme, gost.dev On 10/20/22 1:02 AM, Kanchan Joshi wrote: > Hi, > > Patch 1 is for io-commands. It implements the shift to file-mode based > policy. > Patch 2 is to allow identify-namespace command. This is based on the feedback > received during ALPSS. > > @Sagi: Since patch 1 is changed a bit (changelog below), I did not > apply the reviewed-by tag. Please take a look again. I generally despise any kind of listing of commands, but this looks pretty reasonable. Reviewed-by: Jens Axboe <axboe@kernel.dk> -- Jens Axboe ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] Granular CAP_SYS_ADMIN 2022-10-20 7:02 ` [PATCH 0/2] Granular CAP_SYS_ADMIN Kanchan Joshi ` (2 preceding siblings ...) 2022-10-25 19:43 ` [PATCH 0/2] Granular CAP_SYS_ADMIN Jens Axboe @ 2022-10-25 20:07 ` Keith Busch 3 siblings, 0 replies; 8+ messages in thread From: Keith Busch @ 2022-10-25 20:07 UTC (permalink / raw) To: Kanchan Joshi; +Cc: hch, sagi, axboe, linux-nvme, gost.dev On Thu, Oct 20, 2022 at 12:32:03PM +0530, Kanchan Joshi wrote: > #1: Two NS, one with 666 another with 600 > $: ls -l /dev/ng* > crw-rw-rw- 1 root root 242, 0 Oct 20 12:04 /dev/ng0n1 > crw------- 1 root root 242, 1 Oct 20 12:04 /dev/ng0n2 > > #2: this should fail > $: nvme id-ns /dev/ng0n2 > /dev/ng0n2: Permission denied This looks good to me. The only additional thought I had was to restrict the identification's nsid parameter to ones readable to the user because they could just open /dev/ng0n1 instead and override the nsid with the '--nampespace-id=2' parameter. But that doesn't look very straight now that I look at it again. Reviewed-by: Keith Busch <kbusch@kernel.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-11-01 5:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20221020071338epcas5p16d72f5d4d868b889e3a98688bc454a98@epcas5p1.samsung.com> 2022-10-20 7:02 ` [PATCH 0/2] Granular CAP_SYS_ADMIN Kanchan Joshi [not found] ` <CGME20221020071343epcas5p3722073ab4822d39d6ca91606869f0379@epcas5p3.samsung.com> 2022-10-20 7:02 ` [PATCH 1/2] nvme: fine-granular CAP_SYS_ADMIN for nvme io commands Kanchan Joshi [not found] ` <CGME20221020071346epcas5p4c3b8da5e60f94947ad570cbd151eb38d@epcas5p4.samsung.com> 2022-10-20 7:02 ` [PATCH 2/2] nvme: identify-namespace without CAP_SYS_ADMIN Kanchan Joshi 2022-10-31 6:55 ` Chaitanya Kulkarni 2022-10-31 13:47 ` Kanchan Joshi 2022-11-01 5:20 ` Chaitanya Kulkarni 2022-10-25 19:43 ` [PATCH 0/2] Granular CAP_SYS_ADMIN Jens Axboe 2022-10-25 20:07 ` Keith Busch
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.