| Message ID | 20240619020620.5301-4-krisman@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring op probing fixes | expand |
On 6/18/24 8:06 PM, Gabriel Krisman Bertazi wrote: > We don't need to read the userspace buffer, and the kernel side is > expected to write over it anyway. Perhaps this was meant to allow > expansion of the interface for future parameters? If we ever need to do > it, perhaps it should be done as a new io_uring opcode. Right, it's checked so that we could use it for input values in the future. By ensuring that userspace must zero it, then we could add input values and flags in the future. Is there a good reason to make this separate change? If not, I'd say drop it and we can always discuss when there's an actual need to do so. At least we have the option of passing in some information with the current code, in a backwards compatible fashion.
Jens Axboe <axboe@kernel.dk> writes: > On 6/18/24 8:06 PM, Gabriel Krisman Bertazi wrote: >> We don't need to read the userspace buffer, and the kernel side is >> expected to write over it anyway. Perhaps this was meant to allow >> expansion of the interface for future parameters? If we ever need to do >> it, perhaps it should be done as a new io_uring opcode. > > Right, it's checked so that we could use it for input values in the > future. By ensuring that userspace must zero it, then we could add input > values and flags in the future. > > Is there a good reason to make this separate change? If not, I'd say > drop it and we can always discuss when there's an actual need to do so. > At least we have the option of passing in some information with the > current code, in a backwards compatible fashion. There is no reason other than it is unused. I'm fine with dropping it. I'll wait for feedback on the other patches and, if we need a new iteration, I'll skip this one. Thanks!
On 6/19/24 8:55 AM, Gabriel Krisman Bertazi wrote: > Jens Axboe <axboe@kernel.dk> writes: > >> On 6/18/24 8:06 PM, Gabriel Krisman Bertazi wrote: >>> We don't need to read the userspace buffer, and the kernel side is >>> expected to write over it anyway. Perhaps this was meant to allow >>> expansion of the interface for future parameters? If we ever need to do >>> it, perhaps it should be done as a new io_uring opcode. >> >> Right, it's checked so that we could use it for input values in the >> future. By ensuring that userspace must zero it, then we could add input >> values and flags in the future. >> >> Is there a good reason to make this separate change? If not, I'd say >> drop it and we can always discuss when there's an actual need to do so. >> At least we have the option of passing in some information with the >> current code, in a backwards compatible fashion. > > There is no reason other than it is unused. I'm fine with dropping it. > > I'll wait for feedback on the other patches and, if we need a new > iteration, I'll skip this one. I think the rest look fine. The unsupported part was mostly a thing for backports, did use it myself internally once for the meta kernel. But I do think we should just kill it, so fine with doing that.
diff --git a/io_uring/register.c b/io_uring/register.c index 8409fc80c1cb..a60eba22141a 100644 --- a/io_uring/register.c +++ b/io_uring/register.c @@ -37,7 +37,7 @@ static __cold int io_probe(struct io_ring_ctx *ctx, void __user *arg, { struct io_uring_probe *p; size_t size; - int i, ret; + int i, ret = 0; if (nr_args > IORING_OP_LAST) nr_args = IORING_OP_LAST; @@ -47,13 +47,6 @@ static __cold int io_probe(struct io_ring_ctx *ctx, void __user *arg, if (!p) return -ENOMEM; - ret = -EFAULT; - if (copy_from_user(p, arg, size)) - goto out; - ret = -EINVAL; - if (memchr_inv(p, 0, size)) - goto out; - p->last_op = IORING_OP_LAST - 1; for (i = 0; i < nr_args; i++) { @@ -63,10 +56,8 @@ static __cold int io_probe(struct io_ring_ctx *ctx, void __user *arg, } p->ops_len = i; - ret = 0; if (copy_to_user(arg, p, size)) ret = -EFAULT; -out: kfree(p); return ret; }
We don't need to read the userspace buffer, and the kernel side is expected to write over it anyway. Perhaps this was meant to allow expansion of the interface for future parameters? If we ever need to do it, perhaps it should be done as a new io_uring opcode. Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de> --- io_uring/register.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)