* [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands [not found] <CGME20220719135821epcas5p1b071b0162cc3e1eb803ca687989f106d@epcas5p1.samsung.com> @ 2022-07-19 13:52 ` Ankit Kumar [not found] ` <CGME20220719135832epcas5p31bb7df7c931aba12454b6f16c966a7c8@epcas5p3.samsung.com> ` (5 more replies) 0 siblings, 6 replies; 25+ messages in thread From: Ankit Kumar @ 2022-07-19 13:52 UTC (permalink / raw) To: axboe; +Cc: io-uring, paul, casey, joshi.k, Ankit Kumar This patchset adds test/io_uring_passthrough.c to submit uring passthrough commands to nvme-ns character device. The uring passthrough was introduced with 5.19 io_uring. To send nvme uring passthrough commands we require helpers to fetch NVMe char device (/dev/ngXnY) specific fields such as namespace id, lba size. How to run: ./test/io_uring_passthrough.t /dev/ng0n1 This covers basic write/read with verify for sqthread poll, vectored / nonvectored and fixed IO buffers, which can be easily extended in future. Ankit Kumar (5): configure: check for nvme uring command support io_uring.h: sync sqe entry with 5.20 io_uring nvme: add nvme opcodes, structures and helper functions test: add io_uring passthrough test test/io_uring_passthrough: add test case for poll IO configure | 20 ++ src/include/liburing/io_uring.h | 17 +- test/Makefile | 1 + test/io_uring_passthrough.c | 395 ++++++++++++++++++++++++++++++++ test/nvme.h | 168 ++++++++++++++ 5 files changed, 599 insertions(+), 2 deletions(-) create mode 100644 test/io_uring_passthrough.c create mode 100644 test/nvme.h -- 2.17.1 ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <CGME20220719135832epcas5p31bb7df7c931aba12454b6f16c966a7c8@epcas5p3.samsung.com>]
* [PATCH liburing 1/5] configure: check for nvme uring command support [not found] ` <CGME20220719135832epcas5p31bb7df7c931aba12454b6f16c966a7c8@epcas5p3.samsung.com> @ 2022-07-19 13:52 ` Ankit Kumar 0 siblings, 0 replies; 25+ messages in thread From: Ankit Kumar @ 2022-07-19 13:52 UTC (permalink / raw) To: axboe; +Cc: io-uring, paul, casey, joshi.k, Ankit Kumar Modify configure to check availability of nvme_uring_cmd. The follow up patch will have uring passthrough tests. Signed-off-by: Ankit Kumar <[email protected]> --- configure | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/configure b/configure index 43071dd..1b0cc50 100755 --- a/configure +++ b/configure @@ -367,6 +367,23 @@ if compile_prog "" "" "has_ucontext"; then fi print_config "has_ucontext" "$has_ucontext" +########################################## +# Check NVME_URING_CMD support +nvme_uring_cmd="no" +cat > $TMPC << EOF +#include <linux/nvme_ioctl.h> +int main(void) +{ + struct nvme_uring_cmd *cmd; + + return sizeof(struct nvme_uring_cmd); +} +EOF +if compile_prog "" "" "nvme uring cmd"; then + nvme_uring_cmd="yes" +fi +print_config "NVMe uring command support" "$nvme_uring_cmd" + ############################################################################# if test "$liburing_nolibc" = "yes"; then output_sym "CONFIG_NOLIBC" @@ -402,6 +419,9 @@ fi if test "$array_bounds" = "yes"; then output_sym "CONFIG_HAVE_ARRAY_BOUNDS" fi +if test "$nvme_uring_cmd" = "yes"; then + output_sym "CONFIG_HAVE_NVME_URING" +fi echo "CC=$cc" >> $config_host_mak print_config "CC" "$cc" -- 2.17.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
[parent not found: <CGME20220719135834epcas5p2f63a49277322756394f19e23a1c4e4ce@epcas5p2.samsung.com>]
* [PATCH liburing 2/5] io_uring.h: sync sqe entry with 5.20 io_uring [not found] ` <CGME20220719135834epcas5p2f63a49277322756394f19e23a1c4e4ce@epcas5p2.samsung.com> @ 2022-07-19 13:52 ` Ankit Kumar 0 siblings, 0 replies; 25+ messages in thread From: Ankit Kumar @ 2022-07-19 13:52 UTC (permalink / raw) To: axboe; +Cc: io-uring, paul, casey, joshi.k, Ankit Kumar Add a few missing fields which was added for uring command Signed-off-by: Ankit Kumar <[email protected]> --- src/include/liburing/io_uring.h | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/include/liburing/io_uring.h b/src/include/liburing/io_uring.h index 51126c4..581381d 100644 --- a/src/include/liburing/io_uring.h +++ b/src/include/liburing/io_uring.h @@ -26,6 +26,10 @@ struct io_uring_sqe { union { __u64 off; /* offset into file */ __u64 addr2; + struct { + __u32 cmd_op; + __u32 __pad1; + }; }; union { __u64 addr; /* pointer to buffer or iovecs */ @@ -65,8 +69,17 @@ struct io_uring_sqe { __s32 splice_fd_in; __u32 file_index; }; - __u64 addr3; - __u64 __pad2[1]; + union { + struct { + __u64 addr3; + __u64 __pad2[1]; + }; + /* + * If the ring is initialized with IORING_SETUP_SQE128, then + * this field is used for 80 bytes of arbitrary command data + */ + __u8 cmd[0]; + }; }; /* -- 2.17.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
[parent not found: <CGME20220719135835epcas5p2284cbb16a28c4290d3a886449bc7a6d8@epcas5p2.samsung.com>]
* [PATCH liburing 3/5] nvme: add nvme opcodes, structures and helper functions [not found] ` <CGME20220719135835epcas5p2284cbb16a28c4290d3a886449bc7a6d8@epcas5p2.samsung.com> @ 2022-07-19 13:52 ` Ankit Kumar 0 siblings, 0 replies; 25+ messages in thread From: Ankit Kumar @ 2022-07-19 13:52 UTC (permalink / raw) To: axboe; +Cc: io-uring, paul, casey, joshi.k, Ankit Kumar Add bare minimum structures and helper functions required for io_uring passthrough commands. This will enable the follow up patch to add tests for nvme-ns generic character device. Signed-off-by: Ankit Kumar <[email protected]> --- test/nvme.h | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 test/nvme.h diff --git a/test/nvme.h b/test/nvme.h new file mode 100644 index 0000000..866a7e6 --- /dev/null +++ b/test/nvme.h @@ -0,0 +1,168 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Description: Helpers for NVMe uring passthrough commands + */ +#ifndef LIBURING_NVME_H +#define LIBURING_NVME_H + +#ifdef __cplusplus +extern "C" { +#endif + +#include <sys/ioctl.h> +#include <linux/nvme_ioctl.h> + +/* + * If the uapi headers installed on the system lacks nvme uring command + * support, use the local version to prevent compilation issues. + */ +#ifndef CONFIG_HAVE_NVME_URING +struct nvme_uring_cmd { + __u8 opcode; + __u8 flags; + __u16 rsvd1; + __u32 nsid; + __u32 cdw2; + __u32 cdw3; + __u64 metadata; + __u64 addr; + __u32 metadata_len; + __u32 data_len; + __u32 cdw10; + __u32 cdw11; + __u32 cdw12; + __u32 cdw13; + __u32 cdw14; + __u32 cdw15; + __u32 timeout_ms; + __u32 rsvd2; +}; + +#define NVME_URING_CMD_IO _IOWR('N', 0x80, struct nvme_uring_cmd) +#define NVME_URING_CMD_IO_VEC _IOWR('N', 0x81, struct nvme_uring_cmd) +#endif /* CONFIG_HAVE_NVME_URING */ + +#define NVME_DEFAULT_IOCTL_TIMEOUT 0 +#define NVME_IDENTIFY_DATA_SIZE 4096 +#define NVME_IDENTIFY_CSI_SHIFT 24 +#define NVME_IDENTIFY_CNS_NS 0 +#define NVME_CSI_NVM 0 + +enum nvme_admin_opcode { + nvme_admin_identify = 0x06, +}; + +enum nvme_io_opcode { + nvme_cmd_write = 0x01, + nvme_cmd_read = 0x02, +}; + +int nsid; +__u32 lba_shift; + +struct nvme_lbaf { + __le16 ms; + __u8 ds; + __u8 rp; +}; + +struct nvme_id_ns { + __le64 nsze; + __le64 ncap; + __le64 nuse; + __u8 nsfeat; + __u8 nlbaf; + __u8 flbas; + __u8 mc; + __u8 dpc; + __u8 dps; + __u8 nmic; + __u8 rescap; + __u8 fpi; + __u8 dlfeat; + __le16 nawun; + __le16 nawupf; + __le16 nacwu; + __le16 nabsn; + __le16 nabo; + __le16 nabspf; + __le16 noiob; + __u8 nvmcap[16]; + __le16 npwg; + __le16 npwa; + __le16 npdg; + __le16 npda; + __le16 nows; + __le16 mssrl; + __le32 mcl; + __u8 msrc; + __u8 rsvd81[11]; + __le32 anagrpid; + __u8 rsvd96[3]; + __u8 nsattr; + __le16 nvmsetid; + __le16 endgid; + __u8 nguid[16]; + __u8 eui64[8]; + struct nvme_lbaf lbaf[16]; + __u8 rsvd192[192]; + __u8 vs[3712]; +}; + +static inline int ilog2(uint32_t i) +{ + int log = -1; + + while (i) { + i >>= 1; + log++; + } + return log; +} + +int fio_nvme_get_info(const char *file) +{ + struct nvme_id_ns ns; + int fd, err; + __u32 lba_size; + + fd = open(file, O_RDONLY); + if (fd < 0) + return -errno; + + nsid = ioctl(fd, NVME_IOCTL_ID); + if (nsid < 0) { + fprintf(stderr, "failed to fetch namespace-id\n"); + close(fd); + return -errno; + } + + struct nvme_passthru_cmd cmd = { + .opcode = nvme_admin_identify, + .nsid = nsid, + .addr = (__u64)(uintptr_t)&ns, + .data_len = NVME_IDENTIFY_DATA_SIZE, + .cdw10 = NVME_IDENTIFY_CNS_NS, + .cdw11 = NVME_CSI_NVM << NVME_IDENTIFY_CSI_SHIFT, + .timeout_ms = NVME_DEFAULT_IOCTL_TIMEOUT, + }; + + err = ioctl(fd, NVME_IOCTL_ADMIN_CMD, &cmd); + if (err) { + fprintf(stderr, "failed to fetch identify namespace\n"); + close(fd); + return err; + } + + lba_size = 1 << ns.lbaf[(ns.flbas & 0x0f)].ds; + lba_shift = ilog2(lba_size); + + close(fd); + return 0; +} + +#ifdef __cplusplus +} +#endif + +#endif -- 2.17.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
[parent not found: <CGME20220719135836epcas5p3f28b20cab964ced538d5a7fdfe367bb4@epcas5p3.samsung.com>]
* [PATCH liburing 4/5] test: add io_uring passthrough test [not found] ` <CGME20220719135836epcas5p3f28b20cab964ced538d5a7fdfe367bb4@epcas5p3.samsung.com> @ 2022-07-19 13:52 ` Ankit Kumar 0 siblings, 0 replies; 25+ messages in thread From: Ankit Kumar @ 2022-07-19 13:52 UTC (permalink / raw) To: axboe; +Cc: io-uring, paul, casey, joshi.k, Ankit Kumar Add a way to test uring passthrough commands, which was added with 5.19 kernel. This requires nvme-ns character device (/dev/ngXnY) as filename argument. It runs a combination of read/write tests with sqthread poll, vectored and non-vectored commands, fixed I/O buffers. Signed-off-by: Ankit Kumar <[email protected]> --- test/Makefile | 1 + test/io_uring_passthrough.c | 319 ++++++++++++++++++++++++++++++++++++ 2 files changed, 320 insertions(+) create mode 100644 test/io_uring_passthrough.c diff --git a/test/Makefile b/test/Makefile index 45674c3..8cafcfd 100644 --- a/test/Makefile +++ b/test/Makefile @@ -90,6 +90,7 @@ test_srcs := \ io-cancel.c \ iopoll.c \ io_uring_enter.c \ + io_uring_passthrough.c \ io_uring_register.c \ io_uring_setup.c \ lfs-openat.c \ diff --git a/test/io_uring_passthrough.c b/test/io_uring_passthrough.c new file mode 100644 index 0000000..2e2b806 --- /dev/null +++ b/test/io_uring_passthrough.c @@ -0,0 +1,319 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Description: basic read/write tests for io_uring passthrough commands + */ +#include <errno.h> +#include <stdio.h> +#include <unistd.h> +#include <stdlib.h> +#include <string.h> + +#include "helpers.h" +#include "liburing.h" +#include "nvme.h" + +#define FILE_SIZE (256 * 1024) +#define BS 8192 +#define BUFFERS (FILE_SIZE / BS) + +static struct iovec *vecs; + +/* + * Each offset in the file has the ((test_case / 2) * FILE_SIZE) + * + (offset / sizeof(int)) stored for every + * sizeof(int) address. + */ +static int verify_buf(int tc, void *buf, off_t off) +{ + int i, u_in_buf = BS / sizeof(unsigned int); + unsigned int *ptr; + + off /= sizeof(unsigned int); + off += (tc / 2) * FILE_SIZE; + ptr = buf; + for (i = 0; i < u_in_buf; i++) { + if (off != *ptr) { + fprintf(stderr, "Found %u, wanted %lu\n", *ptr, off); + return 1; + } + ptr++; + off++; + } + + return 0; +} + +static int fill_pattern(int tc) +{ + unsigned int val, *ptr; + int i, j; + int u_in_buf = BS / sizeof(val); + + val = (tc / 2) * FILE_SIZE; + for (i = 0; i < BUFFERS; i++) { + ptr = vecs[i].iov_base; + for (j = 0; j < u_in_buf; j++) { + *ptr = val; + val++; + ptr++; + } + } + + return 0; +} + +static int __test_io(const char *file, struct io_uring *ring, int tc, int read, + int sqthread, int fixed, int nonvec) +{ + struct io_uring_sqe *sqe; + struct io_uring_cqe *cqe; + struct nvme_uring_cmd *cmd; + int open_flags; + int do_fixed; + int i, ret, fd = -1; + off_t offset; + __u64 slba; + __u32 nlb; + +#ifdef VERBOSE + fprintf(stdout, "%s: start %d/%d/%d/%d: ", __FUNCTION__, read, + sqthread, fixed, + nonvec); +#endif + if (read) + open_flags = O_RDONLY; + else + open_flags = O_WRONLY; + + if (fixed) { + ret = t_register_buffers(ring, vecs, BUFFERS); + if (ret == T_SETUP_SKIP) + return 0; + if (ret != T_SETUP_OK) { + fprintf(stderr, "buffer reg failed: %d\n", ret); + goto err; + } + } + + fd = open(file, open_flags); + if (fd < 0) { + perror("file open"); + goto err; + } + + if (sqthread) { + ret = io_uring_register_files(ring, &fd, 1); + if (ret) { + fprintf(stderr, "file reg failed: %d\n", ret); + goto err; + } + } + + if (!read) + fill_pattern(tc); + + offset = 0; + for (i = 0; i < BUFFERS; i++) { + sqe = io_uring_get_sqe(ring); + if (!sqe) { + fprintf(stderr, "sqe get failed\n"); + goto err; + } + if (read) { + int use_fd = fd; + + do_fixed = fixed; + + if (sqthread) + use_fd = 0; + if (fixed && (i & 1)) + do_fixed = 0; + if (do_fixed) { + io_uring_prep_read_fixed(sqe, use_fd, vecs[i].iov_base, + vecs[i].iov_len, + offset, i); + sqe->cmd_op = NVME_URING_CMD_IO; + } else if (nonvec) { + io_uring_prep_read(sqe, use_fd, vecs[i].iov_base, + vecs[i].iov_len, offset); + sqe->cmd_op = NVME_URING_CMD_IO; + } else { + io_uring_prep_readv(sqe, use_fd, &vecs[i], 1, + offset); + sqe->cmd_op = NVME_URING_CMD_IO_VEC; + } + } else { + int use_fd = fd; + + do_fixed = fixed; + + if (sqthread) + use_fd = 0; + if (fixed && (i & 1)) + do_fixed = 0; + if (do_fixed) { + io_uring_prep_write_fixed(sqe, use_fd, vecs[i].iov_base, + vecs[i].iov_len, + offset, i); + sqe->cmd_op = NVME_URING_CMD_IO; + } else if (nonvec) { + io_uring_prep_write(sqe, use_fd, vecs[i].iov_base, + vecs[i].iov_len, offset); + sqe->cmd_op = NVME_URING_CMD_IO; + } else { + io_uring_prep_writev(sqe, use_fd, &vecs[i], 1, + offset); + sqe->cmd_op = NVME_URING_CMD_IO_VEC; + } + } + sqe->opcode = IORING_OP_URING_CMD; + sqe->user_data = ((uint64_t)offset << 32) | i; + if (sqthread) + sqe->flags |= IOSQE_FIXED_FILE; + + /* 80 bytes for NVMe uring passthrough command */ + cmd = (struct nvme_uring_cmd *)sqe->cmd; + memset(cmd, 0, sizeof(struct nvme_uring_cmd)); + + cmd->opcode = read ? nvme_cmd_read : nvme_cmd_write; + + slba = offset >> lba_shift; + nlb = (BS >> lba_shift) - 1; + + /* cdw10 and cdw11 represent starting lba */ + cmd->cdw10 = slba & 0xffffffff; + cmd->cdw11 = slba >> 32; + /* cdw12 represent number of lba's for read/write */ + cmd->cdw12 = nlb; + if (do_fixed || nonvec) { + cmd->addr = (__u64)(uintptr_t)vecs[i].iov_base; + cmd->data_len = vecs[i].iov_len; + } else { + cmd->addr = (__u64)(uintptr_t)&vecs[i]; + cmd->data_len = 1; + } + cmd->nsid = nsid; + + offset += BS; + } + + ret = io_uring_submit(ring); + if (ret != BUFFERS) { + fprintf(stderr, "submit got %d, wanted %d\n", ret, BUFFERS); + goto err; + } + + for (i = 0; i < BUFFERS; i++) { + ret = io_uring_wait_cqe(ring, &cqe); + if (ret) { + fprintf(stderr, "wait_cqe=%d\n", ret); + goto err; + } + if (cqe->res != 0) { + fprintf(stderr, "cqe res %d, wanted 0\n", cqe->res); + goto err; + } + io_uring_cqe_seen(ring, cqe); + if (read) { + int index = cqe->user_data & 0xffffffff; + void *buf = vecs[index].iov_base; + off_t voff = cqe->user_data >> 32; + + if (verify_buf(tc, buf, voff)) + goto err; + } + } + + if (fixed) { + ret = io_uring_unregister_buffers(ring); + if (ret) { + fprintf(stderr, "buffer unreg failed: %d\n", ret); + goto err; + } + } + if (sqthread) { + ret = io_uring_unregister_files(ring); + if (ret) { + fprintf(stderr, "file unreg failed: %d\n", ret); + goto err; + } + } + + close(fd); +#ifdef VERBOSE + fprintf(stdout, "PASS\n"); +#endif + return 0; +err: +#ifdef VERBOSE + fprintf(stderr, "FAILED\n"); +#endif + if (fd != -1) + close(fd); + return 1; +} + +static int test_io(const char *file, int tc, int read, int sqthread, + int fixed, int nonvec) +{ + struct io_uring ring; + int ret, ring_flags = 0; + + ring_flags |= IORING_SETUP_SQE128; + ring_flags |= IORING_SETUP_CQE32; + + if (sqthread) + ring_flags |= IORING_SETUP_SQPOLL; + + ret = t_create_ring(64, &ring, ring_flags); + if (ret == T_SETUP_SKIP) + return 0; + if (ret != T_SETUP_OK) { + fprintf(stderr, "ring create failed: %d\n", ret); + return 1; + } + + ret = __test_io(file, &ring, tc, read, sqthread, fixed, nonvec); + io_uring_queue_exit(&ring); + + return ret; +} + +int main(int argc, char *argv[]) +{ + int i, ret; + char *fname; + + if (argc < 2) { + printf("%s: requires NVMe character device\n", argv[0]); + return T_EXIT_SKIP; + } + + fname = argv[1]; + ret = fio_nvme_get_info(fname); + + if (ret) { + fprintf(stderr, "failed to fetch device info: %d\n", ret); + goto err; + } + + vecs = t_create_buffers(BUFFERS, BS); + + for (i = 0; i < 16; i++) { + int read = (i & 1) != 0; + int sqthread = (i & 2) != 0; + int fixed = (i & 4) != 0; + int nonvec = (i & 8) != 0; + + ret = test_io(fname, i, read, sqthread, fixed, nonvec); + if (ret) { + fprintf(stderr, "test_io failed %d/%d/%d/%d\n", + read, sqthread, fixed, nonvec); + goto err; + } + } + + return T_EXIT_PASS; +err: + return T_EXIT_FAIL; +} -- 2.17.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
[parent not found: <CGME20220719135837epcas5p1eb4beb20bdfbdaaa7409d7b1f6355909@epcas5p1.samsung.com>]
* [PATCH liburing 5/5] test/io_uring_passthrough: add test case for poll IO [not found] ` <CGME20220719135837epcas5p1eb4beb20bdfbdaaa7409d7b1f6355909@epcas5p1.samsung.com> @ 2022-07-19 13:52 ` Ankit Kumar 0 siblings, 0 replies; 25+ messages in thread From: Ankit Kumar @ 2022-07-19 13:52 UTC (permalink / raw) To: axboe; +Cc: io-uring, paul, casey, joshi.k, Ankit Kumar For uring passthrough add test case for poll IO completion. If poll IO is not supported return success. Signed-off-by: Ankit Kumar <[email protected]> --- test/io_uring_passthrough.c | 76 +++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/test/io_uring_passthrough.c b/test/io_uring_passthrough.c index 2e2b806..acb57f8 100644 --- a/test/io_uring_passthrough.c +++ b/test/io_uring_passthrough.c @@ -10,6 +10,7 @@ #include "helpers.h" #include "liburing.h" +#include "../src/syscall.h" #include "nvme.h" #define FILE_SIZE (256 * 1024) @@ -279,6 +280,75 @@ static int test_io(const char *file, int tc, int read, int sqthread, return ret; } +extern int __io_uring_flush_sq(struct io_uring *ring); + +/* + * if we are polling io_uring_submit needs to always enter the + * kernel to fetch events + */ +static int test_io_uring_submit_enters(const char *file) +{ + struct io_uring ring; + int fd, i, ret, ring_flags, open_flags; + unsigned head; + struct io_uring_cqe *cqe; + + ring_flags = IORING_SETUP_IOPOLL; + ring_flags |= IORING_SETUP_SQE128; + ring_flags |= IORING_SETUP_CQE32; + + ret = io_uring_queue_init(64, &ring, ring_flags); + if (ret) { + fprintf(stderr, "ring create failed: %d\n", ret); + return 1; + } + + open_flags = O_WRONLY; + fd = open(file, open_flags); + if (fd < 0) { + perror("file open"); + goto err; + } + + for (i = 0; i < BUFFERS; i++) { + struct io_uring_sqe *sqe; + off_t offset = BS * (rand() % BUFFERS); + + sqe = io_uring_get_sqe(&ring); + io_uring_prep_writev(sqe, fd, &vecs[i], 1, offset); + sqe->user_data = 1; + } + + /* submit manually to avoid adding IORING_ENTER_GETEVENTS */ + ret = __sys_io_uring_enter(ring.ring_fd, __io_uring_flush_sq(&ring), 0, + 0, NULL); + if (ret < 0) + goto err; + + for (i = 0; i < 500; i++) { + ret = io_uring_submit(&ring); + if (ret != 0) { + fprintf(stderr, "still had %d sqes to submit, this is unexpected", ret); + goto err; + } + + io_uring_for_each_cqe(&ring, head, cqe) { + if (cqe->res == -EOPNOTSUPP) + fprintf(stdout, "doesn't support polled IO\n"); + goto ok; + } + usleep(10000); + } +err: + ret = 1; + if (fd != -1) + close(fd); + +ok: + io_uring_queue_exit(&ring); + return ret; +} + int main(int argc, char *argv[]) { int i, ret; @@ -313,6 +383,12 @@ int main(int argc, char *argv[]) } } + ret = test_io_uring_submit_enters(fname); + if (ret) { + fprintf(stderr, "test_io_uring_submit_enters failed\n"); + goto err; + } + return T_EXIT_PASS; err: return T_EXIT_FAIL; -- 2.17.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands 2022-07-19 13:52 ` [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands Ankit Kumar ` (4 preceding siblings ...) [not found] ` <CGME20220719135837epcas5p1eb4beb20bdfbdaaa7409d7b1f6355909@epcas5p1.samsung.com> @ 2022-08-12 0:43 ` Casey Schaufler 2022-08-12 1:51 ` Jens Axboe 5 siblings, 1 reply; 25+ messages in thread From: Casey Schaufler @ 2022-08-12 0:43 UTC (permalink / raw) To: Ankit Kumar, axboe; +Cc: io-uring, paul, joshi.k, casey On 7/19/2022 6:52 AM, Ankit Kumar wrote: > This patchset adds test/io_uring_passthrough.c to submit uring passthrough > commands to nvme-ns character device. The uring passthrough was introduced > with 5.19 io_uring. > > To send nvme uring passthrough commands we require helpers to fetch NVMe > char device (/dev/ngXnY) specific fields such as namespace id, lba size. There wouldn't be a way to run these tests using a more general configuration, would there? I spent way too much time trying to coax my systems into pretending it has this device. > > How to run: > ./test/io_uring_passthrough.t /dev/ng0n1 > > This covers basic write/read with verify for sqthread poll, > vectored / nonvectored and fixed IO buffers, which can be easily extended > in future. > > Ankit Kumar (5): > configure: check for nvme uring command support > io_uring.h: sync sqe entry with 5.20 io_uring > nvme: add nvme opcodes, structures and helper functions > test: add io_uring passthrough test > test/io_uring_passthrough: add test case for poll IO > > configure | 20 ++ > src/include/liburing/io_uring.h | 17 +- > test/Makefile | 1 + > test/io_uring_passthrough.c | 395 ++++++++++++++++++++++++++++++++ > test/nvme.h | 168 ++++++++++++++ > 5 files changed, 599 insertions(+), 2 deletions(-) > create mode 100644 test/io_uring_passthrough.c > create mode 100644 test/nvme.h > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands 2022-08-12 0:43 ` [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands Casey Schaufler @ 2022-08-12 1:51 ` Jens Axboe 2022-08-12 15:33 ` Paul Moore 0 siblings, 1 reply; 25+ messages in thread From: Jens Axboe @ 2022-08-12 1:51 UTC (permalink / raw) To: Casey Schaufler, Ankit Kumar; +Cc: io-uring, paul, joshi.k On 8/11/22 6:43 PM, Casey Schaufler wrote: > On 7/19/2022 6:52 AM, Ankit Kumar wrote: >> This patchset adds test/io_uring_passthrough.c to submit uring passthrough >> commands to nvme-ns character device. The uring passthrough was introduced >> with 5.19 io_uring. >> >> To send nvme uring passthrough commands we require helpers to fetch NVMe >> char device (/dev/ngXnY) specific fields such as namespace id, lba size. > > There wouldn't be a way to run these tests using a more general > configuration, would there? I spent way too much time trying to > coax my systems into pretending it has this device. It's only plumbed up for nvme. Just use qemu with an nvme device? -drive id=drv1,if=none,file=nvme.img,aio=io_uring,cache.direct=on,discard=on \ -device nvme,drive=drv1,serial=blah2 Paul was pondering wiring up a no-op kind of thing for null, though. -- Jens Axboe ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands 2022-08-12 1:51 ` Jens Axboe @ 2022-08-12 15:33 ` Paul Moore 2022-08-12 16:03 ` Casey Schaufler 2022-08-23 23:46 ` [PATCH] Smack: Provide read control for io_uring_cmd Casey Schaufler 0 siblings, 2 replies; 25+ messages in thread From: Paul Moore @ 2022-08-12 15:33 UTC (permalink / raw) To: Jens Axboe; +Cc: Casey Schaufler, Ankit Kumar, io-uring, joshi.k On Thu, Aug 11, 2022 at 9:51 PM Jens Axboe <[email protected]> wrote: > On 8/11/22 6:43 PM, Casey Schaufler wrote: > > On 7/19/2022 6:52 AM, Ankit Kumar wrote: > >> This patchset adds test/io_uring_passthrough.c to submit uring passthrough > >> commands to nvme-ns character device. The uring passthrough was introduced > >> with 5.19 io_uring. > >> > >> To send nvme uring passthrough commands we require helpers to fetch NVMe > >> char device (/dev/ngXnY) specific fields such as namespace id, lba size. > > > > There wouldn't be a way to run these tests using a more general > > configuration, would there? I spent way too much time trying to > > coax my systems into pretending it has this device. > > It's only plumbed up for nvme. Just use qemu with an nvme device? > > -drive id=drv1,if=none,file=nvme.img,aio=io_uring,cache.direct=on,discard=on \ > -device nvme,drive=drv1,serial=blah2 > > Paul was pondering wiring up a no-op kind of thing for null, though. Yep, I started working on that earlier this week, but I've gotten pulled back into the SCTP stuff to try and sort out something odd. Casey, what I have isn't tested, but I'll toss it into my next kernel build to make sure it at least doesn't crash on boot and if it looks good I'll send it to you off-list. -- paul-moore.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands 2022-08-12 15:33 ` Paul Moore @ 2022-08-12 16:03 ` Casey Schaufler 2022-08-13 11:35 ` Kanchan Joshi 2022-08-23 23:46 ` [PATCH] Smack: Provide read control for io_uring_cmd Casey Schaufler 1 sibling, 1 reply; 25+ messages in thread From: Casey Schaufler @ 2022-08-12 16:03 UTC (permalink / raw) To: Paul Moore, Jens Axboe; +Cc: Ankit Kumar, io-uring, joshi.k, casey On 8/12/2022 8:33 AM, Paul Moore wrote: > On Thu, Aug 11, 2022 at 9:51 PM Jens Axboe <[email protected]> wrote: >> On 8/11/22 6:43 PM, Casey Schaufler wrote: >>> On 7/19/2022 6:52 AM, Ankit Kumar wrote: >>>> This patchset adds test/io_uring_passthrough.c to submit uring passthrough >>>> commands to nvme-ns character device. The uring passthrough was introduced >>>> with 5.19 io_uring. >>>> >>>> To send nvme uring passthrough commands we require helpers to fetch NVMe >>>> char device (/dev/ngXnY) specific fields such as namespace id, lba size. >>> There wouldn't be a way to run these tests using a more general >>> configuration, would there? I spent way too much time trying to >>> coax my systems into pretending it has this device. >> It's only plumbed up for nvme. Just use qemu with an nvme device? >> >> -drive id=drv1,if=none,file=nvme.img,aio=io_uring,cache.direct=on,discard=on \ >> -device nvme,drive=drv1,serial=blah2 >> >> Paul was pondering wiring up a no-op kind of thing for null, though. > Yep, I started working on that earlier this week, but I've gotten > pulled back into the SCTP stuff to try and sort out something odd. > > Casey, what I have isn't tested, but I'll toss it into my next kernel > build to make sure it at least doesn't crash on boot and if it looks > good I'll send it to you off-list. Super. Playing with qemu configuration always seems to suck time and rarely gets me where I want to be. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands 2022-08-12 16:03 ` Casey Schaufler @ 2022-08-13 11:35 ` Kanchan Joshi 0 siblings, 0 replies; 25+ messages in thread From: Kanchan Joshi @ 2022-08-13 11:35 UTC (permalink / raw) To: Casey Schaufler; +Cc: Paul Moore, Jens Axboe, Ankit Kumar, io-uring [-- Attachment #1: Type: text/plain, Size: 1776 bytes --] On Fri, Aug 12, 2022 at 09:03:12AM -0700, Casey Schaufler wrote: >On 8/12/2022 8:33 AM, Paul Moore wrote: >> On Thu, Aug 11, 2022 at 9:51 PM Jens Axboe <[email protected]> wrote: >>> On 8/11/22 6:43 PM, Casey Schaufler wrote: >>>> On 7/19/2022 6:52 AM, Ankit Kumar wrote: >>>>> This patchset adds test/io_uring_passthrough.c to submit uring passthrough >>>>> commands to nvme-ns character device. The uring passthrough was introduced >>>>> with 5.19 io_uring. >>>>> >>>>> To send nvme uring passthrough commands we require helpers to fetch NVMe >>>>> char device (/dev/ngXnY) specific fields such as namespace id, lba size. >>>> There wouldn't be a way to run these tests using a more general >>>> configuration, would there? I spent way too much time trying to >>>> coax my systems into pretending it has this device. >>> It's only plumbed up for nvme. Just use qemu with an nvme device? >>> >>> -drive id=drv1,if=none,file=nvme.img,aio=io_uring,cache.direct=on,discard=on \ >>> -device nvme,drive=drv1,serial=blah2 >>> >>> Paul was pondering wiring up a no-op kind of thing for null, though. >> Yep, I started working on that earlier this week, but I've gotten >> pulled back into the SCTP stuff to try and sort out something odd. >> >> Casey, what I have isn't tested, but I'll toss it into my next kernel >> build to make sure it at least doesn't crash on boot and if it looks >> good I'll send it to you off-list. > >Super. Playing with qemu configuration always seems to suck time >and rarely gets me where I want to be. FWIW, one more option (not easier than no-op/null though) is to emulate nvme over a regular-file using loopback-fabrics target. A coarse script is here - https://github.com/joshkan/loopback-nvme-uring/commit/96853963a196f2d307d4d8e60d1276a08b520307 [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-12 15:33 ` Paul Moore 2022-08-12 16:03 ` Casey Schaufler @ 2022-08-23 23:46 ` Casey Schaufler 2022-08-24 0:05 ` Paul Moore 2022-08-27 15:59 ` Kanchan Joshi 1 sibling, 2 replies; 25+ messages in thread From: Casey Schaufler @ 2022-08-23 23:46 UTC (permalink / raw) To: Paul Moore, Jens Axboe; +Cc: Ankit Kumar, io-uring, joshi.k, casey Limit io_uring "cmd" options to files for which the caller has Smack read access. There may be cases where the cmd option may be closer to a write access than a read, but there is no way to make that determination. Signed-off-by: Casey Schaufler <[email protected]> -- security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 001831458fa2..bffccdc494cb 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -42,6 +42,7 @@ #include <linux/fs_context.h> #include <linux/fs_parser.h> #include <linux/watch_queue.h> +#include <linux/io_uring.h> #include "smack.h" #define TRANS_TRUE "TRUE" @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) return -EPERM; } +/** + * smack_uring_cmd - check on file operations for io_uring + * @ioucmd: the command in question + * + * Make a best guess about whether a io_uring "command" should + * be allowed. Use the same logic used for determining if the + * file could be opened for read in the absence of better criteria. + */ +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) +{ + struct file *file = ioucmd->file; + struct smk_audit_info ad; + struct task_smack *tsp; + struct inode *inode; + int rc; + + if (!file) + return -EINVAL; + + tsp = smack_cred(file->f_cred); + inode = file_inode(file); + + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH); + smk_ad_setfield_u_fs_path(&ad, file->f_path); + rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad); + rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc); + + return rc; +} + #endif /* CONFIG_IO_URING */ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = { @@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { #ifdef CONFIG_IO_URING LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds), LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll), + LSM_HOOK_INIT(uring_cmd, smack_uring_cmd), #endif }; ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-23 23:46 ` [PATCH] Smack: Provide read control for io_uring_cmd Casey Schaufler @ 2022-08-24 0:05 ` Paul Moore 2022-08-24 0:07 ` Jens Axboe 2022-08-27 15:59 ` Kanchan Joshi 1 sibling, 1 reply; 25+ messages in thread From: Paul Moore @ 2022-08-24 0:05 UTC (permalink / raw) To: Casey Schaufler; +Cc: Jens Axboe, Ankit Kumar, io-uring, joshi.k On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <[email protected]> wrote: > > Limit io_uring "cmd" options to files for which the caller has > Smack read access. There may be cases where the cmd option may > be closer to a write access than a read, but there is no way > to make that determination. > > Signed-off-by: Casey Schaufler <[email protected]> > -- > security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index 001831458fa2..bffccdc494cb 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c ... > @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) > return -EPERM; > } > > +/** > + * smack_uring_cmd - check on file operations for io_uring > + * @ioucmd: the command in question > + * > + * Make a best guess about whether a io_uring "command" should > + * be allowed. Use the same logic used for determining if the > + * file could be opened for read in the absence of better criteria. > + */ > +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) > +{ > + struct file *file = ioucmd->file; > + struct smk_audit_info ad; > + struct task_smack *tsp; > + struct inode *inode; > + int rc; > + > + if (!file) > + return -EINVAL; Perhaps this is a better question for Jens, but ioucmd->file is always going to be valid when the LSM hook is called, yes? > + tsp = smack_cred(file->f_cred); > + inode = file_inode(file); > + > + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH); > + smk_ad_setfield_u_fs_path(&ad, file->f_path); > + rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad); > + rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc); > + > + return rc; > +} > + > #endif /* CONFIG_IO_URING */ -- paul-moore.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-24 0:05 ` Paul Moore @ 2022-08-24 0:07 ` Jens Axboe 2022-08-26 15:15 ` Paul Moore 0 siblings, 1 reply; 25+ messages in thread From: Jens Axboe @ 2022-08-24 0:07 UTC (permalink / raw) To: Paul Moore, Casey Schaufler; +Cc: Ankit Kumar, io-uring, joshi.k On 8/23/22 6:05 PM, Paul Moore wrote: > On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <[email protected]> wrote: >> >> Limit io_uring "cmd" options to files for which the caller has >> Smack read access. There may be cases where the cmd option may >> be closer to a write access than a read, but there is no way >> to make that determination. >> >> Signed-off-by: Casey Schaufler <[email protected]> >> -- >> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >> index 001831458fa2..bffccdc494cb 100644 >> --- a/security/smack/smack_lsm.c >> +++ b/security/smack/smack_lsm.c > > ... > >> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) >> return -EPERM; >> } >> >> +/** >> + * smack_uring_cmd - check on file operations for io_uring >> + * @ioucmd: the command in question >> + * >> + * Make a best guess about whether a io_uring "command" should >> + * be allowed. Use the same logic used for determining if the >> + * file could be opened for read in the absence of better criteria. >> + */ >> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) >> +{ >> + struct file *file = ioucmd->file; >> + struct smk_audit_info ad; >> + struct task_smack *tsp; >> + struct inode *inode; >> + int rc; >> + >> + if (!file) >> + return -EINVAL; > > Perhaps this is a better question for Jens, but ioucmd->file is always > going to be valid when the LSM hook is called, yes? file will always be valid for uring commands, as they are marked as requiring a file. If no valid fd is given for it, it would've been errored early on, before reaching f_op->uring_cmd(). -- Jens Axboe ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-24 0:07 ` Jens Axboe @ 2022-08-26 15:15 ` Paul Moore 2022-08-26 16:53 ` Casey Schaufler 0 siblings, 1 reply; 25+ messages in thread From: Paul Moore @ 2022-08-26 15:15 UTC (permalink / raw) To: Jens Axboe; +Cc: Casey Schaufler, Ankit Kumar, io-uring, joshi.k On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <[email protected]> wrote: > On 8/23/22 6:05 PM, Paul Moore wrote: > > On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <[email protected]> wrote: > >> > >> Limit io_uring "cmd" options to files for which the caller has > >> Smack read access. There may be cases where the cmd option may > >> be closer to a write access than a read, but there is no way > >> to make that determination. > >> > >> Signed-off-by: Casey Schaufler <[email protected]> > >> -- > >> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ > >> 1 file changed, 32 insertions(+) > >> > >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > >> index 001831458fa2..bffccdc494cb 100644 > >> --- a/security/smack/smack_lsm.c > >> +++ b/security/smack/smack_lsm.c > > > > ... > > > >> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) > >> return -EPERM; > >> } > >> > >> +/** > >> + * smack_uring_cmd - check on file operations for io_uring > >> + * @ioucmd: the command in question > >> + * > >> + * Make a best guess about whether a io_uring "command" should > >> + * be allowed. Use the same logic used for determining if the > >> + * file could be opened for read in the absence of better criteria. > >> + */ > >> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) > >> +{ > >> + struct file *file = ioucmd->file; > >> + struct smk_audit_info ad; > >> + struct task_smack *tsp; > >> + struct inode *inode; > >> + int rc; > >> + > >> + if (!file) > >> + return -EINVAL; > > > > Perhaps this is a better question for Jens, but ioucmd->file is always > > going to be valid when the LSM hook is called, yes? > > file will always be valid for uring commands, as they are marked as > requiring a file. If no valid fd is given for it, it would've been > errored early on, before reaching f_op->uring_cmd(). Hey Casey, where do things stand with this patch? To be specific, did you want me to include this in the lsm/stable-6.0 PR for Linus or are you planning to send it separately? If you want me to send it up, are you planning another revision? There is no right or wrong answer here as far as I'm concerned, I'm just trying to make sure we are all on the same page. -- paul-moore.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-26 15:15 ` Paul Moore @ 2022-08-26 16:53 ` Casey Schaufler 2022-08-26 18:59 ` Paul Moore 0 siblings, 1 reply; 25+ messages in thread From: Casey Schaufler @ 2022-08-26 16:53 UTC (permalink / raw) To: Paul Moore, Jens Axboe; +Cc: Ankit Kumar, io-uring, joshi.k, casey On 8/26/2022 8:15 AM, Paul Moore wrote: > On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <[email protected]> wrote: >> On 8/23/22 6:05 PM, Paul Moore wrote: >>> On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <[email protected]> wrote: >>>> Limit io_uring "cmd" options to files for which the caller has >>>> Smack read access. There may be cases where the cmd option may >>>> be closer to a write access than a read, but there is no way >>>> to make that determination. >>>> >>>> Signed-off-by: Casey Schaufler <[email protected]> >>>> -- >>>> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ >>>> 1 file changed, 32 insertions(+) >>>> >>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >>>> index 001831458fa2..bffccdc494cb 100644 >>>> --- a/security/smack/smack_lsm.c >>>> +++ b/security/smack/smack_lsm.c >>> ... >>> >>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) >>>> return -EPERM; >>>> } >>>> >>>> +/** >>>> + * smack_uring_cmd - check on file operations for io_uring >>>> + * @ioucmd: the command in question >>>> + * >>>> + * Make a best guess about whether a io_uring "command" should >>>> + * be allowed. Use the same logic used for determining if the >>>> + * file could be opened for read in the absence of better criteria. >>>> + */ >>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) >>>> +{ >>>> + struct file *file = ioucmd->file; >>>> + struct smk_audit_info ad; >>>> + struct task_smack *tsp; >>>> + struct inode *inode; >>>> + int rc; >>>> + >>>> + if (!file) >>>> + return -EINVAL; >>> Perhaps this is a better question for Jens, but ioucmd->file is always >>> going to be valid when the LSM hook is called, yes? >> file will always be valid for uring commands, as they are marked as >> requiring a file. If no valid fd is given for it, it would've been >> errored early on, before reaching f_op->uring_cmd(). > Hey Casey, where do things stand with this patch? To be specific, did > you want me to include this in the lsm/stable-6.0 PR for Linus or are > you planning to send it separately? If you want me to send it up, are > you planning another revision? > > There is no right or wrong answer here as far as I'm concerned, I'm > just trying to make sure we are all on the same page. I think the whole LSM fix for io_uring looks better the more complete it is. I don't see the Smack check changing until such time as there's better information available to make decisions upon. If you send it along with the rest of the patch set I think we'll have done our best. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-26 16:53 ` Casey Schaufler @ 2022-08-26 18:59 ` Paul Moore 2022-08-26 19:04 ` Casey Schaufler 0 siblings, 1 reply; 25+ messages in thread From: Paul Moore @ 2022-08-26 18:59 UTC (permalink / raw) To: Casey Schaufler; +Cc: Jens Axboe, Ankit Kumar, io-uring, joshi.k On Fri, Aug 26, 2022 at 12:53 PM Casey Schaufler <[email protected]> wrote: > On 8/26/2022 8:15 AM, Paul Moore wrote: > > On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <[email protected]> wrote: > >> On 8/23/22 6:05 PM, Paul Moore wrote: > >>> On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <[email protected]> wrote: > >>>> Limit io_uring "cmd" options to files for which the caller has > >>>> Smack read access. There may be cases where the cmd option may > >>>> be closer to a write access than a read, but there is no way > >>>> to make that determination. > >>>> > >>>> Signed-off-by: Casey Schaufler <[email protected]> > >>>> -- > >>>> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 32 insertions(+) > >>>> > >>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > >>>> index 001831458fa2..bffccdc494cb 100644 > >>>> --- a/security/smack/smack_lsm.c > >>>> +++ b/security/smack/smack_lsm.c > >>> ... > >>> > >>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) > >>>> return -EPERM; > >>>> } > >>>> > >>>> +/** > >>>> + * smack_uring_cmd - check on file operations for io_uring > >>>> + * @ioucmd: the command in question > >>>> + * > >>>> + * Make a best guess about whether a io_uring "command" should > >>>> + * be allowed. Use the same logic used for determining if the > >>>> + * file could be opened for read in the absence of better criteria. > >>>> + */ > >>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) > >>>> +{ > >>>> + struct file *file = ioucmd->file; > >>>> + struct smk_audit_info ad; > >>>> + struct task_smack *tsp; > >>>> + struct inode *inode; > >>>> + int rc; > >>>> + > >>>> + if (!file) > >>>> + return -EINVAL; > >>> Perhaps this is a better question for Jens, but ioucmd->file is always > >>> going to be valid when the LSM hook is called, yes? > >> file will always be valid for uring commands, as they are marked as > >> requiring a file. If no valid fd is given for it, it would've been > >> errored early on, before reaching f_op->uring_cmd(). > > Hey Casey, where do things stand with this patch? To be specific, did > > you want me to include this in the lsm/stable-6.0 PR for Linus or are > > you planning to send it separately? If you want me to send it up, are > > you planning another revision? > > > > There is no right or wrong answer here as far as I'm concerned, I'm > > just trying to make sure we are all on the same page. > > I think the whole LSM fix for io_uring looks better the more complete > it is. I don't see the Smack check changing until such time as there's > better information available to make decisions upon. If you send it along > with the rest of the patch set I think we'll have done our best. Okay, will do. Would you like me to tag the patch with the 'Fixes:' and stable tags, similar to the LSM and SELinux patches? -- paul-moore.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-26 18:59 ` Paul Moore @ 2022-08-26 19:04 ` Casey Schaufler 2022-08-26 19:10 ` Paul Moore 0 siblings, 1 reply; 25+ messages in thread From: Casey Schaufler @ 2022-08-26 19:04 UTC (permalink / raw) To: Paul Moore; +Cc: Jens Axboe, Ankit Kumar, io-uring, joshi.k, casey On 8/26/2022 11:59 AM, Paul Moore wrote: > On Fri, Aug 26, 2022 at 12:53 PM Casey Schaufler <[email protected]> wrote: >> On 8/26/2022 8:15 AM, Paul Moore wrote: >>> On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <[email protected]> wrote: >>>> On 8/23/22 6:05 PM, Paul Moore wrote: >>>>> On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <[email protected]> wrote: >>>>>> Limit io_uring "cmd" options to files for which the caller has >>>>>> Smack read access. There may be cases where the cmd option may >>>>>> be closer to a write access than a read, but there is no way >>>>>> to make that determination. >>>>>> >>>>>> Signed-off-by: Casey Schaufler <[email protected]> >>>>>> -- >>>>>> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 32 insertions(+) >>>>>> >>>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >>>>>> index 001831458fa2..bffccdc494cb 100644 >>>>>> --- a/security/smack/smack_lsm.c >>>>>> +++ b/security/smack/smack_lsm.c >>>>> ... >>>>> >>>>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) >>>>>> return -EPERM; >>>>>> } >>>>>> >>>>>> +/** >>>>>> + * smack_uring_cmd - check on file operations for io_uring >>>>>> + * @ioucmd: the command in question >>>>>> + * >>>>>> + * Make a best guess about whether a io_uring "command" should >>>>>> + * be allowed. Use the same logic used for determining if the >>>>>> + * file could be opened for read in the absence of better criteria. >>>>>> + */ >>>>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) >>>>>> +{ >>>>>> + struct file *file = ioucmd->file; >>>>>> + struct smk_audit_info ad; >>>>>> + struct task_smack *tsp; >>>>>> + struct inode *inode; >>>>>> + int rc; >>>>>> + >>>>>> + if (!file) >>>>>> + return -EINVAL; >>>>> Perhaps this is a better question for Jens, but ioucmd->file is always >>>>> going to be valid when the LSM hook is called, yes? >>>> file will always be valid for uring commands, as they are marked as >>>> requiring a file. If no valid fd is given for it, it would've been >>>> errored early on, before reaching f_op->uring_cmd(). >>> Hey Casey, where do things stand with this patch? To be specific, did >>> you want me to include this in the lsm/stable-6.0 PR for Linus or are >>> you planning to send it separately? If you want me to send it up, are >>> you planning another revision? >>> >>> There is no right or wrong answer here as far as I'm concerned, I'm >>> just trying to make sure we are all on the same page. >> I think the whole LSM fix for io_uring looks better the more complete >> it is. I don't see the Smack check changing until such time as there's >> better information available to make decisions upon. If you send it along >> with the rest of the patch set I think we'll have done our best. > Okay, will do. Would you like me to tag the patch with the 'Fixes:' > and stable tags, similar to the LSM and SELinux patches? Yes, I think that's best. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-26 19:04 ` Casey Schaufler @ 2022-08-26 19:10 ` Paul Moore 2022-08-26 19:31 ` Casey Schaufler 0 siblings, 1 reply; 25+ messages in thread From: Paul Moore @ 2022-08-26 19:10 UTC (permalink / raw) To: Casey Schaufler; +Cc: Jens Axboe, Ankit Kumar, io-uring, joshi.k On Fri, Aug 26, 2022 at 3:04 PM Casey Schaufler <[email protected]> wrote: > On 8/26/2022 11:59 AM, Paul Moore wrote: > > On Fri, Aug 26, 2022 at 12:53 PM Casey Schaufler <[email protected]> wrote: > >> On 8/26/2022 8:15 AM, Paul Moore wrote: > >>> On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <[email protected]> wrote: > >>>> On 8/23/22 6:05 PM, Paul Moore wrote: > >>>>> On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <[email protected]> wrote: > >>>>>> Limit io_uring "cmd" options to files for which the caller has > >>>>>> Smack read access. There may be cases where the cmd option may > >>>>>> be closer to a write access than a read, but there is no way > >>>>>> to make that determination. > >>>>>> > >>>>>> Signed-off-by: Casey Schaufler <[email protected]> > >>>>>> -- > >>>>>> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ > >>>>>> 1 file changed, 32 insertions(+) > >>>>>> > >>>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > >>>>>> index 001831458fa2..bffccdc494cb 100644 > >>>>>> --- a/security/smack/smack_lsm.c > >>>>>> +++ b/security/smack/smack_lsm.c > >>>>> ... > >>>>> > >>>>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) > >>>>>> return -EPERM; > >>>>>> } > >>>>>> > >>>>>> +/** > >>>>>> + * smack_uring_cmd - check on file operations for io_uring > >>>>>> + * @ioucmd: the command in question > >>>>>> + * > >>>>>> + * Make a best guess about whether a io_uring "command" should > >>>>>> + * be allowed. Use the same logic used for determining if the > >>>>>> + * file could be opened for read in the absence of better criteria. > >>>>>> + */ > >>>>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) > >>>>>> +{ > >>>>>> + struct file *file = ioucmd->file; > >>>>>> + struct smk_audit_info ad; > >>>>>> + struct task_smack *tsp; > >>>>>> + struct inode *inode; > >>>>>> + int rc; > >>>>>> + > >>>>>> + if (!file) > >>>>>> + return -EINVAL; > >>>>> Perhaps this is a better question for Jens, but ioucmd->file is always > >>>>> going to be valid when the LSM hook is called, yes? > >>>> file will always be valid for uring commands, as they are marked as > >>>> requiring a file. If no valid fd is given for it, it would've been > >>>> errored early on, before reaching f_op->uring_cmd(). > >>> Hey Casey, where do things stand with this patch? To be specific, did > >>> you want me to include this in the lsm/stable-6.0 PR for Linus or are > >>> you planning to send it separately? If you want me to send it up, are > >>> you planning another revision? > >>> > >>> There is no right or wrong answer here as far as I'm concerned, I'm > >>> just trying to make sure we are all on the same page. > >> I think the whole LSM fix for io_uring looks better the more complete > >> it is. I don't see the Smack check changing until such time as there's > >> better information available to make decisions upon. If you send it along > >> with the rest of the patch set I think we'll have done our best. > > Okay, will do. Would you like me to tag the patch with the 'Fixes:' > > and stable tags, similar to the LSM and SELinux patches? > > Yes, I think that's best. Done and merged to lsm/stable-6.0. I'm going to let the automated stuff do it's thing and assuming no problems I'll plan to send it to Linus on Monday ... sending stuff like this last thing on a Friday is a little too risky for my tastes. -- paul-moore.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-26 19:10 ` Paul Moore @ 2022-08-26 19:31 ` Casey Schaufler 0 siblings, 0 replies; 25+ messages in thread From: Casey Schaufler @ 2022-08-26 19:31 UTC (permalink / raw) To: Paul Moore; +Cc: Jens Axboe, Ankit Kumar, io-uring, joshi.k, casey On 8/26/2022 12:10 PM, Paul Moore wrote: > On Fri, Aug 26, 2022 at 3:04 PM Casey Schaufler <[email protected]> wrote: >> On 8/26/2022 11:59 AM, Paul Moore wrote: >>> On Fri, Aug 26, 2022 at 12:53 PM Casey Schaufler <[email protected]> wrote: >>>> On 8/26/2022 8:15 AM, Paul Moore wrote: >>>>> On Tue, Aug 23, 2022 at 8:07 PM Jens Axboe <[email protected]> wrote: >>>>>> On 8/23/22 6:05 PM, Paul Moore wrote: >>>>>>> On Tue, Aug 23, 2022 at 7:46 PM Casey Schaufler <[email protected]> wrote: >>>>>>>> Limit io_uring "cmd" options to files for which the caller has >>>>>>>> Smack read access. There may be cases where the cmd option may >>>>>>>> be closer to a write access than a read, but there is no way >>>>>>>> to make that determination. >>>>>>>> >>>>>>>> Signed-off-by: Casey Schaufler <[email protected]> >>>>>>>> -- >>>>>>>> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ >>>>>>>> 1 file changed, 32 insertions(+) >>>>>>>> >>>>>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >>>>>>>> index 001831458fa2..bffccdc494cb 100644 >>>>>>>> --- a/security/smack/smack_lsm.c >>>>>>>> +++ b/security/smack/smack_lsm.c >>>>>>> ... >>>>>>> >>>>>>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) >>>>>>>> return -EPERM; >>>>>>>> } >>>>>>>> >>>>>>>> +/** >>>>>>>> + * smack_uring_cmd - check on file operations for io_uring >>>>>>>> + * @ioucmd: the command in question >>>>>>>> + * >>>>>>>> + * Make a best guess about whether a io_uring "command" should >>>>>>>> + * be allowed. Use the same logic used for determining if the >>>>>>>> + * file could be opened for read in the absence of better criteria. >>>>>>>> + */ >>>>>>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) >>>>>>>> +{ >>>>>>>> + struct file *file = ioucmd->file; >>>>>>>> + struct smk_audit_info ad; >>>>>>>> + struct task_smack *tsp; >>>>>>>> + struct inode *inode; >>>>>>>> + int rc; >>>>>>>> + >>>>>>>> + if (!file) >>>>>>>> + return -EINVAL; >>>>>>> Perhaps this is a better question for Jens, but ioucmd->file is always >>>>>>> going to be valid when the LSM hook is called, yes? >>>>>> file will always be valid for uring commands, as they are marked as >>>>>> requiring a file. If no valid fd is given for it, it would've been >>>>>> errored early on, before reaching f_op->uring_cmd(). >>>>> Hey Casey, where do things stand with this patch? To be specific, did >>>>> you want me to include this in the lsm/stable-6.0 PR for Linus or are >>>>> you planning to send it separately? If you want me to send it up, are >>>>> you planning another revision? >>>>> >>>>> There is no right or wrong answer here as far as I'm concerned, I'm >>>>> just trying to make sure we are all on the same page. >>>> I think the whole LSM fix for io_uring looks better the more complete >>>> it is. I don't see the Smack check changing until such time as there's >>>> better information available to make decisions upon. If you send it along >>>> with the rest of the patch set I think we'll have done our best. >>> Okay, will do. Would you like me to tag the patch with the 'Fixes:' >>> and stable tags, similar to the LSM and SELinux patches? >> Yes, I think that's best. > Done and merged to lsm/stable-6.0. I'm going to let the automated > stuff do it's thing and assuming no problems I'll plan to send it to > Linus on Monday ... sending stuff like this last thing on a Friday is > a little too risky for my tastes. Agreed. Thank you. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-23 23:46 ` [PATCH] Smack: Provide read control for io_uring_cmd Casey Schaufler 2022-08-24 0:05 ` Paul Moore @ 2022-08-27 15:59 ` Kanchan Joshi 2022-08-29 16:20 ` Casey Schaufler 1 sibling, 1 reply; 25+ messages in thread From: Kanchan Joshi @ 2022-08-27 15:59 UTC (permalink / raw) To: Casey Schaufler; +Cc: Paul Moore, Jens Axboe, Ankit Kumar, io-uring [-- Attachment #1: Type: text/plain, Size: 2795 bytes --] On Tue, Aug 23, 2022 at 04:46:18PM -0700, Casey Schaufler wrote: >Limit io_uring "cmd" options to files for which the caller has >Smack read access. There may be cases where the cmd option may >be closer to a write access than a read, but there is no way >to make that determination. > >Signed-off-by: Casey Schaufler <[email protected]> >-- > security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > >diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >index 001831458fa2..bffccdc494cb 100644 >--- a/security/smack/smack_lsm.c >+++ b/security/smack/smack_lsm.c >@@ -42,6 +42,7 @@ > #include <linux/fs_context.h> > #include <linux/fs_parser.h> > #include <linux/watch_queue.h> >+#include <linux/io_uring.h> > #include "smack.h" > > #define TRANS_TRUE "TRUE" >@@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) > return -EPERM; > } > >+/** >+ * smack_uring_cmd - check on file operations for io_uring >+ * @ioucmd: the command in question >+ * >+ * Make a best guess about whether a io_uring "command" should >+ * be allowed. Use the same logic used for determining if the >+ * file could be opened for read in the absence of better criteria. >+ */ >+static int smack_uring_cmd(struct io_uring_cmd *ioucmd) >+{ >+ struct file *file = ioucmd->file; >+ struct smk_audit_info ad; >+ struct task_smack *tsp; >+ struct inode *inode; >+ int rc; >+ >+ if (!file) >+ return -EINVAL; >+ >+ tsp = smack_cred(file->f_cred); >+ inode = file_inode(file); >+ >+ smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH); >+ smk_ad_setfield_u_fs_path(&ad, file->f_path); >+ rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad); >+ rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc); >+ >+ return rc; >+} >+ > #endif /* CONFIG_IO_URING */ > > struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = { >@@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { > #ifdef CONFIG_IO_URING > LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds), > LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll), >+ LSM_HOOK_INIT(uring_cmd, smack_uring_cmd), > #endif Tried this on nvme device (/dev/ng0n1). Took a while to come out of noob setup-related issues but I see that smack is listed (in /sys/kernel/security/lsm), smackfs is present, and the hook (smack_uring_cmd) gets triggered fine on doing I/O on /dev/ng0n1. I/O goes fine, which seems aligned with the label on /dev/ng0n1 (which is set to floor). $ chsmack -L /dev/ng0n1 /dev/ng0n1 access="_" I ran fio (/usr/bin/fio), which also has the same label. Hope you expect the same outcome. Do you run something else to see that things are fine e.g. for /dev/null, which also has the same label "_". If yes, I can try the same on nvme side. [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-27 15:59 ` Kanchan Joshi @ 2022-08-29 16:20 ` Casey Schaufler 2022-08-30 13:08 ` Joel Granados 0 siblings, 1 reply; 25+ messages in thread From: Casey Schaufler @ 2022-08-29 16:20 UTC (permalink / raw) To: Kanchan Joshi; +Cc: Paul Moore, Jens Axboe, Ankit Kumar, io-uring, casey On 8/27/2022 8:59 AM, Kanchan Joshi wrote: > On Tue, Aug 23, 2022 at 04:46:18PM -0700, Casey Schaufler wrote: >> Limit io_uring "cmd" options to files for which the caller has >> Smack read access. There may be cases where the cmd option may >> be closer to a write access than a read, but there is no way >> to make that determination. >> >> Signed-off-by: Casey Schaufler <[email protected]> >> -- >> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >> index 001831458fa2..bffccdc494cb 100644 >> --- a/security/smack/smack_lsm.c >> +++ b/security/smack/smack_lsm.c >> @@ -42,6 +42,7 @@ >> #include <linux/fs_context.h> >> #include <linux/fs_parser.h> >> #include <linux/watch_queue.h> >> +#include <linux/io_uring.h> >> #include "smack.h" >> >> #define TRANS_TRUE "TRUE" >> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) >> return -EPERM; >> } >> >> +/** >> + * smack_uring_cmd - check on file operations for io_uring >> + * @ioucmd: the command in question >> + * >> + * Make a best guess about whether a io_uring "command" should >> + * be allowed. Use the same logic used for determining if the >> + * file could be opened for read in the absence of better criteria. >> + */ >> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) >> +{ >> + struct file *file = ioucmd->file; >> + struct smk_audit_info ad; >> + struct task_smack *tsp; >> + struct inode *inode; >> + int rc; >> + >> + if (!file) >> + return -EINVAL; >> + >> + tsp = smack_cred(file->f_cred); >> + inode = file_inode(file); >> + >> + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH); >> + smk_ad_setfield_u_fs_path(&ad, file->f_path); >> + rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad); >> + rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc); >> + >> + return rc; >> +} >> + >> #endif /* CONFIG_IO_URING */ >> >> struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = { >> @@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[] >> __lsm_ro_after_init = { >> #ifdef CONFIG_IO_URING >> LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds), >> LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll), >> + LSM_HOOK_INIT(uring_cmd, smack_uring_cmd), >> #endif > > Tried this on nvme device (/dev/ng0n1). > Took a while to come out of noob setup-related issues but I see that > smack is listed (in /sys/kernel/security/lsm), smackfs is present, and > the hook (smack_uring_cmd) gets triggered fine on doing I/O on > /dev/ng0n1. > > I/O goes fine, which seems aligned with the label on /dev/ng0n1 (which > is set to floor). > > $ chsmack -L /dev/ng0n1 > /dev/ng0n1 access="_" Setting the Smack on the object that the cmd operates on to something other than "_" would be the correct test. If that is /dev/ng0n1 you could use # chsmack -a Snap /dev/ng0n1 The unprivileged user won't be able to read /dev/ng0n1 so you won't get as far as testing the cmd interface. I don't know io_uring and nvme well enough to know what other objects may be involved. Noob here, too. > > I ran fio (/usr/bin/fio), which also has the same label. > Hope you expect the same outcome. > > Do you run something else to see that things are fine e.g. for > /dev/null, which also has the same label "_". > If yes, I can try the same on nvme side. > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-29 16:20 ` Casey Schaufler @ 2022-08-30 13:08 ` Joel Granados 2022-08-30 14:16 ` Casey Schaufler 0 siblings, 1 reply; 25+ messages in thread From: Joel Granados @ 2022-08-30 13:08 UTC (permalink / raw) To: Casey Schaufler Cc: Kanchan Joshi, Paul Moore, Jens Axboe, Ankit Kumar, io-uring [-- Attachment #1: Type: text/plain, Size: 5284 bytes --] Hey Casey I have tested this patch and I see the smack_uring_cmd prevents access to file when smack labels are different. They way I got there was a bit convoluted and I'll describe it here so ppl can judge how valid the test is. Tested-by: Joel Granados <[email protected]> I started by reproducing what Kanchan had done by changing the smack label from "_" to "Snap". Then I ran the io_uring passthrough test included in liburing with an unprivileged user and saw that the smack_uring_cmd function was NOT executed because smack prevented an open on the device file ("/dev/ng0n1" on my box). So here I got a bit sneaky and changed the label after the open to the device was done. This is how I did it: 1. In the io_uring_passthrough.c tests I stopped execution after the device was open and before the actual IO. 2. While on hold I changed the label of the device from "_" to "Snap". At this point the device had a "Snap" label and the executable had a "_" label. 3. Previous to execution I had placed a printk inside the smack_uring_cmd function. And I also made sure to printk whenever security_uring_command returned because of a security violation. 4. I continued execution and saw that both smack_uiring_cmd and io_uring_cmd returned error. Which is what I expected. I'm still a bit unsure of my setup so if anyone has comments or a way to make it better, I would really appreciate feedback. Best Joel On Mon, Aug 29, 2022 at 09:20:09AM -0700, Casey Schaufler wrote: > On 8/27/2022 8:59 AM, Kanchan Joshi wrote: > > On Tue, Aug 23, 2022 at 04:46:18PM -0700, Casey Schaufler wrote: > >> Limit io_uring "cmd" options to files for which the caller has > >> Smack read access. There may be cases where the cmd option may > >> be closer to a write access than a read, but there is no way > >> to make that determination. > >> > >> Signed-off-by: Casey Schaufler <[email protected]> > >> -- > >> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ > >> 1 file changed, 32 insertions(+) > >> > >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > >> index 001831458fa2..bffccdc494cb 100644 > >> --- a/security/smack/smack_lsm.c > >> +++ b/security/smack/smack_lsm.c > >> @@ -42,6 +42,7 @@ > >> #include <linux/fs_context.h> > >> #include <linux/fs_parser.h> > >> #include <linux/watch_queue.h> > >> +#include <linux/io_uring.h> > >> #include "smack.h" > >> > >> #define TRANS_TRUE "TRUE" > >> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) > >> return -EPERM; > >> } > >> > >> +/** > >> + * smack_uring_cmd - check on file operations for io_uring > >> + * @ioucmd: the command in question > >> + * > >> + * Make a best guess about whether a io_uring "command" should > >> + * be allowed. Use the same logic used for determining if the > >> + * file could be opened for read in the absence of better criteria. > >> + */ > >> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) > >> +{ > >> + struct file *file = ioucmd->file; > >> + struct smk_audit_info ad; > >> + struct task_smack *tsp; > >> + struct inode *inode; > >> + int rc; > >> + > >> + if (!file) > >> + return -EINVAL; > >> + > >> + tsp = smack_cred(file->f_cred); > >> + inode = file_inode(file); > >> + > >> + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH); > >> + smk_ad_setfield_u_fs_path(&ad, file->f_path); > >> + rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad); > >> + rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc); > >> + > >> + return rc; > >> +} > >> + > >> #endif /* CONFIG_IO_URING */ > >> > >> struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = { > >> @@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[] > >> __lsm_ro_after_init = { > >> #ifdef CONFIG_IO_URING > >> LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds), > >> LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll), > >> + LSM_HOOK_INIT(uring_cmd, smack_uring_cmd), > >> #endif > > > > Tried this on nvme device (/dev/ng0n1). > > Took a while to come out of noob setup-related issues but I see that > > smack is listed (in /sys/kernel/security/lsm), smackfs is present, and > > the hook (smack_uring_cmd) gets triggered fine on doing I/O on > > /dev/ng0n1. > > > > I/O goes fine, which seems aligned with the label on /dev/ng0n1 (which > > is set to floor). > > > > $ chsmack -L /dev/ng0n1 > > /dev/ng0n1 access="_" > > Setting the Smack on the object that the cmd operates on to > something other than "_" would be the correct test. If that > is /dev/ng0n1 you could use > > # chsmack -a Snap /dev/ng0n1 > > The unprivileged user won't be able to read /dev/ng0n1 so you > won't get as far as testing the cmd interface. I don't know > io_uring and nvme well enough to know what other objects may > be involved. Noob here, too. > > > > > I ran fio (/usr/bin/fio), which also has the same label. > > Hope you expect the same outcome. > > > > Do you run something else to see that things are fine e.g. for > > /dev/null, which also has the same label "_". > > If yes, I can try the same on nvme side. > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-30 13:08 ` Joel Granados @ 2022-08-30 14:16 ` Casey Schaufler 2022-08-31 7:15 ` Joel Granados 0 siblings, 1 reply; 25+ messages in thread From: Casey Schaufler @ 2022-08-30 14:16 UTC (permalink / raw) To: Joel Granados Cc: Kanchan Joshi, Paul Moore, Jens Axboe, Ankit Kumar, io-uring, casey On 8/30/2022 6:08 AM, Joel Granados wrote: > Hey Casey > > I have tested this patch and I see the smack_uring_cmd prevents access > to file when smack labels are different. They way I got there was a bit > convoluted and I'll describe it here so ppl can judge how valid the test > is. > > Tested-by: Joel Granados <[email protected]> Thank you. > > I started by reproducing what Kanchan had done by changing the smack > label from "_" to "Snap". Then I ran the io_uring passthrough test > included in liburing with an unprivileged user and saw that the > smack_uring_cmd function was NOT executed because smack prevented an open on > the device file ("/dev/ng0n1" on my box). > > So here I got a bit sneaky and changed the label after the open to the > device was done. This is how I did it: > 1. In the io_uring_passthrough.c tests I stopped execution after the > device was open and before the actual IO. > 2. While on hold I changed the label of the device from "_" to "Snap". > At this point the device had a "Snap" label and the executable had a > "_" label. > 3. Previous to execution I had placed a printk inside the > smack_uring_cmd function. And I also made sure to printk whenever > security_uring_command returned because of a security violation. > 4. I continued execution and saw that both smack_uiring_cmd and > io_uring_cmd returned error. Which is what I expected. > > I'm still a bit unsure of my setup so if anyone has comments or a way to > make it better, I would really appreciate feedback. This is a perfectly rational approach to the test. Another approach would be to add a Smack access rule: echo "_ Snap r" > /sys/fs/smackfs/load2 and label the device before the test begins. Step 2 changes from labeling the device to removing the access rule: echo "_ Snap -" > /sys/fs/smackfs/load2 and you will get the same result. I wouldn't change your test, but I would probably add another that does it using the rule change. > Best > > Joel > > On Mon, Aug 29, 2022 at 09:20:09AM -0700, Casey Schaufler wrote: >> On 8/27/2022 8:59 AM, Kanchan Joshi wrote: >>> On Tue, Aug 23, 2022 at 04:46:18PM -0700, Casey Schaufler wrote: >>>> Limit io_uring "cmd" options to files for which the caller has >>>> Smack read access. There may be cases where the cmd option may >>>> be closer to a write access than a read, but there is no way >>>> to make that determination. >>>> >>>> Signed-off-by: Casey Schaufler <[email protected]> >>>> -- >>>> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ >>>> 1 file changed, 32 insertions(+) >>>> >>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >>>> index 001831458fa2..bffccdc494cb 100644 >>>> --- a/security/smack/smack_lsm.c >>>> +++ b/security/smack/smack_lsm.c >>>> @@ -42,6 +42,7 @@ >>>> #include <linux/fs_context.h> >>>> #include <linux/fs_parser.h> >>>> #include <linux/watch_queue.h> >>>> +#include <linux/io_uring.h> >>>> #include "smack.h" >>>> >>>> #define TRANS_TRUE "TRUE" >>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) >>>> return -EPERM; >>>> } >>>> >>>> +/** >>>> + * smack_uring_cmd - check on file operations for io_uring >>>> + * @ioucmd: the command in question >>>> + * >>>> + * Make a best guess about whether a io_uring "command" should >>>> + * be allowed. Use the same logic used for determining if the >>>> + * file could be opened for read in the absence of better criteria. >>>> + */ >>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) >>>> +{ >>>> + struct file *file = ioucmd->file; >>>> + struct smk_audit_info ad; >>>> + struct task_smack *tsp; >>>> + struct inode *inode; >>>> + int rc; >>>> + >>>> + if (!file) >>>> + return -EINVAL; >>>> + >>>> + tsp = smack_cred(file->f_cred); >>>> + inode = file_inode(file); >>>> + >>>> + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH); >>>> + smk_ad_setfield_u_fs_path(&ad, file->f_path); >>>> + rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad); >>>> + rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc); >>>> + >>>> + return rc; >>>> +} >>>> + >>>> #endif /* CONFIG_IO_URING */ >>>> >>>> struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = { >>>> @@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[] >>>> __lsm_ro_after_init = { >>>> #ifdef CONFIG_IO_URING >>>> LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds), >>>> LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll), >>>> + LSM_HOOK_INIT(uring_cmd, smack_uring_cmd), >>>> #endif >>> Tried this on nvme device (/dev/ng0n1). >>> Took a while to come out of noob setup-related issues but I see that >>> smack is listed (in /sys/kernel/security/lsm), smackfs is present, and >>> the hook (smack_uring_cmd) gets triggered fine on doing I/O on >>> /dev/ng0n1. >>> >>> I/O goes fine, which seems aligned with the label on /dev/ng0n1 (which >>> is set to floor). >>> >>> $ chsmack -L /dev/ng0n1 >>> /dev/ng0n1 access="_" >> Setting the Smack on the object that the cmd operates on to >> something other than "_" would be the correct test. If that >> is /dev/ng0n1 you could use >> >> # chsmack -a Snap /dev/ng0n1 >> >> The unprivileged user won't be able to read /dev/ng0n1 so you >> won't get as far as testing the cmd interface. I don't know >> io_uring and nvme well enough to know what other objects may >> be involved. Noob here, too. >> >>> I ran fio (/usr/bin/fio), which also has the same label. >>> Hope you expect the same outcome. >>> >>> Do you run something else to see that things are fine e.g. for >>> /dev/null, which also has the same label "_". >>> If yes, I can try the same on nvme side. >>> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Smack: Provide read control for io_uring_cmd 2022-08-30 14:16 ` Casey Schaufler @ 2022-08-31 7:15 ` Joel Granados 0 siblings, 0 replies; 25+ messages in thread From: Joel Granados @ 2022-08-31 7:15 UTC (permalink / raw) To: Casey Schaufler Cc: Kanchan Joshi, Paul Moore, Jens Axboe, Ankit Kumar, io-uring [-- Attachment #1: Type: text/plain, Size: 8404 bytes --] On Tue, Aug 30, 2022 at 07:16:55AM -0700, Casey Schaufler wrote: > On 8/30/2022 6:08 AM, Joel Granados wrote: > > Hey Casey > > > > I have tested this patch and I see the smack_uring_cmd prevents access > > to file when smack labels are different. They way I got there was a bit > > convoluted and I'll describe it here so ppl can judge how valid the test > > is. > > > > Tested-by: Joel Granados <[email protected]> > > Thank you. np > > > > > I started by reproducing what Kanchan had done by changing the smack > > label from "_" to "Snap". Then I ran the io_uring passthrough test > > included in liburing with an unprivileged user and saw that the > > smack_uring_cmd function was NOT executed because smack prevented an open on > > the device file ("/dev/ng0n1" on my box). > > > > So here I got a bit sneaky and changed the label after the open to the > > device was done. This is how I did it: > > 1. In the io_uring_passthrough.c tests I stopped execution after the > > device was open and before the actual IO. > > 2. While on hold I changed the label of the device from "_" to "Snap". > > At this point the device had a "Snap" label and the executable had a > > "_" label. > > 3. Previous to execution I had placed a printk inside the > > smack_uring_cmd function. And I also made sure to printk whenever > > security_uring_command returned because of a security violation. > > 4. I continued execution and saw that both smack_uiring_cmd and > > io_uring_cmd returned error. Which is what I expected. > > > > I'm still a bit unsure of my setup so if anyone has comments or a way to > > make it better, I would really appreciate feedback. > > This is a perfectly rational approach to the test. Another approach > would be to add a Smack access rule: > > echo "_ Snap r" > /sys/fs/smackfs/load2 > > and label the device before the test begins. Step 2 changes from labeling > the device to removing the access rule: > > echo "_ Snap -" > /sys/fs/smackfs/load2 > > and you will get the same result. I wouldn't change your test, but I > would probably add another that does it using the rule change. Followed your proposal and I could see that it went passed the "file open: permission denied" error. However it did not execute smack_uring_cmd as smack prevented execution of an ioctl call [1]. This is probably because the test that I'm using from liburing does a lot of things to set things up besides just opening the device. I tried several strings on /sys/fs/smackfs/load2 but had no luck at actually arriving to the smack_uring_cmd function. Here is what I tried: 1. echo "_ Snap r-x---" > /sys/fs/smackfs/load2 which prevented access but not in smack_uring_cmd 2. echo "_ Snap -wx---" > /sys/fs/smackfs/load2 This of course prevented me from opening the /dev/ng0n1 3. echo "_ Snap rw----" > /sys/fs/smackfs/load2 This went through the smack_uring_cmd and allowed the interaction. [1] : Here is the traceback of where smack prevents execution of the ioctl call: #0 smk_tskacc (tsp=0xffff888107a27300, obj_known=0xffff888105dda540, mode=mode@entry=2, a=a@entry=0xffffc90000c3be80) at ../security/smack/smack_access.c:258 #1 0xffffffff8143fbb0 in smk_curacc (obj_known=<optimized out>, mode=mode@entry=2, a=a@entry=0xffffc90000c3be80) at ../security/smack/smack_access.c:278 #2 0xffffffff8143b4e4 in smack_file_ioctl (file=<optimized out>, cmd=3225964097, arg=<optimized out>) at ../security/smack/smack_lsm.c:1539 #3 0xffffffff81411c3f in security_file_ioctl (file=file@entry=0xffff8881038c8b00, cmd=cmd@entry=3225964097, arg=arg@entry=140728408424048) at ../security/security.c:1552 #4 0xffffffff8126ca3e in __do_sys_ioctl (arg=140728408424048, cmd=3225964097, fd=3) at ../fs/ioctl.c:864 #5 __se_sys_ioctl (arg=140728408424048, cmd=3225964097, fd=3) at ../fs/ioctl.c:856 #6 __x64_sys_ioctl (regs=<optimized out>) at ../fs/ioctl.c:856 #7 0xffffffff81da0978 in do_syscall_x64 (nr=<optimized out>, regs=0xffffc90000c3bf58) at ../arch/x86/entry/common.c:50 #8 do_syscall_64 (regs=0xffffc90000c3bf58, nr=<optimized out>) at ../arch/x86/entry/common.c:80 #9 0xffffffff81e0009b in entry_SYSCALL_64 () at ../arch/x86/entry/entry_64.S:120 #10 0x00007f9c22ae9000 in ?? () #11 0x0000000000000000 in ?? () > > > Best > > > > Joel > > > > On Mon, Aug 29, 2022 at 09:20:09AM -0700, Casey Schaufler wrote: > >> On 8/27/2022 8:59 AM, Kanchan Joshi wrote: > >>> On Tue, Aug 23, 2022 at 04:46:18PM -0700, Casey Schaufler wrote: > >>>> Limit io_uring "cmd" options to files for which the caller has > >>>> Smack read access. There may be cases where the cmd option may > >>>> be closer to a write access than a read, but there is no way > >>>> to make that determination. > >>>> > >>>> Signed-off-by: Casey Schaufler <[email protected]> > >>>> -- > >>>> security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 32 insertions(+) > >>>> > >>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > >>>> index 001831458fa2..bffccdc494cb 100644 > >>>> --- a/security/smack/smack_lsm.c > >>>> +++ b/security/smack/smack_lsm.c > >>>> @@ -42,6 +42,7 @@ > >>>> #include <linux/fs_context.h> > >>>> #include <linux/fs_parser.h> > >>>> #include <linux/watch_queue.h> > >>>> +#include <linux/io_uring.h> > >>>> #include "smack.h" > >>>> > >>>> #define TRANS_TRUE "TRUE" > >>>> @@ -4732,6 +4733,36 @@ static int smack_uring_sqpoll(void) > >>>> return -EPERM; > >>>> } > >>>> > >>>> +/** > >>>> + * smack_uring_cmd - check on file operations for io_uring > >>>> + * @ioucmd: the command in question > >>>> + * > >>>> + * Make a best guess about whether a io_uring "command" should > >>>> + * be allowed. Use the same logic used for determining if the > >>>> + * file could be opened for read in the absence of better criteria. > >>>> + */ > >>>> +static int smack_uring_cmd(struct io_uring_cmd *ioucmd) > >>>> +{ > >>>> + struct file *file = ioucmd->file; > >>>> + struct smk_audit_info ad; > >>>> + struct task_smack *tsp; > >>>> + struct inode *inode; > >>>> + int rc; > >>>> + > >>>> + if (!file) > >>>> + return -EINVAL; > >>>> + > >>>> + tsp = smack_cred(file->f_cred); > >>>> + inode = file_inode(file); > >>>> + > >>>> + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH); > >>>> + smk_ad_setfield_u_fs_path(&ad, file->f_path); > >>>> + rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad); > >>>> + rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc); > >>>> + > >>>> + return rc; > >>>> +} > >>>> + > >>>> #endif /* CONFIG_IO_URING */ > >>>> > >>>> struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = { > >>>> @@ -4889,6 +4920,7 @@ static struct security_hook_list smack_hooks[] > >>>> __lsm_ro_after_init = { > >>>> #ifdef CONFIG_IO_URING > >>>> LSM_HOOK_INIT(uring_override_creds, smack_uring_override_creds), > >>>> LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll), > >>>> + LSM_HOOK_INIT(uring_cmd, smack_uring_cmd), > >>>> #endif > >>> Tried this on nvme device (/dev/ng0n1). > >>> Took a while to come out of noob setup-related issues but I see that > >>> smack is listed (in /sys/kernel/security/lsm), smackfs is present, and > >>> the hook (smack_uring_cmd) gets triggered fine on doing I/O on > >>> /dev/ng0n1. > >>> > >>> I/O goes fine, which seems aligned with the label on /dev/ng0n1 (which > >>> is set to floor). > >>> > >>> $ chsmack -L /dev/ng0n1 > >>> /dev/ng0n1 access="_" > >> Setting the Smack on the object that the cmd operates on to > >> something other than "_" would be the correct test. If that > >> is /dev/ng0n1 you could use > >> > >> # chsmack -a Snap /dev/ng0n1 > >> > >> The unprivileged user won't be able to read /dev/ng0n1 so you > >> won't get as far as testing the cmd interface. I don't know > >> io_uring and nvme well enough to know what other objects may > >> be involved. Noob here, too. > >> > >>> I ran fio (/usr/bin/fio), which also has the same label. > >>> Hope you expect the same outcome. > >>> > >>> Do you run something else to see that things are fine e.g. for > >>> /dev/null, which also has the same label "_". > >>> If yes, I can try the same on nvme side. > >>> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2022-08-31 7:19 UTC | newest] Thread overview: 25+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <CGME20220719135821epcas5p1b071b0162cc3e1eb803ca687989f106d@epcas5p1.samsung.com> 2022-07-19 13:52 ` [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands Ankit Kumar [not found] ` <CGME20220719135832epcas5p31bb7df7c931aba12454b6f16c966a7c8@epcas5p3.samsung.com> 2022-07-19 13:52 ` [PATCH liburing 1/5] configure: check for nvme uring command support Ankit Kumar [not found] ` <CGME20220719135834epcas5p2f63a49277322756394f19e23a1c4e4ce@epcas5p2.samsung.com> 2022-07-19 13:52 ` [PATCH liburing 2/5] io_uring.h: sync sqe entry with 5.20 io_uring Ankit Kumar [not found] ` <CGME20220719135835epcas5p2284cbb16a28c4290d3a886449bc7a6d8@epcas5p2.samsung.com> 2022-07-19 13:52 ` [PATCH liburing 3/5] nvme: add nvme opcodes, structures and helper functions Ankit Kumar [not found] ` <CGME20220719135836epcas5p3f28b20cab964ced538d5a7fdfe367bb4@epcas5p3.samsung.com> 2022-07-19 13:52 ` [PATCH liburing 4/5] test: add io_uring passthrough test Ankit Kumar [not found] ` <CGME20220719135837epcas5p1eb4beb20bdfbdaaa7409d7b1f6355909@epcas5p1.samsung.com> 2022-07-19 13:52 ` [PATCH liburing 5/5] test/io_uring_passthrough: add test case for poll IO Ankit Kumar 2022-08-12 0:43 ` [PATCH liburing 0/5] Add basic test for nvme uring passthrough commands Casey Schaufler 2022-08-12 1:51 ` Jens Axboe 2022-08-12 15:33 ` Paul Moore 2022-08-12 16:03 ` Casey Schaufler 2022-08-13 11:35 ` Kanchan Joshi 2022-08-23 23:46 ` [PATCH] Smack: Provide read control for io_uring_cmd Casey Schaufler 2022-08-24 0:05 ` Paul Moore 2022-08-24 0:07 ` Jens Axboe 2022-08-26 15:15 ` Paul Moore 2022-08-26 16:53 ` Casey Schaufler 2022-08-26 18:59 ` Paul Moore 2022-08-26 19:04 ` Casey Schaufler 2022-08-26 19:10 ` Paul Moore 2022-08-26 19:31 ` Casey Schaufler 2022-08-27 15:59 ` Kanchan Joshi 2022-08-29 16:20 ` Casey Schaufler 2022-08-30 13:08 ` Joel Granados 2022-08-30 14:16 ` Casey Schaufler 2022-08-31 7:15 ` Joel Granados
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox