netdev - Re: [PATCH 2/2] Rescan the hash2 list if the hash chains have got cross-linked.

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CANn89iKTC29of9bkVKWcLv0W27JFvkub7fuBMeK_J3a3Q-B1Cg@mail.gmail.com>
Date: Wed, 26 Jul 2023 15:37:09 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: David Laight <David.Laight@...lab.com>
Cc: "willemdebruijn.kernel@...il.com" <willemdebruijn.kernel@...il.com>, 
	"davem@...emloft.net" <davem@...emloft.net>, "dsahern@...nel.org" <dsahern@...nel.org>, 
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/2] Rescan the hash2 list if the hash chains have got cross-linked.

On Wed, Jul 26, 2023 at 2:06 PM David Laight <David.Laight@...lab.com> wrote:
>
> udp_lib_rehash() can get called at any time and will move a
> socket to a different hash2 chain.
> This can cause udp4_lib_lookup2() (processing incoming UDP) to
> fail to find a socket and an ICMP port unreachable be sent.
>
> Prior to ca065d0cf80fa the lookup used 'hlist_nulls' and checked
> that the 'end if list' marker was on the correct list.
>
> Rather than re-instate the 'nulls' list just check that the final
> socket is on the correct list.
>
> The cross-linking can definitely happen (see earlier issues with
> it looping forever because gcc cached the list head).
>
> Fixes: ca065d0cf80fa ("udp: no longer use SLAB_DESTROY_BY_RCU")
> Signed-off-by: David Laight <david.laight@...lab.com>
> ---

Hi David, thanks a lot for the investigations.

I do not think this is the proper fix.

UDP rehash has always been buggy, because we lack an rcu grace period
between the removal of the socket
from the old hash bucket to the new one.

We need to stuff a synchronize_rcu() somewhere in udp_lib_rehash(),
and that might not be easy [1]
and might add unexpected latency to some real time applications.
([1] : Not sure if we are allowed to sleep in udp_lib_rehash())

Also note that adding a synchronize_rcu() would mean the socket would
not be found anyway by some incoming packets.

I think that rehash is tricky to implement if you expect that all
incoming packets must find the socket, wherever it is located.


>  net/ipv4/udp.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index ad64d6c4cd99..ed92ba7610b0 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -443,6 +443,7 @@ static struct sock *udp4_lib_lookup2(struct net *net,
>                                      struct sk_buff *skb)
>  {
>         unsigned int hash2, slot2;
> +       unsigned int hash2_rescan;
>         struct udp_hslot *hslot2;
>         struct sock *sk, *result;
>         int score, badness;
> @@ -451,9 +452,12 @@ static struct sock *udp4_lib_lookup2(struct net *net,
>         slot2 = hash2 & udptable->mask;
>         hslot2 = &udptable->hash2[slot2];
>
> +rescan:
> +       hash2_rescan = hash2;
>         result = NULL;
>         badness = 0;
>         udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
> +               hash2_rescan = udp_sk(sk)->udp_portaddr_hash;
>                 score = compute_score(sk, net, saddr, sport,
>                                       daddr, hnum, dif, sdif);
>                 if (score > badness) {
> @@ -467,6 +471,16 @@ static struct sock *udp4_lib_lookup2(struct net *net,
>                         badness = score;
>                 }
>         }
> +
> +       /* udp sockets can get moved to a different hash chain.
> +        * If the chains have got crossed then rescan.
> +        */
> +       if ((hash2_rescan & udptable->mask) != slot2) {

This is only going to catch one of the possible cases.

If we really want to add extra checks in this fast path, we would have
to check all found sockets,
not only the last one.