netdev - Re: [PATCH net-next 3/3] ipv6: obsolete cached dst when removing them from fib tree

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:   Wed, 18 Oct 2017 10:56:39 -0700
From:   Wei Wang <weiwan@...gle.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        Martin KaFai Lau <kafai@...com>,
        Xin Long <lucien.xin@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Hannes Frederic Sowa <hannes@...essinduktion.org>
Subject: Re: [PATCH net-next 3/3] ipv6: obsolete cached dst when removing them
 from fib tree

On Wed, Oct 18, 2017 at 6:03 AM, Paolo Abeni <pabeni@...hat.com> wrote:
> On Tue, 2017-10-17 at 13:48 -0700, Wei Wang wrote:
>> On Tue, Oct 17, 2017 at 1:02 PM, Paolo Abeni <pabeni@...hat.com> wrote:
>> > Meanwhile others sockets may grab more references to (and use) the same
>> > aged-out dst.
>> >
>>
>> I don't think other sockets could grab more reference to this dst
>> because this dst should already be removed from the fib6 tree.
>
> With the current net-next code, the dst is not removed from the fib
> tree while someone else is holding it and dst_check() does not fail
> after that the cached dst is aged out. If a socket cache grab a
> reference to the CACHE dst, it will not release it untill the next
> sernum change, regardless of the dst aging.
>
>> > The commit 1e2ea8ad37be ("ipv6: set dst.obsolete when a cached route
>> > has expired") was the solution to the above issue prior to the recent
>> > refactor.
>> >
>>
>> I don't really understand how this commit is solving the above issue.
>> This commit still only ages out cached route if &rt->dst.__refcnt ==
>> 1. So if socket is holding refcnt to this dst and dst_check() is not
>> getting called,  this cached route still won't get deleted.
>
> Setting obsolete to DST_OBSOLETE_KILL forced whoever was holding the
> dst reference to drop it on the next dst_check(), so that refcnt could
> go down.
>

Yes. Understood.
Martin and I had a discussion yesterday. We both think it is not a
good idea to set obolete to DST_OBSOLETE_KILL but not to remove it
from the fib6 tree.
It is because others who do a route lookup later might potentially
find this route and tries to use this route. However, dst_check() will
show this route is invalid. So the user will redo the route lookup.
But as this route is not yet deleted from the tree, it will find this
route again. This seems like a bad situation.
And again, setting obsolete to DST_OBSOLETE_KILL does not prevent some
idle socket holding on to this dst for a long time...

With the above said, I am now convinced what you have in your patch is
the correct thing to do. Just remove the cached route without checking
the refcnt when it is aged.

> Cheers,
>
> Paolo