netdev - Re: [PATCH net] sctp: use new rhlist interface on sctp transport rhashtable

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_eOu5oZJ67r8WJ-Vc0O8fLj5X4y-ROeE2aX-12Xd2jzDw@mail.gmail.com>
Date:   Wed, 16 Nov 2016 21:34:52 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Neil Horman <nhorman@...driver.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem <davem@...emloft.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Vlad Yasevich <vyasevich@...il.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Phil Sutter <phil@....cc>
Subject: Re: [PATCH net] sctp: use new rhlist interface on sctp transport rhashtable

On Wed, Nov 16, 2016 at 2:04 AM, Neil Horman <nhorman@...driver.com> wrote:
> On Tue, Nov 15, 2016 at 11:23:11PM +0800, Xin Long wrote:
>> Now sctp transport rhashtable uses hash(lport, dport, daddr) as the key
>> to hash a node to one chain. If in one host thousands of assocs connect
>> to one server with the same lport and different laddrs (although it's
>> not a normal case), all the transports would be hashed into the same
>> chain.
>>
>> It may cause to keep returning -EBUSY when inserting a new node, as the
>> chain is too long and sctp inserts a transport node in a loop, which
>> could even lead to system hangs there.
>>
>> The new rhlist interface works for this case that there are many nodes
>> with the same key in one chain. It puts them into a list then makes this
>> list be as a node of the chain.
>>
>> This patch is to replace rhashtable_ interface with rhltable_ interface.
>> Since a chain would not be too long and it would not return -EBUSY with
>> this fix when inserting a node, the reinsert loop is also removed here.
>>
>> Signed-off-by: Xin Long <lucien.xin@...il.com>
>
> Does this really buy us anything in this case though?  If the use case is that a
> majority of the associations map to the same key, then you might avoid EBUSY for
> the individual associaion that doesn't map there, but you still have to cope
> with a huge linear search for the majority of the keys.
>

This patch is NOT for improving performance, it is to reorganize
transports in rhashtable in another way to avoid EBUSY, rhlist is born
for this.

Before this patch, the transport insert codes are pretty bad, if it returns
EBUSY, it would retry in a loop. now this patch avoid this and even
removed that loop, it's a fix for this issue.

> Might be more reasonable to mix saddr into the hash function so that your use
> case gets spread through the hash table more evenly.

we can not do this:
1. it will increase rhashtable's size when using multihome, if a host has
    N addrs, the size for one assoc will be N times bigger than now.
2. the hash node is inside transport, if we mix saddrs, when using multihome
    one transport would be hashed many times with different saddrs, we
    would have to define new structure to link transport.
we do not need to do this:
1. as the changelog said, "it's not a normal case", in one host (client), it
shouldn't connect to the same server with different saddrs.
2. now as long as paddr+dport+lport are different, rhashtable can hash
it evenly.