Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported protocols

[Jens Axboe <axboe@kernel.dk>]

On 10/20/22 5:53 AM, Pavel Begunkov wrote:
> On 10/20/22 13:49, Jens Axboe wrote:
>> On 10/20/22 2:13 AM, Stefan Metzmacher wrote:
>>> Hi Pavel,
>>>
>>>> If a protocol doesn't support zerocopy it will silently fall back to
>>>> copying. This type of behaviour has always been a source of troubles
>>>> so it's better to fail such requests instead. For now explicitly
>>>> whitelist supported protocols in io_uring, which should be turned later
>>>> into a socket flag.
>>>>
>>>> Cc: <stable@vger.kernel.org> # 6.0
>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>> ---
>>>> ?? io_uring/net.c | 9 +++++++++
>>>> ?? 1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/io_uring/net.c b/io_uring/net.c
>>>> index 8c7226b5bf41..28127f1de1f0 100644
>>>> --- a/io_uring/net.c
>>>> +++ b/io_uring/net.c
>>>> @@ -120,6 +120,13 @@ static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags)
>>>> ?????? }
>>>> ?? }
>>>> ?? +static inline bool io_sock_support_zc(struct socket *sock)
>>>> +{
>>>> +??? return likely(sock->sk && sk_fullsock(sock->sk) &&
>>>> +???????????? (sock->sk->sk_protocol == IPPROTO_TCP ||
>>>> +????????????? sock->sk->sk_protocol == IPPROTO_UDP));
>>>> +}
>>>
>>> Can we please make this more generic (at least for 6.1, which is likely be an lts release)
>>>
>>> It means my out of tree smbdirect driver would not be able to provide SENDMSG_ZC.
>>>
>>> Currently sk_setsockopt has this logic:
>>>
>>> ???????? case SO_ZEROCOPY:
>>> ???????????????? if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6) {
>>> ???????????????????????? if (!(sk_is_tcp(sk) ||
>>> ?????????????????????????????? (sk->sk_type == SOCK_DGRAM &&
>>> ??????????????????????????????? sk->sk_protocol == IPPROTO_UDP)))
>>> ???????????????????????????????? ret = -EOPNOTSUPP;
>>> ???????????????? } else if (sk->sk_family != PF_RDS) {
>>> ???????????????????????? ret = -EOPNOTSUPP;
>>> ???????????????? }
>>> ???????????????? if (!ret) {
>>> ???????????????????????? if (val < 0 || val > 1)
>>> ???????????????????????????????? ret = -EINVAL;
>>> ???????????????????????? else
>>> ???????????????????????????????? sock_valbool_flag(sk, SOCK_ZEROCOPY, valbool);
>>> ???????????????? }
>>> ???????????????? break;
>>>
>>> Maybe the socket creation code could set
>>> unsigned char skc_so_zerocopy_supported:1;
>>> and/or
>>> unsigned char skc_zerocopy_msg_ubuf_supported:1;
>>>
>>> In order to avoid the manual complex tests.
>>
>> I agree that would be cleaner, even for 6.1. Let's drop these two
>> for now.
> 
> As I mentioned let's drop, but if not for smb I do think it's
> better as doesn't require changes in multiple /net files.

I do think it's cleaner to do as a socket flag rather than hardcode it
in the caller (and potentially making bad assumptions, even if the
out-of-tree code is a bit of a reach for sure).

-- 
Jens Axboe