netdev - Re: [PATCH net-next 2/2] datagram, udp: Set local address and rehash socket atomically against lookup

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: <857fae8c-d0cd-4c9d-bfd0-881af9e77b0f@redhat.com>
Date: Fri, 6 Dec 2024 16:10:18 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Stefano Brivio <sbrivio@...hat.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
 Kuniyuki Iwashima <kuniyu@...zon.com>, Mike Manning <mvrmanning@...il.com>,
 David Gibson <david@...son.dropbear.id.au>,
 Paul Holzinger <pholzing@...hat.com>, Philo Lu <lulie@...ux.alibaba.com>,
 Cambda Zhu <cambda@...ux.alibaba.com>, Fred Chen <fred.cc@...baba-inc.com>,
 Yubing Qiu <yubing.qiuyubing@...baba-inc.com>
Subject: Re: [PATCH net-next 2/2] datagram, udp: Set local address and rehash
 socket atomically against lookup



On 12/6/24 14:35, Stefano Brivio wrote:
> On Fri, 6 Dec 2024 13:36:47 +0100
> Paolo Abeni <pabeni@...hat.com> wrote:
> 
>> On 12/6/24 11:50, Stefano Brivio wrote:
>>> On Thu, 5 Dec 2024 17:53:33 +0100 Paolo Abeni <pabeni@...hat.com> wrote:  
>>>> I'm wondering if the issue could be solved (almost) entirely in the
>>>> rehash callback?!? if the rehash happens on connect and the the socket
>>>> does not have hash4 yet (it's not a reconnect) do the l4 hashing before
>>>> everything else.  
>>>
>>> So, yes, that's actually the first thing I tried: do the hashing (any
>>> hash) before setting the address (I guess that's what you mean by
>>> "everything else").
>>>
>>> If you take this series, and drop the changes in __udp4_lib_lookup(), I
>>> guess that would match what you suggest.  
>>
>> I mean something slightly different. Just to explain the idea something
>> alike the following (completely untested):
>>
>> ---
>> diff --git a/net/ipv4/datagram.c b/net/ipv4/datagram.c
>> index cc6d0bd7b0a9..e9cc6edbcdc6 100644
>> --- a/net/ipv4/datagram.c
>> +++ b/net/ipv4/datagram.c
>> @@ -61,6 +61,10 @@ int __ip4_datagram_connect(struct sock *sk, struct
>> sockaddr *uaddr, int addr_len
>>  		err = -EACCES;
>>  		goto out;
>>  	}
>> +
>> +	sk->sk_state = TCP_ESTABLISHED;
>> +	inet->inet_daddr = fl4->daddr;
>> +	inet->inet_dport = usin->sin_port;
>>  	if (!inet->inet_saddr)
>>  		inet->inet_saddr = fl4->saddr;	/* Update source address */
>>  	if (!inet->inet_rcv_saddr) {
>> @@ -68,10 +72,7 @@ int __ip4_datagram_connect(struct sock *sk, struct
>> sockaddr *uaddr, int addr_len
>>  		if (sk->sk_prot->rehash)
>>  			sk->sk_prot->rehash(sk);
>>  	}
>> -	inet->inet_daddr = fl4->daddr;
>> -	inet->inet_dport = usin->sin_port;

Side note: I think that moving the initialization of the above fields
before the rehash is separate fix - otherwise reconnect will screw hash4.

I'll submit just that (sub) chunk separately.

Cheers,

Paolo