| Message ID | 472c1b08-0409-bd55-7c4a-6d33f07efced@kernel.dk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring/net: disable partial retries for recvmsg with cmsg | expand |
Hi Jens, > We cannot sanely handle partial retries for recvmsg if we have cmsg > attached. If we don't, then we'd just be overwriting the initial cmsg > header on retries. Alternatively we could increment and handle this > appropriately, but it doesn't seem worth the complication. > > Link: https://lore.kernel.org/io-uring/0b0d4411-c8fd-4272-770b-e030af6919a0@kernel.dk/ > Cc: stable@vger.kernel.org # 5.10+ > Reported-by: Stefan Metzmacher <metze@samba.org> > Signed-off-by: Jens Axboe <axboe@kernel.dk> > > --- > > diff --git a/io_uring/net.c b/io_uring/net.c > index fe1c478c7dec..6674a0759390 100644 > --- a/io_uring/net.c > +++ b/io_uring/net.c > @@ -788,7 +788,8 @@ int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags) > flags = sr->msg_flags; > if (force_nonblock) > flags |= MSG_DONTWAIT; > - if (flags & MSG_WAITALL) > + /* disable partial retry for recvmsg with cmsg attached */ > + if (flags & MSG_WAITALL && !kmsg->controllen) > min_ret = iov_iter_count(&kmsg->msg.msg_iter); Isn't kmsg->controllen only used for REQ_F_APOLL_MULTISHOT? I guess the correct value would be kmsg->msg.msg_controllen? Maybe the safest change would be something like this (completely untested!): diff --git a/io_uring/net.c b/io_uring/net.c index 89e839013837..1dd5322fb732 100644 --- a/io_uring/net.c +++ b/io_uring/net.c @@ -781,14 +781,14 @@ int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags) flags = sr->msg_flags; if (force_nonblock) flags |= MSG_DONTWAIT; - if (flags & MSG_WAITALL) - min_ret = iov_iter_count(&kmsg->msg.msg_iter); kmsg->msg.msg_get_inq = 1; if (req->flags & REQ_F_APOLL_MULTISHOT) ret = io_recvmsg_multishot(sock, sr, kmsg, flags, &mshot_finished); else + if (flags & MSG_WAITALL && !kmsg->msg.msg_controllen) + min_ret = iov_iter_count(&kmsg->msg.msg_iter); ret = __sys_recvmsg_sock(sock, &kmsg->msg, sr->umsg, kmsg->uaddr, flags); metze
On 6/20/23 2:26?AM, Stefan Metzmacher wrote: > Hi Jens, > >> We cannot sanely handle partial retries for recvmsg if we have cmsg >> attached. If we don't, then we'd just be overwriting the initial cmsg >> header on retries. Alternatively we could increment and handle this >> appropriately, but it doesn't seem worth the complication. >> >> Link: https://lore.kernel.org/io-uring/0b0d4411-c8fd-4272-770b-e030af6919a0@kernel.dk/ >> Cc: stable@vger.kernel.org # 5.10+ >> Reported-by: Stefan Metzmacher <metze@samba.org> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> >> >> --- >> >> diff --git a/io_uring/net.c b/io_uring/net.c >> index fe1c478c7dec..6674a0759390 100644 >> --- a/io_uring/net.c >> +++ b/io_uring/net.c >> @@ -788,7 +788,8 @@ int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags) >> flags = sr->msg_flags; >> if (force_nonblock) >> flags |= MSG_DONTWAIT; >> - if (flags & MSG_WAITALL) >> + /* disable partial retry for recvmsg with cmsg attached */ >> + if (flags & MSG_WAITALL && !kmsg->controllen) >> min_ret = iov_iter_count(&kmsg->msg.msg_iter); > > Isn't kmsg->controllen only used for REQ_F_APOLL_MULTISHOT? > > I guess the correct value would be kmsg->msg.msg_controllen? Gah, yes indeed it should! > Maybe the safest change would be something like this (completely untested!): > > diff --git a/io_uring/net.c b/io_uring/net.c > index 89e839013837..1dd5322fb732 100644 > --- a/io_uring/net.c > +++ b/io_uring/net.c > @@ -781,14 +781,14 @@ int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags) > flags = sr->msg_flags; > if (force_nonblock) > flags |= MSG_DONTWAIT; > - if (flags & MSG_WAITALL) > - min_ret = iov_iter_count(&kmsg->msg.msg_iter); > > kmsg->msg.msg_get_inq = 1; > if (req->flags & REQ_F_APOLL_MULTISHOT) > ret = io_recvmsg_multishot(sock, sr, kmsg, flags, > &mshot_finished); > else > + if (flags & MSG_WAITALL && !kmsg->msg.msg_controllen) > + min_ret = iov_iter_count(&kmsg->msg.msg_iter); > ret = __sys_recvmsg_sock(sock, &kmsg->msg, sr->umsg, > kmsg->uaddr, flags); I like putting it there, as MSG_WAITALL is explicitly disallowed for multishot anyway. The check belongs in that branch rather than as a whole.
diff --git a/io_uring/net.c b/io_uring/net.c index fe1c478c7dec..6674a0759390 100644 --- a/io_uring/net.c +++ b/io_uring/net.c @@ -788,7 +788,8 @@ int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags) flags = sr->msg_flags; if (force_nonblock) flags |= MSG_DONTWAIT; - if (flags & MSG_WAITALL) + /* disable partial retry for recvmsg with cmsg attached */ + if (flags & MSG_WAITALL && !kmsg->controllen) min_ret = iov_iter_count(&kmsg->msg.msg_iter); kmsg->msg.msg_get_inq = 1;
We cannot sanely handle partial retries for recvmsg if we have cmsg attached. If we don't, then we'd just be overwriting the initial cmsg header on retries. Alternatively we could increment and handle this appropriately, but it doesn't seem worth the complication. Link: https://lore.kernel.org/io-uring/0b0d4411-c8fd-4272-770b-e030af6919a0@kernel.dk/ Cc: stable@vger.kernel.org # 5.10+ Reported-by: Stefan Metzmacher <metze@samba.org> Signed-off-by: Jens Axboe <axboe@kernel.dk> ---