[PATCH] io_uring/net: disable partial retries for recvmsg with cmsg

[PATCH] io_uring/net: disable partial retries for recvmsg with cmsg

[Jens Axboe]

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);
 
 	kmsg->msg.msg_get_inq = 1;

-- 
Jens Axboe

Re: [PATCH] io_uring/net: disable partial retries for recvmsg with cmsg

[Stefan Metzmacher]

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

Re: [PATCH] io_uring/net: disable partial retries for recvmsg with cmsg

[Jens Axboe]

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.

-- 
Jens Axboe