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

[Jens Axboe <axboe@kernel.dk>]

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