| Message ID | 20170925173522.99892-1-tracywwnj@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] ipv6: remove incorrect WARN_ON() in fib6_del() | expand |
On Mon, Sep 25, 2017 at 05:35:22PM +0000, Wei Wang wrote: > From: Wei Wang <weiwan@google.com> > > fib6_del() generates WARN_ON() when rt->dst.obsolete > 0. This does not > make sense because it is possible that the route passed in is already > deleted by some other thread and rt->dst.obsolete is set to > DST_OBSOLETE_DEAD. > So this commit deletes this WARN_ON() and also remove the > "#ifdef RT6_DEBUG >= 2" condition so that if the route is already > obsolete, we return right at the beginning of fib6_del(). > > > diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c > index e5308d7cbd75..693bcd7ef6d2 100644 > --- a/net/ipv6/ip6_fib.c > +++ b/net/ipv6/ip6_fib.c > @@ -1592,13 +1592,7 @@ int fib6_del(struct rt6_info *rt, struct nl_info *info) > struct net *net = info->nl_net; > struct rt6_info **rtp; > > -#if RT6_DEBUG >= 2 > - if (rt->dst.obsolete > 0) { > - WARN_ON(fn); fn should have already been set to NULL if it is removed from the fib6 tree? > - return -ENOENT; > - } > -#endif > - if (!fn || rt == net->ipv6.ip6_null_entry) > + if (!fn || rt->dst.obsolete > 0 || rt == net->ipv6.ip6_null_entry) > return -ENOENT; > > WARN_ON(!(fn->fn_flags & RTN_RTINFO)); > -- > 2.14.1.821.g8fa685d3b7-goog >
On Mon, Sep 25, 2017 at 5:56 PM, Martin KaFai Lau <kafai@fb.com> wrote: > On Mon, Sep 25, 2017 at 05:35:22PM +0000, Wei Wang wrote: >> From: Wei Wang <weiwan@google.com> >> >> fib6_del() generates WARN_ON() when rt->dst.obsolete > 0. This does not >> make sense because it is possible that the route passed in is already >> deleted by some other thread and rt->dst.obsolete is set to >> DST_OBSOLETE_DEAD. >> So this commit deletes this WARN_ON() and also remove the >> "#ifdef RT6_DEBUG >= 2" condition so that if the route is already >> obsolete, we return right at the beginning of fib6_del(). >> >> >> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c >> index e5308d7cbd75..693bcd7ef6d2 100644 >> --- a/net/ipv6/ip6_fib.c >> +++ b/net/ipv6/ip6_fib.c >> @@ -1592,13 +1592,7 @@ int fib6_del(struct rt6_info *rt, struct nl_info *info) >> struct net *net = info->nl_net; >> struct rt6_info **rtp; >> >> -#if RT6_DEBUG >= 2 >> - if (rt->dst.obsolete > 0) { >> - WARN_ON(fn); > fn should have already been set to NULL if it is removed > from the fib6 tree? > That is true. rt->rt6i_node (fn) should already be marked as NULL. That means the check on rt->dst.obsolete is redundant. I will remove it in v2. Thanks Martin. >> - return -ENOENT; >> - } >> -#endif >> - if (!fn || rt == net->ipv6.ip6_null_entry) >> + if (!fn || rt->dst.obsolete > 0 || rt == net->ipv6.ip6_null_entry) >> return -ENOENT; >> >> WARN_ON(!(fn->fn_flags & RTN_RTINFO)); >> -- >> 2.14.1.821.g8fa685d3b7-goog >>
On Tue, Sep 26, 2017 at 01:16:05AM +0000, Wei Wang wrote: > On Mon, Sep 25, 2017 at 5:56 PM, Martin KaFai Lau <kafai@fb.com> wrote: > > On Mon, Sep 25, 2017 at 05:35:22PM +0000, Wei Wang wrote: > >> From: Wei Wang <weiwan@google.com> > >> > >> fib6_del() generates WARN_ON() when rt->dst.obsolete > 0. This does not > >> make sense because it is possible that the route passed in is already > >> deleted by some other thread and rt->dst.obsolete is set to > >> DST_OBSOLETE_DEAD. > >> So this commit deletes this WARN_ON() and also remove the > >> "#ifdef RT6_DEBUG >= 2" condition so that if the route is already > >> obsolete, we return right at the beginning of fib6_del(). > >> > >> > >> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c > >> index e5308d7cbd75..693bcd7ef6d2 100644 > >> --- a/net/ipv6/ip6_fib.c > >> +++ b/net/ipv6/ip6_fib.c > >> @@ -1592,13 +1592,7 @@ int fib6_del(struct rt6_info *rt, struct nl_info *info) > >> struct net *net = info->nl_net; > >> struct rt6_info **rtp; > >> > >> -#if RT6_DEBUG >= 2 > >> - if (rt->dst.obsolete > 0) { > >> - WARN_ON(fn); > > fn should have already been set to NULL if it is removed > > from the fib6 tree? > > > > That is true. rt->rt6i_node (fn) should already be marked as NULL. 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. > That means the check on rt->dst.obsolete is redundant. > I will remove it in v2. > Thanks Martin. > > > >> - return -ENOENT; > >> - } > >> -#endif > >> - if (!fn || rt == net->ipv6.ip6_null_entry) > >> + if (!fn || rt->dst.obsolete > 0 || rt == net->ipv6.ip6_null_entry) > >> return -ENOENT; > >> > >> WARN_ON(!(fn->fn_flags & RTN_RTINFO)); > >> -- > >> 2.14.1.821.g8fa685d3b7-goog > >>
On Mon, Sep 25, 2017 at 7:07 PM, Martin KaFai Lau <kafai@fb.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.
On Mon, Sep 25, 2017 at 7:23 PM, Eric Dumazet <edumazet@google.com> wrote: > On Mon, Sep 25, 2017 at 7:07 PM, Martin KaFai Lau <kafai@fb.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.
On Mon, Sep 25, 2017 at 10:52 PM, Wei Wang <weiwan@google.com> wrote: > On Mon, Sep 25, 2017 at 7:23 PM, Eric Dumazet <edumazet@google.com> wrote: >> On Mon, Sep 25, 2017 at 7:07 PM, Martin KaFai Lau <kafai@fb.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 ...
On Tue, Sep 26, 2017 at 6:20 AM, Eric Dumazet <edumazet@google.com> wrote: > On Mon, Sep 25, 2017 at 10:52 PM, Wei Wang <weiwan@google.com> wrote: >> On Mon, Sep 25, 2017 at 7:23 PM, Eric Dumazet <edumazet@google.com> wrote: >>> On Mon, Sep 25, 2017 at 7:07 PM, Martin KaFai Lau <kafai@fb.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@mellanox.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@mellanox.com> Signed-off-by: Jiri Pirko <jiri@mellanox.com> Signed-off-by: David S. Miller <davem@davemloft.net> So no further action is needed on this. Thanks. Wei
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c index e5308d7cbd75..693bcd7ef6d2 100644 --- a/net/ipv6/ip6_fib.c +++ b/net/ipv6/ip6_fib.c @@ -1592,13 +1592,7 @@ int fib6_del(struct rt6_info *rt, struct nl_info *info) struct net *net = info->nl_net; struct rt6_info **rtp; -#if RT6_DEBUG >= 2 - if (rt->dst.obsolete > 0) { - WARN_ON(fn); - return -ENOENT; - } -#endif - if (!fn || rt == net->ipv6.ip6_null_entry) + if (!fn || rt->dst.obsolete > 0 || rt == net->ipv6.ip6_null_entry) return -ENOENT; WARN_ON(!(fn->fn_flags & RTN_RTINFO));