netdev - Re: [PATCH net] ipmr: remove cache_resolve_queue

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]

Date:   Tue, 3 Sep 2019 16:18:41 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org, Phil Karn <karn@...q.net>,
        Sukumar Gopalakrishnan <sukumarg1973@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH net] ipmr: remove cache_resolve_queue_len

On 03/09/2019 15:55, Hangbin Liu wrote:
> Hi Nikolay,
> 
> Thanks for the feedback, see comments below.
> 
> On Tue, Sep 03, 2019 at 12:15:34PM +0300, Nikolay Aleksandrov wrote:
>> On 03/09/2019 11:43, Hangbin Liu wrote:
>>> This is a re-post of previous patch wrote by David Miller[1].
>>>
>>> Phil Karn reported[2] that on busy networks with lots of unresolved
>>> multicast routing entries, the creation of new multicast group routes
>>> can be extremely slow and unreliable.
>>>
>>> The reason is we hard-coded multicast route entries with unresolved source
>>> addresses(cache_resolve_queue_len) to 10. If some multicast route never
>>> resolves and the unresolved source addresses increased, there will
>>> be no ability to create new multicast route cache.
>>>
>>> To resolve this issue, we need either add a sysctl entry to make the
>>> cache_resolve_queue_len configurable, or just remove cache_resolve_queue_len
>>> directly, as we already have the socket receive queue limits of mrouted
>>> socket, pointed by David.
>>>
>>> From my side, I'd perfer to remove the cache_resolve_queue_len instead
>>> of creating two more(IPv4 and IPv6 version) sysctl entry.
>>>
>>> [1] https://lkml.org/lkml/2018/7/22/11
>>> [2] https://lkml.org/lkml/2018/7/21/343
>>>
>>> Reported-by: Phil Karn <karn@...q.net>
>>> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
>>> ---
>>>  include/linux/mroute_base.h |  2 --
>>>  net/ipv4/ipmr.c             | 27 ++++++++++++++++++---------
>>>  net/ipv6/ip6mr.c            | 10 +++-------
>>>  3 files changed, 21 insertions(+), 18 deletions(-)
>>>
>>
>> Hi,
>> IMO this is definitely net-next material. A few more comments below.
> 
> I thoug this is a bug fix. But it looks more suitable to net-next as you said.
>>
>> Note that hosts will automatically have this limit lifted to about 270
>> entries with current defaults, some might be surprised if they have
>> higher limits set and could be left with queues allowing for thousands
>> of entries in a linked list.
> 
> I think this is just a cache list and should be expired soon. The cache
> creation would also failed if there is no buffer.
> 
> But if you like, I can write a patch with sysctl parameter.

I wasn't suggesting the sysctl, I'm actually against it as I've said before when
commenting these patches. Just wanted to note that some setups will end up with
the possibility of having thousands of entries in a linked list after updating. Unless
we change the data structure I guess we just have to live with it, I wouldn't want to
add a sysctl or another hard limit that will have to be removed after a while. In the
long run we might update the algorithm used for unres entries.

So to make it clear: I'm fine with this patch as it is, definitely an improvement IMO.

>>
>>> +static int queue_count(struct mr_table *mrt)
>>> +{
>>> +	struct list_head *pos;
>>> +	int count = 0;
>>> +
>>> +	list_for_each(pos, &mrt->mfc_unres_queue)
>>> +		count++;
>>> +	return count;
>>> +}
>>
>> I don't think we hold the mfc_unres_lock here while walking
>> the unresolved list below in ipmr_fill_table().
> 
> ah, yes, I will fix this.
> 
> Thanks
> Hangbin
>