Re: [PATCH] io_uring/net: save msghdr->msg_control for retries

[Jens Axboe <axboe@kernel.dk>]

On 6/19/23 8:40 AM, Stefan Metzmacher wrote:
> Am 19.06.23 um 16:38 schrieb Jens Axboe:
>> On 6/19/23 7:27?AM, Stefan Metzmacher wrote:
>>> Am 19.06.23 um 15:09 schrieb Stefan Metzmacher:
>>>> Am 19.06.23 um 15:05 schrieb Jens Axboe:
>>>>> On 6/19/23 3:57?AM, Stefan Metzmacher wrote:
>>>>>> Hi Jens,
>>>>>>
>>>>>>> If the application sets ->msg_control and we have to later retry this
>>>>>>> command, or if it got queued with IOSQE_ASYNC to begin with, then we
>>>>>>> need to retain the original msg_control value. This is due to the net
>>>>>>> stack overwriting this field with an in-kernel pointer, to copy it
>>>>>>> in. Hitting that path for the second time will now fail the copy from
>>>>>>> user, as it's attempting to copy from a non-user address.
>>>>>>
>>>>>> I'm not 100% sure about the impact of this change.
>>>>>>
>>>>>> But I think the logic we need is that only the
>>>>>> first __sys_sendmsg_sock() that returns > 0 should
>>>>>> see msg_control. A retry because of MSG_WAITALL should
>>>>>> clear msg_control[len] for a follow up __sys_sendmsg_sock().
>>>>>> And I fear the patch below would not clear it...
>>>>>>
>>>>>> Otherwise the receiver/socket-layer will get the same msg_control twice,
>>>>>> which is unexpected.
>>>>>
>>>>> Yes agree, if we do transfer some (but not all) data and WAITALL is set,
>>>>> it should get cleared. I'll post a patch for that.
>>>>
>>>> Thanks!
>>>>
>>>>> Note that it was also broken before, just differently broken. The most
>>>>> likely outcome here was a full retry and now getting -EFAULT.
>>>>
>>>> Yes, I can see that it was broken before...
>>>
>>> I haven't checked myself, but I'm wondering about the recvmsg case,
>>> I guess we would need to advance the msg_control buffer after each
>>> iteration, in order to avoid overwritting the already received messages
>>> on retry.
>>>
>>> This all gets complicated with things like MSG_CTRUNC.
>>>
>>> I guess it's too late to reject MSG_WAITALL together with msg_control
>>> for io_recvmsg() because of compat reasons,
>>> but as MSG_WAITALL is also processed in the socket layer, we could keep it
>>> simple for now and skip the this retry logic:
>>>
>>>          if (flags & MSG_WAITALL)
>>>                  min_ret = iov_iter_count(&kmsg->msg.msg_iter);
>>>
>>> This might become something similar to this,
>>> but likely more complex, as would need to record kmsg->controllen == 0
>>> condition already in io_recvmsg_prep:
>>>
>>>          if (flags & MSG_WAITALL && kmsg->controllen == 0)
>>>                  min_ret = iov_iter_count(&kmsg->msg.msg_iter);
>>
>> Yep agree, I think this is the best way - ensure that once we transfer
>> data with cmsg, it's a one-shot kind of deal.
>>
>> Do you want to cut a patch for that one?
> 
> No, sorry I'm busy with other stuff and not able to to do any testing...

OK that's fine, I'll post both.

-- 
Jens Axboe