[PATCH net-next] ipv6: do not check fib6_has_expires() in fib6_info_release()

[PATCH net-next] ipv6: do not check fib6_has_expires() in fib6_info_release()

[Eric Dumazet]

My prior patch went a bit too far, because apparently fib6_has_expires()
could be true while f6i->gc_link is not hashed yet.

fib6_set_expires_locked() can indeed set RTF_EXPIRES
while f6i->fib6_table is NULL.

Original syzbot reports were about corruptions caused
by dangling f6i->gc_link.

Fixes: 5a08d0065a91 ("ipv6: add debug checks in fib6_info_release()")
Reported-by: syzbot+c15aa445274af8674f41@syzkaller.appspotmail.com
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: David Ahern <dsahern@kernel.org>
Cc: Kui-Feng Lee <thinker.li@gmail.com>
---
 include/net/ip6_fib.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index e1e7a894863a7891610ce5afb2034473cc208d3e..95ed495c3a4028457baf1503c367d2e7a6e14770 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -329,7 +329,6 @@ static inline bool fib6_info_hold_safe(struct fib6_info *f6i)
 static inline void fib6_info_release(struct fib6_info *f6i)
 {
 	if (f6i && refcount_dec_and_test(&f6i->fib6_ref)) {
-		DEBUG_NET_WARN_ON_ONCE(fib6_has_expires(f6i));
 		DEBUG_NET_WARN_ON_ONCE(!hlist_unhashed(&f6i->gc_link));
 		call_rcu(&f6i->rcu, fib6_info_destroy_rcu);
 	}
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH net-next] ipv6: do not check fib6_has_expires() in fib6_info_release()

[David Ahern]

On 12/7/23 1:13 PM, Eric Dumazet wrote:
> My prior patch went a bit too far, because apparently fib6_has_expires()
> could be true while f6i->gc_link is not hashed yet.

yes, and I got distracted by that stack trace and avoiding errors in the
create function. The diff I sent does not solve any corruption with list
since the newly allocated f6i is not linked (and gc-link is initialized).

Kui-Feng: no need to send that patch again since it is not really
changing anything. Let's see if the other warn on triggers.

> 
> fib6_set_expires_locked() can indeed set RTF_EXPIRES
> while f6i->fib6_table is NULL.
> 
> Original syzbot reports were about corruptions caused
> by dangling f6i->gc_link.
> 
> Fixes: 5a08d0065a91 ("ipv6: add debug checks in fib6_info_release()")
> Reported-by: syzbot+c15aa445274af8674f41@syzkaller.appspotmail.com
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Ahern <dsahern@kernel.org>
> Cc: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  include/net/ip6_fib.h | 1 -
>  1 file changed, 1 deletion(-)
> 


Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next] ipv6: do not check fib6_has_expires() in fib6_info_release()

[Kui-Feng Lee]

On 12/7/23 12:25, David Ahern wrote:
> On 12/7/23 1:13 PM, Eric Dumazet wrote:
>> My prior patch went a bit too far, because apparently fib6_has_expires()
>> could be true while f6i->gc_link is not hashed yet.
> 
> yes, and I got distracted by that stack trace and avoiding errors in the
> create function. The diff I sent does not solve any corruption with list
> since the newly allocated f6i is not linked (and gc-link is initialized).
> 
> Kui-Feng: no need to send that patch again since it is not really
> changing anything. Let's see if the other warn on triggers.

Got it!

> 
>>
>> fib6_set_expires_locked() can indeed set RTF_EXPIRES
>> while f6i->fib6_table is NULL.
>>
>> Original syzbot reports were about corruptions caused
>> by dangling f6i->gc_link.
>>
>> Fixes: 5a08d0065a91 ("ipv6: add debug checks in fib6_info_release()")
>> Reported-by: syzbot+c15aa445274af8674f41@syzkaller.appspotmail.com
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Cc: David Ahern <dsahern@kernel.org>
>> Cc: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   include/net/ip6_fib.h | 1 -
>>   1 file changed, 1 deletion(-)
>>
> 
> 
> Reviewed-by: David Ahern <dsahern@kernel.org>
> 
> 

Re: [PATCH net-next] ipv6: do not check fib6_has_expires() in fib6_info_release()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  7 Dec 2023 20:13:22 +0000 you wrote:
> My prior patch went a bit too far, because apparently fib6_has_expires()
> could be true while f6i->gc_link is not hashed yet.
> 
> fib6_set_expires_locked() can indeed set RTF_EXPIRES
> while f6i->fib6_table is NULL.
> 
> Original syzbot reports were about corruptions caused
> by dangling f6i->gc_link.
> 
> [...]

Here is the summary with links:
  - [net-next] ipv6: do not check fib6_has_expires() in fib6_info_release()
    https://git.kernel.org/netdev/net-next/c/a3c205d0560f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html