netdev - Re: [PATCH net] ipv6: remove incorrect WARN_ON() in fib6

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]

Date:   Wed, 27 Sep 2017 14:08:46 -0700
From:   Wei Wang <weiwan@...gle.com>
To:     Eric Dumazet <edumazet@...gle.com>,
        Martin KaFai Lau <kafai@...com>,
        David Miller <davem@...emloft.net>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net] ipv6: remove incorrect WARN_ON() in fib6_del()

On Tue, Sep 26, 2017 at 6:20 AM, Eric Dumazet <edumazet@...gle.com> wrote:
> On Mon, Sep 25, 2017 at 10:52 PM, Wei Wang <weiwan@...gle.com> wrote:
>> On Mon, Sep 25, 2017 at 7:23 PM, Eric Dumazet <edumazet@...gle.com> wrote:
>>> On Mon, Sep 25, 2017 at 7:07 PM, Martin KaFai Lau <kafai@...com> wrote:
>>>
>>>> I am probably still missing something.
>>>>
>>>> Considering the del operation should be under the writer lock,
>>>> if rt->rt6i_node should be NULL (for rt that has already been
>>>> removed from fib6), why this WARN_ON() is triggered?
>>>>
>>>> An example may help.
>>>>
>>>
>>> Look at the stack trace, you'll find the answers...
>>>
>>> ip6_link_failure() -> ip6_del_rt()
>>>
>>> Note that rt might have been deleted from the _tree_ already.
>>
>> Had a brief talk with Martin.
>> He has a valid point.
>> The current WARN_ON() code is as follows:
>> #if RT6_DEBUG >= 2
>>        if (rt->dst.obsolete > 0) {
>>                WARN_ON(fn);
>>                return -ENOENT;
>>        }
>> #endif
>>
>> The WARN_ON() only triggers when fn is not NULL. (I missed it before.)
>> In theory, fib6_del() calls fib6_del_route() which should set
>> rt->rt6i_node to NULL and rt->dst.obsolete to DST_OBSOLETE_DEAD within
>> the same write_lock session.
>> If those 2 values are inconsistent, it indicates something is wrong.
>> Will need more time to root cause the issue.
>>
>> Please ignore this patch. Sorry about the confusion.
>
> Oh well, for some reason I was seeing WARN_ON(1)  here, since this is
> a construct I often add in my tests ...

Just an update on this issue:
This WARNING issue should already be fixed by commit
7483cea79957312e9f8e9cf760a1bc5d6c507113:
Author: Ido Schimmel <idosch@...lanox.com>
Date:   Thu Aug 3 13:28:22 2017 +0200

    ipv6: fib: Unlink replaced routes from their nodes

    When a route is deleted its node pointer is set to NULL to indicate it's
    no longer linked to its node. Do the same for routes that are replaced.

    This will later allow us to test if a route is still in the FIB by
    checking its node pointer instead of its reference count.

    Signed-off-by: Ido Schimmel <idosch@...lanox.com>
    Signed-off-by: Jiri Pirko <jiri@...lanox.com>
    Signed-off-by: David S. Miller <davem@...emloft.net>

So no further action is needed on this.

Thanks.
Wei