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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c31e5b41-2433-e947-c42e-cecb86c1429a@kernel.dk>
Date:   Thu, 20 Oct 2022 05:59:29 -0700
From:   Jens Axboe <axboe@...nel.dk>
To:     Pavel Begunkov <asml.silence@...il.com>,
        Stefan Metzmacher <metze@...ba.org>, io-uring@...r.kernel.org
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH for-6.1 1/2] io_uring/net: fail zc send for unsupported
 protocols

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@...r.kernel.org> # 6.0
>>>> Signed-off-by: Pavel Begunkov <asml.silence@...il.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