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

[Pavel Begunkov <asml.silence@gmail.com>]

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.

-- 
Pavel Begunkov