netdev - Re: [PATCH for-6.1 1/3] net: flag sockets supporting msghdr originated zerocopy

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]

Date:   Mon, 24 Oct 2022 19:45:42 +0200
From:   Stefan Metzmacher <metze@...ba.org>
To:     Pavel Begunkov <asml.silence@...il.com>,
        Jens Axboe <axboe@...nel.dk>, Jakub Kicinski <kuba@...nel.org>
Cc:     Paolo Abeni <pabeni@...hat.com>,
        "David S . Miller" <davem@...emloft.net>, io-uring@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH for-6.1 1/3] net: flag sockets supporting msghdr
 originated zerocopy

Am 24.10.22 um 19:00 schrieb Pavel Begunkov:
> On 10/24/22 15:12, Stefan Metzmacher wrote:
>> Am 24.10.22 um 14:49 schrieb Stefan Metzmacher:
>>> Am 22.10.22 um 18:07 schrieb Jens Axboe:
>>>> On 10/22/22 9:57 AM, Pavel Begunkov wrote:
>>>>> On 10/21/22 17:14, Jakub Kicinski wrote:
>>>>>> On Fri, 21 Oct 2022 11:16:39 +0100 Pavel Begunkov wrote:
>>>>>>> We need an efficient way in io_uring to check whether a socket supports
>>>>>>> zerocopy with msghdr provided ubuf_info. Add a new flag into the struct
>>>>>>> socket flags fields.
>>>>>>>
>>>>>>> Cc: <stable@...r.kernel.org> # 6.0
>>>>>>> Signed-off-by: Pavel Begunkov <asml.silence@...il.com>
>>>>>>> ---
>>>>>>> ? include/linux/net.h | 1 +
>>>>>>> ? net/ipv4/tcp.c????? | 1 +
>>>>>>> ? net/ipv4/udp.c????? | 1 +
>>>>>>> ? 3 files changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/linux/net.h b/include/linux/net.h
>>>>>>> index 711c3593c3b8..18d942bbdf6e 100644
>>>>>>> --- a/include/linux/net.h
>>>>>>> +++ b/include/linux/net.h
>>>>>>> @@ -41,6 +41,7 @@ struct net;
>>>>>>> ? #define SOCK_NOSPACE??????? 2
>>>>>>> ? #define SOCK_PASSCRED??????? 3
>>>>>>> ? #define SOCK_PASSSEC??????? 4
>>>>>>> +#define SOCK_SUPPORT_ZC??????? 5
>>>>>>
>>>>>> Acked-by: Jakub Kicinski <kuba@...nel.org>
>>>>>>
>>>>>> Any idea on when this will make it to Linus? If within a week we can
>>>>>> probably delay:
>>>>>
>>>>> After a chat with Jens, IIUC he can take it and send out to
>>>>> Linus early. So, sounds like a good plan
>>>>
>>>> Yes, and let's retain the name for now, can always be changed if we need
>>>> to make it more granular. I'll ship this off before -rc2.
>>>
>>> I'm now always getting -EOPNOTSUPP from SENDMSG_ZC for tcp connections...
>>
>> The problem is that inet_accept() doesn't set SOCK_SUPPORT_ZC...
> 
> Yeah, you're right, accept is not handled, not great. Thanks
> for tracking it down. And I'll add a test for it
> 
> 
>> This hack below fixes it for me, but I'm sure there's a nicer way...
>>
>> The natural way would be to overload inet_csk_accept for tcp,
>> but sk1->sk_prot->accept() is called with sk2->sk_socket == NULL,
>> because sock_graft() is called later...
> 
> I think it's acceptable for a fix. sk_is_tcp() sounds a bit better
> assuming that sk_type/protocol are set there.
> 
> 
>> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
>> index 3dd02396517d..0f35f2a32756 100644
>> --- a/net/ipv4/af_inet.c
>> +++ b/net/ipv4/af_inet.c
>> @@ -736,6 +736,7 @@ EXPORT_SYMBOL(inet_stream_connect);
>>    *    Accept a pending connection. The TCP layer now gives BSD semantics.
>>    */
>>
>> +#include <net/transp_v6.h>
>>   int inet_accept(struct socket *sock, struct socket *newsock, int flags,
>>           bool kern)
>>   {
>> @@ -754,6 +755,8 @@ int inet_accept(struct socket *sock, struct socket *newsock, int flags,
>>             (TCPF_ESTABLISHED | TCPF_SYN_RECV |
>>             TCPF_CLOSE_WAIT | TCPF_CLOSE)));
>>
>> +    if (sk2->sk_prot == &tcp_prot || sk2->sk_prot == &tcpv6_prot)
>> +        set_bit(SOCK_SUPPORT_ZC, &newsock->flags);
>>       sock_graft(sk2, newsock);

Hmm, I think we just need to check if the flag was set on the listening socket,
otherwise we have a hard coded value again...

This also works for me, and seems like a much nicer patch:

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 3dd02396517d..4728087c42a5 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -754,6 +754,8 @@ int inet_accept(struct socket *sock, struct socket *newsock, int flags,
  		  (TCPF_ESTABLISHED | TCPF_SYN_RECV |
  		  TCPF_CLOSE_WAIT | TCPF_CLOSE)));

+	if (test_bit(SOCK_SUPPORT_ZC, &sock->flags))
+		set_bit(SOCK_SUPPORT_ZC, &newsock->flags);
  	sock_graft(sk2, newsock);

  	newsock->state = SS_CONNECTED;