netdev - Re: [PATCH net-next] sctp: Avoid enqueuing addr events redundantly

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: <CADvbK_eUK9QLzJ2HYtqQ1woAF=pcgTbvckeqCk1Es50HkxdZTg@mail.gmail.com>
Date: Tue, 29 Oct 2024 12:02:41 -0400
From: Xin Long <lucien.xin@...il.com>
To: Gilad Naaman <gnaaman@...venets.com>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, linux-sctp@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] sctp: Avoid enqueuing addr events redundantly

On Mon, Oct 28, 2024 at 4:10 AM Gilad Naaman <gnaaman@...venets.com> wrote:
>
> Avoid modifying or enqueuing new events if it's possible to tell that no
> one will consume them.
>
> Since enqueueing requires searching the current queue for opposite
> events for the same address, adding addresses en-masse turns this
> inetaddr_event into a bottle-neck, as it will get slower and slower
> with each address added.
>
> Signed-off-by: Gilad Naaman <gnaaman@...venets.com>
> ---
>  net/sctp/protocol.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index 39ca5403d4d7..2e548961b740 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -738,6 +738,20 @@ void sctp_addr_wq_mgmt(struct net *net, struct sctp_sockaddr_entry *addr, int cm
>          */
>
>         spin_lock_bh(&net->sctp.addr_wq_lock);
> +
> +       /* Avoid searching the queue or modifying it if there are no consumers,
> +        * as it can lead to performance degradation if addresses are modified
> +        * en-masse.
> +        *
> +        * If the queue already contains some events, update it anyway to avoid
> +        * ugly races between new sessions and new address events.
> +        */
> +       if (list_empty(&net->sctp.auto_asconf_splist) &&
> +           list_empty(&net->sctp.addr_waitq)) {
> +               spin_unlock_bh(&net->sctp.addr_wq_lock);
> +               return;

What if after this but before the addr is deleted from local_addr_list in
sctp_inetaddr_event(), a new SCTP association is created with these addrs
in local_addr_list, will it miss this asconf addr_del?

Thanks.

> +       }
> +
>         /* Offsets existing events in addr_wq */
>         addrw = sctp_addr_wq_lookup(net, addr);
>         if (addrw) {
> --
> 2.46.0
>