| Message ID | 20231105223008.125563-2-dyudaken@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: mshot read fix for buffer size changes | expand |
On 11/5/23 3:30 PM, Dylan Yudaken wrote: > For addr: this field is not used, since buffer select is forced. But by forcing > it to be zero it leaves open future uses of the field. > > len is actually usable, you could imagine that you want to receive > multishot up to a certain length. > However right now this is not how it is implemented, and it seems > safer to force this to be zero. > > Fixes: fc68fcda0491 ("io_uring/rw: add support for IORING_OP_READ_MULTISHOT") > Signed-off-by: Dylan Yudaken <dyudaken@gmail.com> > --- > io_uring/rw.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/io_uring/rw.c b/io_uring/rw.c > index 1c76de483ef6..ea86498d8769 100644 > --- a/io_uring/rw.c > +++ b/io_uring/rw.c > @@ -111,6 +111,13 @@ int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe) > rw->len = READ_ONCE(sqe->len); > rw->flags = READ_ONCE(sqe->rw_flags); > > + if (req->opcode == IORING_OP_READ_MULTISHOT) { > + if (rw->addr) > + return -EINVAL; > + if (rw->len) > + return -EINVAL; > + } Should we just put these in io_read_mshot_prep() instead? Ala the below. In general I think it'd be nice to have a core prep_rw, and then each variant will have its own prep. Then we can get away from random opcode checking in there. I do agree with the change in general, just think we can tweak it a bit to make it a bit cleaner. diff --git a/io_uring/rw.c b/io_uring/rw.c index 1c76de483ef6..635a1bf5df70 100644 --- a/io_uring/rw.c +++ b/io_uring/rw.c @@ -129,6 +129,7 @@ int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe) */ int io_read_mshot_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) { + struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw); int ret; /* must be used with provided buffers */ @@ -139,6 +140,9 @@ int io_read_mshot_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) if (unlikely(ret)) return ret; + if (rw->addr || rw->len) + return -EINVAL; + req->flags |= REQ_F_APOLL_MULTISHOT; return 0; }
On 11/6/23 7:32 AM, Jens Axboe wrote: > On 11/5/23 3:30 PM, Dylan Yudaken wrote: >> For addr: this field is not used, since buffer select is forced. But by forcing >> it to be zero it leaves open future uses of the field. >> >> len is actually usable, you could imagine that you want to receive >> multishot up to a certain length. >> However right now this is not how it is implemented, and it seems >> safer to force this to be zero. >> >> Fixes: fc68fcda0491 ("io_uring/rw: add support for IORING_OP_READ_MULTISHOT") >> Signed-off-by: Dylan Yudaken <dyudaken@gmail.com> >> --- >> io_uring/rw.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/io_uring/rw.c b/io_uring/rw.c >> index 1c76de483ef6..ea86498d8769 100644 >> --- a/io_uring/rw.c >> +++ b/io_uring/rw.c >> @@ -111,6 +111,13 @@ int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe) >> rw->len = READ_ONCE(sqe->len); >> rw->flags = READ_ONCE(sqe->rw_flags); >> >> + if (req->opcode == IORING_OP_READ_MULTISHOT) { >> + if (rw->addr) >> + return -EINVAL; >> + if (rw->len) >> + return -EINVAL; >> + } > > Should we just put these in io_read_mshot_prep() instead? Ala the below. > In general I think it'd be nice to have a core prep_rw, and then each > variant will have its own prep. Then we can get away from random opcode > checking in there. > > I do agree with the change in general, just think we can tweak it a bit > to make it a bit cleaner. Sent out two cleanups that take it in this direction in general, fwiw.
On Mon, Nov 6, 2023 at 2:51 PM Jens Axboe <axboe@kernel.dk> wrote: > > On 11/6/23 7:32 AM, Jens Axboe wrote: > > On 11/5/23 3:30 PM, Dylan Yudaken wrote: > >> For addr: this field is not used, since buffer select is forced. But by forcing > >> it to be zero it leaves open future uses of the field. > >> > >> len is actually usable, you could imagine that you want to receive > >> multishot up to a certain length. > >> However right now this is not how it is implemented, and it seems > >> safer to force this to be zero. > >> > >> Fixes: fc68fcda0491 ("io_uring/rw: add support for IORING_OP_READ_MULTISHOT") > >> Signed-off-by: Dylan Yudaken <dyudaken@gmail.com> > >> --- > >> io_uring/rw.c | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/io_uring/rw.c b/io_uring/rw.c > >> index 1c76de483ef6..ea86498d8769 100644 > >> --- a/io_uring/rw.c > >> +++ b/io_uring/rw.c > >> @@ -111,6 +111,13 @@ int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe) > >> rw->len = READ_ONCE(sqe->len); > >> rw->flags = READ_ONCE(sqe->rw_flags); > >> > >> + if (req->opcode == IORING_OP_READ_MULTISHOT) { > >> + if (rw->addr) > >> + return -EINVAL; > >> + if (rw->len) > >> + return -EINVAL; > >> + } > > > > Should we just put these in io_read_mshot_prep() instead? Ala the below. > > In general I think it'd be nice to have a core prep_rw, and then each > > variant will have its own prep. Then we can get away from random opcode > > checking in there. > > > > I do agree with the change in general, just think we can tweak it a bit > > to make it a bit cleaner. > > Sent out two cleanups that take it in this direction in general, fwiw. Yes - I think this approach is better, will rebase on these
diff --git a/io_uring/rw.c b/io_uring/rw.c index 1c76de483ef6..ea86498d8769 100644 --- a/io_uring/rw.c +++ b/io_uring/rw.c @@ -111,6 +111,13 @@ int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe) rw->len = READ_ONCE(sqe->len); rw->flags = READ_ONCE(sqe->rw_flags); + if (req->opcode == IORING_OP_READ_MULTISHOT) { + if (rw->addr) + return -EINVAL; + if (rw->len) + return -EINVAL; + } + /* Have to do this validation here, as this is in io_read() rw->len might * have chanaged due to buffer selection */
For addr: this field is not used, since buffer select is forced. But by forcing it to be zero it leaves open future uses of the field. len is actually usable, you could imagine that you want to receive multishot up to a certain length. However right now this is not how it is implemented, and it seems safer to force this to be zero. Fixes: fc68fcda0491 ("io_uring/rw: add support for IORING_OP_READ_MULTISHOT") Signed-off-by: Dylan Yudaken <dyudaken@gmail.com> --- io_uring/rw.c | 7 +++++++ 1 file changed, 7 insertions(+)