| Message ID | 20230724142237.358769-3-leitao@debian.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | io_uring: Initial support for {s,g}etsockopt commands | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On 07/24, Breno Leitao wrote: > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, > where a sockptr_t is either userspace or kernel space, and handled as > such. > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). We probably need to also have bpf bits in the new io_uring_cmd_getsockopt? > Differently from the getsockopt(2), the optlen field is not a userspace > pointers. In getsockopt(2), userspace provides optlen pointer, which is > overwritten by the kernel. In this implementation, userspace passes a > u32, and the new value is returned in cqe->res. I.e., optlen is not a > pointer. > > Important to say that userspace needs to keep the pointer alive until > the CQE is completed. > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > include/uapi/linux/io_uring.h | 7 ++++++ > io_uring/uring_cmd.c | 43 +++++++++++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h > index 9fc7195f25df..8152151080db 100644 > --- a/include/uapi/linux/io_uring.h > +++ b/include/uapi/linux/io_uring.h > @@ -43,6 +43,10 @@ struct io_uring_sqe { > union { > __u64 addr; /* pointer to buffer or iovecs */ > __u64 splice_off_in; > + struct { > + __u32 level; > + __u32 optname; > + }; > }; > __u32 len; /* buffer size or number of iovecs */ > union { > @@ -79,6 +83,7 @@ struct io_uring_sqe { > union { > __s32 splice_fd_in; > __u32 file_index; > + __u32 optlen; > struct { > __u16 addr_len; > __u16 __pad3[1]; > @@ -89,6 +94,7 @@ struct io_uring_sqe { > __u64 addr3; > __u64 __pad2[1]; > }; > + __u64 optval; > /* > * If the ring is initialized with IORING_SETUP_SQE128, then > * this field is used for 80 bytes of arbitrary command data > @@ -729,6 +735,7 @@ struct io_uring_recvmsg_out { > enum { > SOCKET_URING_OP_SIOCINQ = 0, > SOCKET_URING_OP_SIOCOUTQ, > + SOCKET_URING_OP_GETSOCKOPT, > }; > > #ifdef __cplusplus > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > index 8e7a03c1b20e..16c857cbf3b0 100644 > --- a/io_uring/uring_cmd.c > +++ b/io_uring/uring_cmd.c > @@ -166,6 +166,47 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, > } > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); > > +static inline int io_uring_cmd_getsockopt(struct socket *sock, > + struct io_uring_cmd *cmd) > +{ > + void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); > + int optname = READ_ONCE(cmd->sqe->optname); > + int optlen = READ_ONCE(cmd->sqe->optlen); > + int level = READ_ONCE(cmd->sqe->level); > + void *koptval; > + int err; > + > + err = security_socket_getsockopt(sock, level, optname); > + if (err) > + return err; > + > + koptval = kmalloc(optlen, GFP_KERNEL); > + if (!koptval) > + return -ENOMEM; > + > + err = copy_from_user(koptval, optval, optlen); > + if (err) > + goto fail; > + > + err = -EOPNOTSUPP; > + if (level == SOL_SOCKET) { > + err = sk_getsockopt(sock->sk, level, optname, > + KERNEL_SOCKPTR(koptval), > + KERNEL_SOCKPTR(&optlen)); > + if (err) > + goto fail; > + } > + > + err = copy_to_user(optval, koptval, optlen); > + > +fail: > + kfree(koptval); > + if (err) > + return err; > + else > + return optlen; > +} > + > int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > { > struct socket *sock = cmd->file->private_data; > @@ -187,6 +228,8 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > if (ret) > return ret; > return arg; > + case SOCKET_URING_OP_GETSOCKOPT: > + return io_uring_cmd_getsockopt(sock, cmd); > default: > return -EOPNOTSUPP; > } > -- > 2.34.1 >
Breno Leitao wrote: > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, > where a sockptr_t is either userspace or kernel space, and handled as > such. > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). > > Differently from the getsockopt(2), the optlen field is not a userspace > pointers. In getsockopt(2), userspace provides optlen pointer, which is > overwritten by the kernel. In this implementation, userspace passes a > u32, and the new value is returned in cqe->res. I.e., optlen is not a > pointer. > > Important to say that userspace needs to keep the pointer alive until > the CQE is completed. > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > include/uapi/linux/io_uring.h | 7 ++++++ > io_uring/uring_cmd.c | 43 +++++++++++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h > index 9fc7195f25df..8152151080db 100644 > --- a/include/uapi/linux/io_uring.h > +++ b/include/uapi/linux/io_uring.h > @@ -43,6 +43,10 @@ struct io_uring_sqe { > union { > __u64 addr; /* pointer to buffer or iovecs */ > __u64 splice_off_in; > + struct { > + __u32 level; > + __u32 optname; > + }; > }; > __u32 len; /* buffer size or number of iovecs */ > union { > @@ -79,6 +83,7 @@ struct io_uring_sqe { > union { > __s32 splice_fd_in; > __u32 file_index; > + __u32 optlen; > struct { > __u16 addr_len; > __u16 __pad3[1]; > @@ -89,6 +94,7 @@ struct io_uring_sqe { > __u64 addr3; > __u64 __pad2[1]; > }; > + __u64 optval; > /* > * If the ring is initialized with IORING_SETUP_SQE128, then > * this field is used for 80 bytes of arbitrary command data > @@ -729,6 +735,7 @@ struct io_uring_recvmsg_out { > enum { > SOCKET_URING_OP_SIOCINQ = 0, > SOCKET_URING_OP_SIOCOUTQ, > + SOCKET_URING_OP_GETSOCKOPT, > }; > > #ifdef __cplusplus > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > index 8e7a03c1b20e..16c857cbf3b0 100644 > --- a/io_uring/uring_cmd.c > +++ b/io_uring/uring_cmd.c > @@ -166,6 +166,47 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, > } > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); > > +static inline int io_uring_cmd_getsockopt(struct socket *sock, > + struct io_uring_cmd *cmd) > +{ > + void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); > + int optname = READ_ONCE(cmd->sqe->optname); > + int optlen = READ_ONCE(cmd->sqe->optlen); > + int level = READ_ONCE(cmd->sqe->level); > + void *koptval; > + int err; > + > + err = security_socket_getsockopt(sock, level, optname); > + if (err) > + return err; > + > + koptval = kmalloc(optlen, GFP_KERNEL); > + if (!koptval) > + return -ENOMEM; This will try to kmalloc any length that userspace passes? That is unnecessary .. > + > + err = copy_from_user(koptval, optval, optlen); > + if (err) > + goto fail; > + > + err = -EOPNOTSUPP; > + if (level == SOL_SOCKET) { > + err = sk_getsockopt(sock->sk, level, optname, > + KERNEL_SOCKPTR(koptval), > + KERNEL_SOCKPTR(&optlen)); .. sk_getsockopt defines a union of acceptable fields, which are all fairly small. I notice that BPF added BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN to work around the issue of pre-allocating for the worst case. But that also needs to deal woth other getsockopt levels. > + if (err) > + goto fail; > + } > + > + err = copy_to_user(optval, koptval, optlen); > + > +fail: > + kfree(koptval); > + if (err) > + return err; > + else > + return optlen; > +} > + > int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > { > struct socket *sock = cmd->file->private_data; > @@ -187,6 +228,8 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > if (ret) > return ret; > return arg; > + case SOCKET_URING_OP_GETSOCKOPT: > + return io_uring_cmd_getsockopt(sock, cmd); > default: > return -EOPNOTSUPP; > } > -- > 2.34.1 >
On Mon, Jul 24, 2023 at 10:31:28AM -0700, Stanislav Fomichev wrote: > On 07/24, Breno Leitao wrote: > > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where > > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, > > where a sockptr_t is either userspace or kernel space, and handled as > > such. > > > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). > > We probably need to also have bpf bits in the new > io_uring_cmd_getsockopt? It might be interesting to have the BPF hook for this function as well, but I would like to do it in a following patch, so, I can experiment with it better, if that is OK.
On Mon, Jul 24, 2023 at 06:58:04PM -0400, Willem de Bruijn wrote: > Breno Leitao wrote: > > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where > > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, > > where a sockptr_t is either userspace or kernel space, and handled as > > such. > > > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). > > > > Differently from the getsockopt(2), the optlen field is not a userspace > > pointers. In getsockopt(2), userspace provides optlen pointer, which is > > overwritten by the kernel. In this implementation, userspace passes a > > u32, and the new value is returned in cqe->res. I.e., optlen is not a > > pointer. > > > > Important to say that userspace needs to keep the pointer alive until > > the CQE is completed. > > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > --- > > include/uapi/linux/io_uring.h | 7 ++++++ > > io_uring/uring_cmd.c | 43 +++++++++++++++++++++++++++++++++++ > > 2 files changed, 50 insertions(+) > > > > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h > > index 9fc7195f25df..8152151080db 100644 > > --- a/include/uapi/linux/io_uring.h > > +++ b/include/uapi/linux/io_uring.h > > @@ -43,6 +43,10 @@ struct io_uring_sqe { > > union { > > __u64 addr; /* pointer to buffer or iovecs */ > > __u64 splice_off_in; > > + struct { > > + __u32 level; > > + __u32 optname; > > + }; > > }; > > __u32 len; /* buffer size or number of iovecs */ > > union { > > @@ -79,6 +83,7 @@ struct io_uring_sqe { > > union { > > __s32 splice_fd_in; > > __u32 file_index; > > + __u32 optlen; > > struct { > > __u16 addr_len; > > __u16 __pad3[1]; > > @@ -89,6 +94,7 @@ struct io_uring_sqe { > > __u64 addr3; > > __u64 __pad2[1]; > > }; > > + __u64 optval; > > /* > > * If the ring is initialized with IORING_SETUP_SQE128, then > > * this field is used for 80 bytes of arbitrary command data > > @@ -729,6 +735,7 @@ struct io_uring_recvmsg_out { > > enum { > > SOCKET_URING_OP_SIOCINQ = 0, > > SOCKET_URING_OP_SIOCOUTQ, > > + SOCKET_URING_OP_GETSOCKOPT, > > }; > > > > #ifdef __cplusplus > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > > index 8e7a03c1b20e..16c857cbf3b0 100644 > > --- a/io_uring/uring_cmd.c > > +++ b/io_uring/uring_cmd.c > > @@ -166,6 +166,47 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, > > } > > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); > > > > +static inline int io_uring_cmd_getsockopt(struct socket *sock, > > + struct io_uring_cmd *cmd) > > +{ > > + void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); > > + int optname = READ_ONCE(cmd->sqe->optname); > > + int optlen = READ_ONCE(cmd->sqe->optlen); > > + int level = READ_ONCE(cmd->sqe->level); > > + void *koptval; > > + int err; > > + > > + err = security_socket_getsockopt(sock, level, optname); > > + if (err) > > + return err; > > + > > + koptval = kmalloc(optlen, GFP_KERNEL); > > + if (!koptval) > > + return -ENOMEM; > > This will try to kmalloc any length that userspace passes? Yes, this value is coming directly from userspace. > That is unnecessary .. > > + > > + err = copy_from_user(koptval, optval, optlen); > > + if (err) > > + goto fail; > > + > > + err = -EOPNOTSUPP; > > + if (level == SOL_SOCKET) { > > + err = sk_getsockopt(sock->sk, level, optname, > > + KERNEL_SOCKPTR(koptval), > > + KERNEL_SOCKPTR(&optlen)); > > .. sk_getsockopt defines a union of acceptable fields, which > are all fairly small. Right, and they are all I need for SOL_SOCKET level for now. > I notice that BPF added BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN to > work around the issue of pre-allocating for the worst case. I am having a hard time how to understand how BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN gets the MAX_OPTLEN. Reading this function, it seems it is conditionally get_user(). #define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen) ({ int __ret = 0; if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT)) get_user(__ret, optlen); __ret; }) That said, how is BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN being used to workaroundthe pre-allocating for the worst case? > But that also needs to deal woth other getsockopt levels. Right, and we also have a similar kmalloc() on the setsockopt path (SOCKET_URING_OP_SETSOCKOPT). What about if I pass the userspace sockptr (USER_SOCKPTR) to the {g,s}etsockopt callback directly, instead of kmalloc() in io_uring(), as I was doing int the RFC[1]? It avoids any extra kmalloc at all. Something as: static inline int io_uring_cmd_getsockopt(struct socket *sock, struct io_uring_cmd *cmd) { void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); int optlen = READ_ONCE(cmd->sqe->optlen); int optname = READ_ONCE(cmd->sqe->optname); int level = READ_ONCE(cmd->sqe->level); int err; err = security_socket_getsockopt(sock, level, optname); if (err) return err; if (level == SOL_SOCKET) { err = sk_getsockopt(sock->sk, level, optname, USER_SOCKPTR(optval), KERNEL_SOCKPTR(&optlen)); if (err < 0) return err; return optlen; } return -EOPNOTSUPP; Thanks for the review! [1] Link: https://lore.kernel.org/all/20230719102737.2513246-3-leitao@debian.org/
Breno Leitao wrote: > On Mon, Jul 24, 2023 at 06:58:04PM -0400, Willem de Bruijn wrote: > > Breno Leitao wrote: > > > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where > > > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, > > > where a sockptr_t is either userspace or kernel space, and handled as > > > such. > > > > > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). > > > > > > Differently from the getsockopt(2), the optlen field is not a userspace > > > pointers. In getsockopt(2), userspace provides optlen pointer, which is > > > overwritten by the kernel. In this implementation, userspace passes a > > > u32, and the new value is returned in cqe->res. I.e., optlen is not a > > > pointer. > > > > > > Important to say that userspace needs to keep the pointer alive until > > > the CQE is completed. > > > > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > > --- > > > include/uapi/linux/io_uring.h | 7 ++++++ > > > io_uring/uring_cmd.c | 43 +++++++++++++++++++++++++++++++++++ > > > 2 files changed, 50 insertions(+) > > > > > > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h > > > index 9fc7195f25df..8152151080db 100644 > > > --- a/include/uapi/linux/io_uring.h > > > +++ b/include/uapi/linux/io_uring.h > > > @@ -43,6 +43,10 @@ struct io_uring_sqe { > > > union { > > > __u64 addr; /* pointer to buffer or iovecs */ > > > __u64 splice_off_in; > > > + struct { > > > + __u32 level; > > > + __u32 optname; > > > + }; > > > }; > > > __u32 len; /* buffer size or number of iovecs */ > > > union { > > > @@ -79,6 +83,7 @@ struct io_uring_sqe { > > > union { > > > __s32 splice_fd_in; > > > __u32 file_index; > > > + __u32 optlen; > > > struct { > > > __u16 addr_len; > > > __u16 __pad3[1]; > > > @@ -89,6 +94,7 @@ struct io_uring_sqe { > > > __u64 addr3; > > > __u64 __pad2[1]; > > > }; > > > + __u64 optval; > > > /* > > > * If the ring is initialized with IORING_SETUP_SQE128, then > > > * this field is used for 80 bytes of arbitrary command data > > > @@ -729,6 +735,7 @@ struct io_uring_recvmsg_out { > > > enum { > > > SOCKET_URING_OP_SIOCINQ = 0, > > > SOCKET_URING_OP_SIOCOUTQ, > > > + SOCKET_URING_OP_GETSOCKOPT, > > > }; > > > > > > #ifdef __cplusplus > > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > > > index 8e7a03c1b20e..16c857cbf3b0 100644 > > > --- a/io_uring/uring_cmd.c > > > +++ b/io_uring/uring_cmd.c > > > @@ -166,6 +166,47 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, > > > } > > > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); > > > > > > +static inline int io_uring_cmd_getsockopt(struct socket *sock, > > > + struct io_uring_cmd *cmd) > > > +{ > > > + void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); > > > + int optname = READ_ONCE(cmd->sqe->optname); > > > + int optlen = READ_ONCE(cmd->sqe->optlen); > > > + int level = READ_ONCE(cmd->sqe->level); > > > + void *koptval; > > > + int err; > > > + > > > + err = security_socket_getsockopt(sock, level, optname); > > > + if (err) > > > + return err; > > > + > > > + koptval = kmalloc(optlen, GFP_KERNEL); > > > + if (!koptval) > > > + return -ENOMEM; > > > > This will try to kmalloc any length that userspace passes? > > Yes, this value is coming directly from userspace. > > > That is unnecessary .. > > > + > > > + err = copy_from_user(koptval, optval, optlen); > > > + if (err) > > > + goto fail; > > > + > > > + err = -EOPNOTSUPP; > > > + if (level == SOL_SOCKET) { > > > + err = sk_getsockopt(sock->sk, level, optname, > > > + KERNEL_SOCKPTR(koptval), > > > + KERNEL_SOCKPTR(&optlen)); > > > > .. sk_getsockopt defines a union of acceptable fields, which > > are all fairly small. > > Right, and they are all I need for SOL_SOCKET level for now. > > > I notice that BPF added BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN to > > work around the issue of pre-allocating for the worst case. > > I am having a hard time how to understand how > BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN gets the MAX_OPTLEN. Reading this > function, it seems it is conditionally get_user(). > > > #define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen) > ({ > int __ret = 0; > if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT)) > get_user(__ret, optlen); > __ret; > }) > > That said, how is BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN being used to > workaroundthe pre-allocating for the worst case? > > > But that also needs to deal woth other getsockopt levels. > > Right, and we also have a similar kmalloc() on the setsockopt path > (SOCKET_URING_OP_SETSOCKOPT). > > What about if I pass the userspace sockptr (USER_SOCKPTR) to the > {g,s}etsockopt callback directly, instead of kmalloc() in io_uring(), as > I was doing int the RFC[1]? It avoids any extra kmalloc at all. That looks like a great solution to me. Avoids the whole problem of kmalloc based on untrusted user input. > Something as: > > static inline int io_uring_cmd_getsockopt(struct socket *sock, > struct io_uring_cmd *cmd) > { > void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); > int optlen = READ_ONCE(cmd->sqe->optlen); > int optname = READ_ONCE(cmd->sqe->optname); > int level = READ_ONCE(cmd->sqe->level); > int err; > > err = security_socket_getsockopt(sock, level, optname); > if (err) > return err; > > if (level == SOL_SOCKET) { > err = sk_getsockopt(sock->sk, level, optname, > USER_SOCKPTR(optval), > KERNEL_SOCKPTR(&optlen)); > if (err < 0) > return err; > return optlen; > } Do you have a plan to extend this to other levels? No need to implement immediately, but it would be good to know whether it is feasible to extend the current solution when the need (inevitably) shows up. > > return -EOPNOTSUPP; > > Thanks for the review! > > [1] Link: https://lore.kernel.org/all/20230719102737.2513246-3-leitao@debian.org/
On Tue, Jul 25, 2023 at 09:56:50AM -0400, Willem de Bruijn wrote: > Breno Leitao wrote: > > On Mon, Jul 24, 2023 at 06:58:04PM -0400, Willem de Bruijn wrote: > > > Breno Leitao wrote: > > > > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where > > > > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, > > > > where a sockptr_t is either userspace or kernel space, and handled as > > > > such. > > > > > > > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). > > > > > > > > Differently from the getsockopt(2), the optlen field is not a userspace > > > > pointers. In getsockopt(2), userspace provides optlen pointer, which is > > > > overwritten by the kernel. In this implementation, userspace passes a > > > > u32, and the new value is returned in cqe->res. I.e., optlen is not a > > > > pointer. > > > > > > > > Important to say that userspace needs to keep the pointer alive until > > > > the CQE is completed. > > > > > > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > > > --- > > > > include/uapi/linux/io_uring.h | 7 ++++++ > > > > io_uring/uring_cmd.c | 43 +++++++++++++++++++++++++++++++++++ > > > > 2 files changed, 50 insertions(+) > > > > > > > > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h > > > > index 9fc7195f25df..8152151080db 100644 > > > > --- a/include/uapi/linux/io_uring.h > > > > +++ b/include/uapi/linux/io_uring.h > > > > @@ -43,6 +43,10 @@ struct io_uring_sqe { > > > > union { > > > > __u64 addr; /* pointer to buffer or iovecs */ > > > > __u64 splice_off_in; > > > > + struct { > > > > + __u32 level; > > > > + __u32 optname; > > > > + }; > > > > }; > > > > __u32 len; /* buffer size or number of iovecs */ > > > > union { > > > > @@ -79,6 +83,7 @@ struct io_uring_sqe { > > > > union { > > > > __s32 splice_fd_in; > > > > __u32 file_index; > > > > + __u32 optlen; > > > > struct { > > > > __u16 addr_len; > > > > __u16 __pad3[1]; > > > > @@ -89,6 +94,7 @@ struct io_uring_sqe { > > > > __u64 addr3; > > > > __u64 __pad2[1]; > > > > }; > > > > + __u64 optval; > > > > /* > > > > * If the ring is initialized with IORING_SETUP_SQE128, then > > > > * this field is used for 80 bytes of arbitrary command data > > > > @@ -729,6 +735,7 @@ struct io_uring_recvmsg_out { > > > > enum { > > > > SOCKET_URING_OP_SIOCINQ = 0, > > > > SOCKET_URING_OP_SIOCOUTQ, > > > > + SOCKET_URING_OP_GETSOCKOPT, > > > > }; > > > > > > > > #ifdef __cplusplus > > > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > > > > index 8e7a03c1b20e..16c857cbf3b0 100644 > > > > --- a/io_uring/uring_cmd.c > > > > +++ b/io_uring/uring_cmd.c > > > > @@ -166,6 +166,47 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, > > > > } > > > > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); > > > > > > > > +static inline int io_uring_cmd_getsockopt(struct socket *sock, > > > > + struct io_uring_cmd *cmd) > > > > +{ > > > > + void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); > > > > + int optname = READ_ONCE(cmd->sqe->optname); > > > > + int optlen = READ_ONCE(cmd->sqe->optlen); > > > > + int level = READ_ONCE(cmd->sqe->level); > > > > + void *koptval; > > > > + int err; > > > > + > > > > + err = security_socket_getsockopt(sock, level, optname); > > > > + if (err) > > > > + return err; > > > > + > > > > + koptval = kmalloc(optlen, GFP_KERNEL); > > > > + if (!koptval) > > > > + return -ENOMEM; > > > > > > This will try to kmalloc any length that userspace passes? > > > > Yes, this value is coming directly from userspace. > > > > > That is unnecessary .. > > > > + > > > > + err = copy_from_user(koptval, optval, optlen); > > > > + if (err) > > > > + goto fail; > > > > + > > > > + err = -EOPNOTSUPP; > > > > + if (level == SOL_SOCKET) { > > > > + err = sk_getsockopt(sock->sk, level, optname, > > > > + KERNEL_SOCKPTR(koptval), > > > > + KERNEL_SOCKPTR(&optlen)); > > > > > > .. sk_getsockopt defines a union of acceptable fields, which > > > are all fairly small. > > > > Right, and they are all I need for SOL_SOCKET level for now. > > > > > I notice that BPF added BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN to > > > work around the issue of pre-allocating for the worst case. > > > > I am having a hard time how to understand how > > BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN gets the MAX_OPTLEN. Reading this > > function, it seems it is conditionally get_user(). > > > > > > #define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen) > > ({ > > int __ret = 0; > > if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT)) > > get_user(__ret, optlen); > > __ret; > > }) > > > > That said, how is BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN being used to > > workaroundthe pre-allocating for the worst case? > > > > > But that also needs to deal woth other getsockopt levels. > > > > Right, and we also have a similar kmalloc() on the setsockopt path > > (SOCKET_URING_OP_SETSOCKOPT). > > > > What about if I pass the userspace sockptr (USER_SOCKPTR) to the > > {g,s}etsockopt callback directly, instead of kmalloc() in io_uring(), as > > I was doing int the RFC[1]? It avoids any extra kmalloc at all. > > That looks like a great solution to me. > > Avoids the whole problem of kmalloc based on untrusted user input. > > > Something as: > > > > static inline int io_uring_cmd_getsockopt(struct socket *sock, > > struct io_uring_cmd *cmd) > > { > > void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); > > int optlen = READ_ONCE(cmd->sqe->optlen); > > int optname = READ_ONCE(cmd->sqe->optname); > > int level = READ_ONCE(cmd->sqe->level); > > int err; > > > > err = security_socket_getsockopt(sock, level, optname); > > if (err) > > return err; > > > > if (level == SOL_SOCKET) { > > err = sk_getsockopt(sock->sk, level, optname, > > USER_SOCKPTR(optval), > > KERNEL_SOCKPTR(&optlen)); > > if (err < 0) > > return err; > > return optlen; > > } > > Do you have a plan to extend this to other levels? > > No need to implement immediately, but it would be good to know > whether it is feasible to extend the current solution when the > need (inevitably) shows up. Yes, I plan to extend getsockopt() to all levels, but it means we need to convert proto_ops->setsockopt to uset sockptr_t instead of userpointers. This might require some intrusive changes, but totally doable.
On 07/25, Breno Leitao wrote: > On Mon, Jul 24, 2023 at 10:31:28AM -0700, Stanislav Fomichev wrote: > > On 07/24, Breno Leitao wrote: > > > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where > > > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, > > > where a sockptr_t is either userspace or kernel space, and handled as > > > such. > > > > > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). > > > > We probably need to also have bpf bits in the new > > io_uring_cmd_getsockopt? > > It might be interesting to have the BPF hook for this function as > well, but I would like to do it in a following patch, so, I can > experiment with it better, if that is OK. We are not using io_uring, so fine with me. However, having a way to bypass get/setsockopt bpf might be problematic for some other heavy io_uring users. Lemme CC a bunch of Meta folks explicitly. I'm not sure what that state of bpf support in io_uring.
On 7/25/23 10:02 AM, Stanislav Fomichev wrote: > On 07/25, Breno Leitao wrote: >> On Mon, Jul 24, 2023 at 10:31:28AM -0700, Stanislav Fomichev wrote: >>> On 07/24, Breno Leitao wrote: >>>> Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where >>>> level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, >>>> where a sockptr_t is either userspace or kernel space, and handled as >>>> such. >>>> >>>> Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). >>> >>> We probably need to also have bpf bits in the new >>> io_uring_cmd_getsockopt? I also think this inconsistency behavior should be avoided. >> >> It might be interesting to have the BPF hook for this function as >> well, but I would like to do it in a following patch, so, I can >> experiment with it better, if that is OK. > > We are not using io_uring, so fine with me. However, having a way to bypass > get/setsockopt bpf might be problematic for some other heavy io_uring > users. > > Lemme CC a bunch of Meta folks explicitly. I'm not sure what that state > of bpf support in io_uring. We have use cases on the "cgroup/{g,s}etsockopt". It will be a surprise when the user moves from the syscall {g,s}etsockopt to SOCKET_URING_OP_*SOCKOPT and figured that the bpf handling is skipped.
On Tue, Jul 25, 2023 at 10:56:23AM -0700, Martin KaFai Lau wrote: > On 7/25/23 10:02 AM, Stanislav Fomichev wrote: > > On 07/25, Breno Leitao wrote: > > > On Mon, Jul 24, 2023 at 10:31:28AM -0700, Stanislav Fomichev wrote: > > > > On 07/24, Breno Leitao wrote: > > > > > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where > > > > > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, > > > > > where a sockptr_t is either userspace or kernel space, and handled as > > > > > such. > > > > > > > > > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). > > > > > > > > We probably need to also have bpf bits in the new > > > > io_uring_cmd_getsockopt? > > I also think this inconsistency behavior should be avoided. > > > > > > > It might be interesting to have the BPF hook for this function as > > > well, but I would like to do it in a following patch, so, I can > > > experiment with it better, if that is OK. > > > > We are not using io_uring, so fine with me. However, having a way to bypass > > get/setsockopt bpf might be problematic for some other heavy io_uring > > users. > > > > Lemme CC a bunch of Meta folks explicitly. I'm not sure what that state > > of bpf support in io_uring. > > We have use cases on the "cgroup/{g,s}etsockopt". It will be a surprise when > the user moves from the syscall {g,s}etsockopt to SOCKET_URING_OP_*SOCKOPT > and figured that the bpf handling is skipped. Ok, I will add the BPF bits in the next revision then. Thanks for clarifying it.
Hello Stanislav, On Tue, Jul 25, 2023 at 10:02:40AM -0700, Stanislav Fomichev wrote: > On 07/25, Breno Leitao wrote: > > On Mon, Jul 24, 2023 at 10:31:28AM -0700, Stanislav Fomichev wrote: > > > On 07/24, Breno Leitao wrote: > > > > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where > > > > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, > > > > where a sockptr_t is either userspace or kernel space, and handled as > > > > such. > > > > > > > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). > > > > > > We probably need to also have bpf bits in the new > > > io_uring_cmd_getsockopt? > > > > It might be interesting to have the BPF hook for this function as > > well, but I would like to do it in a following patch, so, I can > > experiment with it better, if that is OK. I spent smoe time looking at the problem, and I understand we want to call something as BPF_CGROUP_RUN_PROG_{G,S}ETSOCKOPT() into io_uring_cmd_{g,s}etsockopt(). Per the previous conversation with Williem, io_uring_cmd_{g,s}etsockopt() should use optval as a user pointer (void __user *optval), and optlen as a kernel integer (it comes as from the io_uring SQE), such as: void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); int optlen = READ_ONCE(cmd->sqe->optlen); Function BPF_CGROUP_RUN_PROG_GETSOCKOPT() calls __cgroup_bpf_run_filter_getsockopt() which expects userpointer for optlen and optval. At the same time BPF_CGROUP_RUN_PROG_GETSOCKOPT_KERN() expects kernel pointers for both optlen and optval. In this current patchset, it has user pointer for optval and kernel value for optlen. I.e., a third combination. So, none of the functions would work properly, and we probably do not want to create another function. I am wondering if it is a good idea to move __cgroup_bpf_run_filter_getsockopt() to use sockptr_t, so, it will be able to adapt to any combination. Any feedback is appreciate. Thanks!
On Fri, Jul 28, 2023 at 10:03 AM Breno Leitao <leitao@debian.org> wrote: > > Hello Stanislav, > > On Tue, Jul 25, 2023 at 10:02:40AM -0700, Stanislav Fomichev wrote: > > On 07/25, Breno Leitao wrote: > > > On Mon, Jul 24, 2023 at 10:31:28AM -0700, Stanislav Fomichev wrote: > > > > On 07/24, Breno Leitao wrote: > > > > > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where > > > > > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, > > > > > where a sockptr_t is either userspace or kernel space, and handled as > > > > > such. > > > > > > > > > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). > > > > > > > > We probably need to also have bpf bits in the new > > > > io_uring_cmd_getsockopt? > > > > > > It might be interesting to have the BPF hook for this function as > > > well, but I would like to do it in a following patch, so, I can > > > experiment with it better, if that is OK. > > I spent smoe time looking at the problem, and I understand we want to > call something as BPF_CGROUP_RUN_PROG_{G,S}ETSOCKOPT() into > io_uring_cmd_{g,s}etsockopt(). > > Per the previous conversation with Williem, > io_uring_cmd_{g,s}etsockopt() should use optval as a user pointer (void __user > *optval), and optlen as a kernel integer (it comes as from the io_uring > SQE), such as: > > void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); > int optlen = READ_ONCE(cmd->sqe->optlen); > > Function BPF_CGROUP_RUN_PROG_GETSOCKOPT() calls > __cgroup_bpf_run_filter_getsockopt() which expects userpointer for > optlen and optval. > > At the same time BPF_CGROUP_RUN_PROG_GETSOCKOPT_KERN() expects kernel > pointers for both optlen and optval. > > In this current patchset, it has user pointer for optval and kernel value > for optlen. I.e., a third combination. So, none of the functions would > work properly, and we probably do not want to create another function. > > I am wondering if it is a good idea to move > __cgroup_bpf_run_filter_getsockopt() to use sockptr_t, so, it will be > able to adapt to any combination. Yeah, I think it makes sense. However, note that the intent of that optlen being a __user pointer is to possibly write some (updated) value back into the userspace. Presumably, you'll pass that updated optlen into some io_uring completion queue? (maybe a stupid question, not super familiar with io_uring) > Any feedback is appreciate. > Thanks!
On Fri, Jul 28, 2023 at 11:07:10AM -0700, Stanislav Fomichev wrote: > On Fri, Jul 28, 2023 at 10:03 AM Breno Leitao <leitao@debian.org> wrote: > > > > Hello Stanislav, > > > > On Tue, Jul 25, 2023 at 10:02:40AM -0700, Stanislav Fomichev wrote: > > > On 07/25, Breno Leitao wrote: > > > > On Mon, Jul 24, 2023 at 10:31:28AM -0700, Stanislav Fomichev wrote: > > > > > On 07/24, Breno Leitao wrote: > > > > > > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where > > > > > > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, > > > > > > where a sockptr_t is either userspace or kernel space, and handled as > > > > > > such. > > > > > > > > > > > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). > > > > > > > > > > We probably need to also have bpf bits in the new > > > > > io_uring_cmd_getsockopt? > > > > > > > > It might be interesting to have the BPF hook for this function as > > > > well, but I would like to do it in a following patch, so, I can > > > > experiment with it better, if that is OK. > > > > I spent smoe time looking at the problem, and I understand we want to > > call something as BPF_CGROUP_RUN_PROG_{G,S}ETSOCKOPT() into > > io_uring_cmd_{g,s}etsockopt(). > > > > Per the previous conversation with Williem, > > io_uring_cmd_{g,s}etsockopt() should use optval as a user pointer (void __user > > *optval), and optlen as a kernel integer (it comes as from the io_uring > > SQE), such as: > > > > void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); > > int optlen = READ_ONCE(cmd->sqe->optlen); > > > > Function BPF_CGROUP_RUN_PROG_GETSOCKOPT() calls > > __cgroup_bpf_run_filter_getsockopt() which expects userpointer for > > optlen and optval. > > > > At the same time BPF_CGROUP_RUN_PROG_GETSOCKOPT_KERN() expects kernel > > pointers for both optlen and optval. > > > > In this current patchset, it has user pointer for optval and kernel value > > for optlen. I.e., a third combination. So, none of the functions would > > work properly, and we probably do not want to create another function. > > > > I am wondering if it is a good idea to move > > __cgroup_bpf_run_filter_getsockopt() to use sockptr_t, so, it will be > > able to adapt to any combination. > > Yeah, I think it makes sense. However, note that the intent of that > optlen being a __user pointer is to possibly write some (updated) > value back into the userspace. > Presumably, you'll pass that updated optlen into some io_uring > completion queue? (maybe a stupid question, not super familiar with > io_uring) On io_uring proposal, the optlen is part of the SQE for setsockopt(). You give a userpointer (optval) and set the optlen in the SQE->optlen. For getsockopt(), the optlen is returned as a result of the operation, in the CQE->res. If you need more detail about it, I documented this behaviour in the cover-letter (PS1): https://lore.kernel.org/all/20230724142237.358769-1-leitao@debian.org/ Thanks for the feedback!
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 9fc7195f25df..8152151080db 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -43,6 +43,10 @@ struct io_uring_sqe { union { __u64 addr; /* pointer to buffer or iovecs */ __u64 splice_off_in; + struct { + __u32 level; + __u32 optname; + }; }; __u32 len; /* buffer size or number of iovecs */ union { @@ -79,6 +83,7 @@ struct io_uring_sqe { union { __s32 splice_fd_in; __u32 file_index; + __u32 optlen; struct { __u16 addr_len; __u16 __pad3[1]; @@ -89,6 +94,7 @@ struct io_uring_sqe { __u64 addr3; __u64 __pad2[1]; }; + __u64 optval; /* * If the ring is initialized with IORING_SETUP_SQE128, then * this field is used for 80 bytes of arbitrary command data @@ -729,6 +735,7 @@ struct io_uring_recvmsg_out { enum { SOCKET_URING_OP_SIOCINQ = 0, SOCKET_URING_OP_SIOCOUTQ, + SOCKET_URING_OP_GETSOCKOPT, }; #ifdef __cplusplus diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c index 8e7a03c1b20e..16c857cbf3b0 100644 --- a/io_uring/uring_cmd.c +++ b/io_uring/uring_cmd.c @@ -166,6 +166,47 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, } EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); +static inline int io_uring_cmd_getsockopt(struct socket *sock, + struct io_uring_cmd *cmd) +{ + void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval)); + int optname = READ_ONCE(cmd->sqe->optname); + int optlen = READ_ONCE(cmd->sqe->optlen); + int level = READ_ONCE(cmd->sqe->level); + void *koptval; + int err; + + err = security_socket_getsockopt(sock, level, optname); + if (err) + return err; + + koptval = kmalloc(optlen, GFP_KERNEL); + if (!koptval) + return -ENOMEM; + + err = copy_from_user(koptval, optval, optlen); + if (err) + goto fail; + + err = -EOPNOTSUPP; + if (level == SOL_SOCKET) { + err = sk_getsockopt(sock->sk, level, optname, + KERNEL_SOCKPTR(koptval), + KERNEL_SOCKPTR(&optlen)); + if (err) + goto fail; + } + + err = copy_to_user(optval, koptval, optlen); + +fail: + kfree(koptval); + if (err) + return err; + else + return optlen; +} + int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) { struct socket *sock = cmd->file->private_data; @@ -187,6 +228,8 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) if (ret) return ret; return arg; + case SOCKET_URING_OP_GETSOCKOPT: + return io_uring_cmd_getsockopt(sock, cmd); default: return -EOPNOTSUPP; }
Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure, where a sockptr_t is either userspace or kernel space, and handled as such. Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt(). Differently from the getsockopt(2), the optlen field is not a userspace pointers. In getsockopt(2), userspace provides optlen pointer, which is overwritten by the kernel. In this implementation, userspace passes a u32, and the new value is returned in cqe->res. I.e., optlen is not a pointer. Important to say that userspace needs to keep the pointer alive until the CQE is completed. Signed-off-by: Breno Leitao <leitao@debian.org> --- include/uapi/linux/io_uring.h | 7 ++++++ io_uring/uring_cmd.c | 43 +++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+)