netdev - Re: [PATCH net-next v2] 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CADvbK_fV3t8Txh73gOVGh8NV401xb0dQDNJjc-YsW9kUeEkhEg@mail.gmail.com>
Date: Thu, 7 Nov 2024 09:38:40 -0500
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 v2] sctp: Avoid enqueuing addr events redundantly

On Mon, Nov 4, 2024 at 3:36 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>
Acked-by: Xin Long <lucien.xin@...il.com>

Thanks.

> ---
> Changes in v2:
>  - Reorder list removal to avoid race with new sessions
> ---
>  net/sctp/ipv6.c     |  2 +-
>  net/sctp/protocol.c | 16 +++++++++++++++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index f7b809c0d142..b96c849545ae 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -103,10 +103,10 @@ static int sctp_inet6addr_event(struct notifier_block *this, unsigned long ev,
>                             ipv6_addr_equal(&addr->a.v6.sin6_addr,
>                                             &ifa->addr) &&
>                             addr->a.v6.sin6_scope_id == ifa->idev->dev->ifindex) {
> -                               sctp_addr_wq_mgmt(net, addr, SCTP_ADDR_DEL);
>                                 found = 1;
>                                 addr->valid = 0;
>                                 list_del_rcu(&addr->list);
> +                               sctp_addr_wq_mgmt(net, addr, SCTP_ADDR_DEL);
>                                 break;
>                         }
>                 }
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index 39ca5403d4d7..8b9a1b96695e 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;
> +       }
> +
>         /* Offsets existing events in addr_wq */
>         addrw = sctp_addr_wq_lookup(net, addr);
>         if (addrw) {
> @@ -808,10 +822,10 @@ static int sctp_inetaddr_event(struct notifier_block *this, unsigned long ev,
>                         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;
>                                 addr->valid = 0;
>                                 list_del_rcu(&addr->list);
> +                               sctp_addr_wq_mgmt(net, addr, SCTP_ADDR_DEL);
>                                 break;
>                         }
>                 }
> --
> 2.34.1
>