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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <8A95C40F-233E-4828-9D19-D26CC6307CC7@drivenets.com>
Date: Tue, 29 Oct 2024 17:32:11 +0000
From: Gilad Naaman <gnaaman@...venets.com>
To: Xin Long <lucien.xin@...il.com>
CC: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
	"linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] sctp: Avoid enqueuing addr events redundantly



> On 29 Oct 2024, at 18:02, Xin Long <lucien.xin@...il.com> wrote:
> 
> CAUTION: External E-Mail - Use caution with links and attachments
> 
> 
> 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.

Great point.

I think this can be solved by making sure `list_del_rcu(&addr->list);` is called
before `sctp_addr_wq_mgmt`.

I’ll resend this after I manage to un-bork my git-send-email config.

Thank you!

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