netdev - Re: Solving address deletion bottleneck in SCTP

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: <CADvbK_c5jNywSZOwSb-qcfxoNwauG1vkQFg6a8h4QOq50Q9uSQ@mail.gmail.com>
Date: Tue, 29 Oct 2024 11:06:00 -0400
From: Xin Long <lucien.xin@...il.com>
To: Gilad Naaman <gnaaman@...venets.com>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, linux-sctp@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: Solving address deletion bottleneck in SCTP

On Mon, Oct 28, 2024 at 8:49 AM Gilad Naaman <gnaaman@...venets.com> wrote:
>
> Hello,
>
> We've noticed that when a namespace has a large amount of IP addresses,
> the list `net->sctp.local_addr_list` gets obscenely long.
>
> This list contains both IPv4 and IPv6 addresses, of all scopes, and it is
> a single long list, instead of a hashtable.
>
> In our case we had 12K interfaces, each with an IPv4 and 2 IPv6 addresses
> (GUA+LLA), which made deletion of a single address pretty expensive, since
> it requires a linear search through 36K addresses.
>
> Internally we solved it pretty naively by turning the list into hashmap, which
> helped us avoid this bottleneck:
>
>     + #define SCTP_ADDR_HSIZE_SHIFT     8
>     + #define SCTP_ADDR_HSIZE           (1 << SCTP_ADDR_HSIZE_SHIFT)
>
>     -   struct list_head local_addr_list;
>     +   struct list_head local_addr_list[SCTP_ADDR_HSIZE];
>
>
> I've used the same factor used by the IPv6 & IPv4 address tables.
>
> I am not entirely sure this patch solves a big enough problem for the greater
> general kernel community to warrant the increased memory usage (~2KiB-p-netns),
> so I'll avoid sending it.
>
> Recently, though, both IPv4 and IPv6 tables were namespacified, which makes
> me think that maybe local_addr_list is no longer necessary, enabling us to
> them directly instead of maintaining a separate list.
>
> As far as I could tell, the only field of `struct sctp_sockaddr_entry` that
> are used for items of this list, aside from the address itself, is the `valid`
> bit, which can probably be folded into `struct in_ifaddr` and `struct inet6_ifaddr`.
>
> What I'm suggesting, in short is:
>  - Represent `valid` inside the original address structs.
>  - Replace iteration of `local_addr_list` with iteration of ns addr tables
>  - Eliminate `local_addr_list`
>
> Is this a reasonable proposal?
This would simplify sctp_inet6addr_event() and sctp_inetaddr_event(),
but complicate sctp_copy_laddrs() and sctp_copy_local_addr_list().

Would you like to create a patch for this and let's see how it looks?

Note I don't think that that 'valid' bit is useful:

               if (addr->a.sa.sa_family == AF_INET &&
                               addr->a.v4.sin_addr.s_addr ==
                               ifa->ifa_local) {
                       sctp_addr_wq_mgmt(net, addr, SCTP_ADDR_DEL);
                       found = 1;
                                      <-------- [1]
                       addr->valid = 0;
                       list_del_rcu(&addr->list);
                       break;
               }

'addr' can be copied before "addr->valid = 0;" with addr->valid =1 in
another thread anyway. I think you can ignore this 'valid' bit.

Thanks.