[PATCH net-next] ipv6: add debug checks in fib6_info_release()

[PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Eric Dumazet]

Some elusive syzbot reports are hinting to fib6_info_release(),
with a potential dangling f6i->gc_link anchor.

Add debug checks so that syzbot can catch the issue earlier eventually.

BUG: KASAN: slab-use-after-free in __hlist_del include/linux/list.h:990 [inline]
BUG: KASAN: slab-use-after-free in hlist_del_init include/linux/list.h:1016 [inline]
BUG: KASAN: slab-use-after-free in fib6_clean_expires_locked include/net/ip6_fib.h:533 [inline]
BUG: KASAN: slab-use-after-free in fib6_purge_rt+0x986/0x9c0 net/ipv6/ip6_fib.c:1064
Write of size 8 at addr ffff88802805a840 by task syz-executor.1/10057

CPU: 1 PID: 10057 Comm: syz-executor.1 Not tainted 6.7.0-rc2-syzkaller-00029-g9b6de136b5f0 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/10/2023
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xd9/0x1b0 lib/dump_stack.c:106
print_address_description mm/kasan/report.c:364 [inline]
print_report+0xc4/0x620 mm/kasan/report.c:475
kasan_report+0xda/0x110 mm/kasan/report.c:588
__hlist_del include/linux/list.h:990 [inline]
hlist_del_init include/linux/list.h:1016 [inline]
fib6_clean_expires_locked include/net/ip6_fib.h:533 [inline]
fib6_purge_rt+0x986/0x9c0 net/ipv6/ip6_fib.c:1064
fib6_del_route net/ipv6/ip6_fib.c:1993 [inline]
fib6_del+0xa7a/0x1750 net/ipv6/ip6_fib.c:2038
__ip6_del_rt net/ipv6/route.c:3866 [inline]
ip6_del_rt+0xf7/0x200 net/ipv6/route.c:3881
ndisc_router_discovery+0x295b/0x3560 net/ipv6/ndisc.c:1372
ndisc_rcv+0x3de/0x5f0 net/ipv6/ndisc.c:1856
icmpv6_rcv+0x1470/0x19c0 net/ipv6/icmp.c:979
ip6_protocol_deliver_rcu+0x170/0x13e0 net/ipv6/ip6_input.c:438
ip6_input_finish+0x14f/0x2f0 net/ipv6/ip6_input.c:483
NF_HOOK include/linux/netfilter.h:314 [inline]
NF_HOOK include/linux/netfilter.h:308 [inline]
ip6_input+0xa1/0xc0 net/ipv6/ip6_input.c:492
ip6_mc_input+0x48b/0xf40 net/ipv6/ip6_input.c:586
dst_input include/net/dst.h:461 [inline]
ip6_rcv_finish net/ipv6/ip6_input.c:79 [inline]
NF_HOOK include/linux/netfilter.h:314 [inline]
NF_HOOK include/linux/netfilter.h:308 [inline]
ipv6_rcv+0x24e/0x380 net/ipv6/ip6_input.c:310
__netif_receive_skb_one_core+0x115/0x180 net/core/dev.c:5529
__netif_receive_skb+0x1f/0x1b0 net/core/dev.c:5643
netif_receive_skb_internal net/core/dev.c:5729 [inline]
netif_receive_skb+0x133/0x700 net/core/dev.c:5788
tun_rx_batched+0x429/0x780 drivers/net/tun.c:1579
tun_get_user+0x29e3/0x3bc0 drivers/net/tun.c:2002
tun_chr_write_iter+0xe8/0x210 drivers/net/tun.c:2048
call_write_iter include/linux/fs.h:2020 [inline]
new_sync_write fs/read_write.c:491 [inline]
vfs_write+0x64f/0xdf0 fs/read_write.c:584
ksys_write+0x12f/0x250 fs/read_write.c:637
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b
RIP: 0033:0x7f38e387b82f
Code: 89 54 24 18 48 89 74 24 10 89 7c 24 08 e8 b9 80 02 00 48 8b 54 24 18 48 8b 74 24 10 41 89 c0 8b 7c 24 08 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 31 44 89 c7 48 89 44 24 08 e8 0c 81 02 00 48
RSP: 002b:00007f38e45c9090 EFLAGS: 00000293 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007f38e399bf80 RCX: 00007f38e387b82f
RDX: 00000000000003b6 RSI: 0000000020000680 RDI: 00000000000000c8
RBP: 00007f38e38c847a R08: 0000000000000000 R09: 0000000000000000
R10: 00000000000003b6 R11: 0000000000000293 R12: 0000000000000000
R13: 000000000000000b R14: 00007f38e399bf80 R15: 00007f38e3abfa48
</TASK>

Allocated by task 10044:
kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
kasan_set_track+0x25/0x30 mm/kasan/common.c:52
____kasan_kmalloc mm/kasan/common.c:374 [inline]
__kasan_kmalloc+0xa2/0xb0 mm/kasan/common.c:383
kasan_kmalloc include/linux/kasan.h:198 [inline]
__do_kmalloc_node mm/slab_common.c:1007 [inline]
__kmalloc+0x59/0x90 mm/slab_common.c:1020
kmalloc include/linux/slab.h:604 [inline]
kzalloc include/linux/slab.h:721 [inline]
fib6_info_alloc+0x40/0x160 net/ipv6/ip6_fib.c:155
ip6_route_info_create+0x337/0x1e70 net/ipv6/route.c:3749
ip6_route_add+0x26/0x150 net/ipv6/route.c:3843
rt6_add_route_info+0x2e7/0x4b0 net/ipv6/route.c:4316
rt6_route_rcv+0x76c/0xbf0 net/ipv6/route.c:985
ndisc_router_discovery+0x138b/0x3560 net/ipv6/ndisc.c:1529
ndisc_rcv+0x3de/0x5f0 net/ipv6/ndisc.c:1856
icmpv6_rcv+0x1470/0x19c0 net/ipv6/icmp.c:979
ip6_protocol_deliver_rcu+0x170/0x13e0 net/ipv6/ip6_input.c:438
ip6_input_finish+0x14f/0x2f0 net/ipv6/ip6_input.c:483
NF_HOOK include/linux/netfilter.h:314 [inline]
NF_HOOK include/linux/netfilter.h:308 [inline]
ip6_input+0xa1/0xc0 net/ipv6/ip6_input.c:492
ip6_mc_input+0x48b/0xf40 net/ipv6/ip6_input.c:586
dst_input include/net/dst.h:461 [inline]
ip6_rcv_finish net/ipv6/ip6_input.c:79 [inline]
NF_HOOK include/linux/netfilter.h:314 [inline]
NF_HOOK include/linux/netfilter.h:308 [inline]
ipv6_rcv+0x24e/0x380 net/ipv6/ip6_input.c:310
__netif_receive_skb_one_core+0x115/0x180 net/core/dev.c:5529
__netif_receive_skb+0x1f/0x1b0 net/core/dev.c:5643
netif_receive_skb_internal net/core/dev.c:5729 [inline]
netif_receive_skb+0x133/0x700 net/core/dev.c:5788
tun_rx_batched+0x429/0x780 drivers/net/tun.c:1579
tun_get_user+0x29e3/0x3bc0 drivers/net/tun.c:2002
tun_chr_write_iter+0xe8/0x210 drivers/net/tun.c:2048
call_write_iter include/linux/fs.h:2020 [inline]
new_sync_write fs/read_write.c:491 [inline]
vfs_write+0x64f/0xdf0 fs/read_write.c:584
ksys_write+0x12f/0x250 fs/read_write.c:637
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b

Freed by task 5123:
kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
kasan_set_track+0x25/0x30 mm/kasan/common.c:52
kasan_save_free_info+0x2b/0x40 mm/kasan/generic.c:522
____kasan_slab_free mm/kasan/common.c:236 [inline]
____kasan_slab_free+0x15b/0x1b0 mm/kasan/common.c:200
kasan_slab_free include/linux/kasan.h:164 [inline]
slab_free_hook mm/slub.c:1800 [inline]
slab_free_freelist_hook+0x114/0x1e0 mm/slub.c:1826
slab_free mm/slub.c:3809 [inline]
__kmem_cache_free+0xc0/0x180 mm/slub.c:3822
rcu_do_batch kernel/rcu/tree.c:2158 [inline]
rcu_core+0x819/0x1680 kernel/rcu/tree.c:2431
__do_softirq+0x21a/0x8de kernel/softirq.c:553

Last potentially related work creation:
kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
__kasan_record_aux_stack+0xbc/0xd0 mm/kasan/generic.c:492
__call_rcu_common.constprop.0+0x9a/0x7a0 kernel/rcu/tree.c:2681
fib6_info_release include/net/ip6_fib.h:332 [inline]
fib6_info_release include/net/ip6_fib.h:329 [inline]
rt6_route_rcv+0xa4e/0xbf0 net/ipv6/route.c:997
ndisc_router_discovery+0x138b/0x3560 net/ipv6/ndisc.c:1529
ndisc_rcv+0x3de/0x5f0 net/ipv6/ndisc.c:1856
icmpv6_rcv+0x1470/0x19c0 net/ipv6/icmp.c:979
ip6_protocol_deliver_rcu+0x170/0x13e0 net/ipv6/ip6_input.c:438
ip6_input_finish+0x14f/0x2f0 net/ipv6/ip6_input.c:483
NF_HOOK include/linux/netfilter.h:314 [inline]
NF_HOOK include/linux/netfilter.h:308 [inline]
ip6_input+0xa1/0xc0 net/ipv6/ip6_input.c:492
ip6_mc_input+0x48b/0xf40 net/ipv6/ip6_input.c:586
dst_input include/net/dst.h:461 [inline]
ip6_rcv_finish net/ipv6/ip6_input.c:79 [inline]
NF_HOOK include/linux/netfilter.h:314 [inline]
NF_HOOK include/linux/netfilter.h:308 [inline]
ipv6_rcv+0x24e/0x380 net/ipv6/ip6_input.c:310
__netif_receive_skb_one_core+0x115/0x180 net/core/dev.c:5529
__netif_receive_skb+0x1f/0x1b0 net/core/dev.c:5643
netif_receive_skb_internal net/core/dev.c:5729 [inline]
netif_receive_skb+0x133/0x700 net/core/dev.c:5788
tun_rx_batched+0x429/0x780 drivers/net/tun.c:1579
tun_get_user+0x29e3/0x3bc0 drivers/net/tun.c:2002
tun_chr_write_iter+0xe8/0x210 drivers/net/tun.c:2048
call_write_iter include/linux/fs.h:2020 [inline]
new_sync_write fs/read_write.c:491 [inline]
vfs_write+0x64f/0xdf0 fs/read_write.c:584
ksys_write+0x12f/0x250 fs/read_write.c:637
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b

Second to last potentially related work creation:
kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
__kasan_record_aux_stack+0xbc/0xd0 mm/kasan/generic.c:492
insert_work+0x38/0x230 kernel/workqueue.c:1647
__queue_work+0xcdc/0x11f0 kernel/workqueue.c:1803
call_timer_fn+0x193/0x590 kernel/time/timer.c:1700
expire_timers kernel/time/timer.c:1746 [inline]
__run_timers+0x585/0xb20 kernel/time/timer.c:2022
run_timer_softirq+0x58/0xd0 kernel/time/timer.c:2035
__do_softirq+0x21a/0x8de kernel/softirq.c:553

The buggy address belongs to the object at ffff88802805a800
which belongs to the cache kmalloc-512 of size 512
The buggy address is located 64 bytes inside of
freed 512-byte region [ffff88802805a800, ffff88802805aa00)

The buggy address belongs to the physical page:
page:ffffea0000a01600 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x28058
head:ffffea0000a01600 order:2 entire_mapcount:0 nr_pages_mapped:0 pincount:0
flags: 0xfff00000000840(slab|head|node=0|zone=1|lastcpupid=0x7ff)
page_type: 0xffffffff()
raw: 00fff00000000840 ffff888013041c80 ffffea0001e02600 dead000000000002
raw: 0000000000000000 0000000000100010 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 2, migratetype Unmovable, gfp_mask 0x1d20c0(__GFP_IO|__GFP_FS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC|__GFP_HARDWALL), pid 18706, tgid 18699 (syz-executor.2), ts 999991973280, free_ts 996884464281
set_page_owner include/linux/page_owner.h:31 [inline]
post_alloc_hook+0x2d0/0x350 mm/page_alloc.c:1537
prep_new_page mm/page_alloc.c:1544 [inline]
get_page_from_freelist+0xa25/0x36d0 mm/page_alloc.c:3312
__alloc_pages+0x22e/0x2420 mm/page_alloc.c:4568
alloc_pages_mpol+0x258/0x5f0 mm/mempolicy.c:2133
alloc_slab_page mm/slub.c:1870 [inline]
allocate_slab mm/slub.c:2017 [inline]
new_slab+0x283/0x3c0 mm/slub.c:2070
___slab_alloc+0x979/0x1500 mm/slub.c:3223
__slab_alloc.constprop.0+0x56/0xa0 mm/slub.c:3322
__slab_alloc_node mm/slub.c:3375 [inline]
slab_alloc_node mm/slub.c:3468 [inline]
__kmem_cache_alloc_node+0x131/0x310 mm/slub.c:3517
__do_kmalloc_node mm/slab_common.c:1006 [inline]
__kmalloc+0x49/0x90 mm/slab_common.c:1020
kmalloc include/linux/slab.h:604 [inline]
kzalloc include/linux/slab.h:721 [inline]
copy_splice_read+0x1ac/0x8f0 fs/splice.c:338
vfs_splice_read fs/splice.c:992 [inline]
vfs_splice_read+0x2ea/0x3b0 fs/splice.c:962
splice_direct_to_actor+0x2a5/0xa30 fs/splice.c:1069
do_splice_direct+0x1af/0x280 fs/splice.c:1194
do_sendfile+0xb3e/0x1310 fs/read_write.c:1254
__do_sys_sendfile64 fs/read_write.c:1322 [inline]
__se_sys_sendfile64 fs/read_write.c:1308 [inline]
__x64_sys_sendfile64+0x1d6/0x220 fs/read_write.c:1308
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
page last free stack trace:
reset_page_owner include/linux/page_owner.h:24 [inline]
free_pages_prepare mm/page_alloc.c:1137 [inline]
free_unref_page_prepare+0x4fa/0xaa0 mm/page_alloc.c:2347
free_unref_page_list+0xe6/0xb40 mm/page_alloc.c:2533
release_pages+0x32a/0x14f0 mm/swap.c:1042
tlb_batch_pages_flush+0x9a/0x190 mm/mmu_gather.c:98
tlb_flush_mmu_free mm/mmu_gather.c:293 [inline]
tlb_flush_mmu mm/mmu_gather.c:300 [inline]
tlb_finish_mmu+0x14b/0x6f0 mm/mmu_gather.c:392
exit_mmap+0x38b/0xa70 mm/mmap.c:3321
__mmput+0x12a/0x4d0 kernel/fork.c:1349
mmput+0x62/0x70 kernel/fork.c:1371
exit_mm kernel/exit.c:567 [inline]
do_exit+0x9ad/0x2ae0 kernel/exit.c:858
do_group_exit+0xd4/0x2a0 kernel/exit.c:1021
get_signal+0x23be/0x2790 kernel/signal.c:2904
arch_do_signal_or_restart+0x90/0x7f0 arch/x86/kernel/signal.c:309
exit_to_user_mode_loop kernel/entry/common.c:168 [inline]
exit_to_user_mode_prepare+0x121/0x240 kernel/entry/common.c:204
irqentry_exit_to_user_mode+0xa/0x40 kernel/entry/common.c:309
asm_sysvec_apic_timer_interrupt+0x1a/0x20 arch/x86/include/asm/idtentry.h:645

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/ip6_fib.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 1ba9f4ddf2f6db6c6ebf0a0675ca74fea2273fd9..e1e7a894863a7891610ce5afb2034473cc208d3e 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -328,8 +328,11 @@ 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))
+	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);
+	}
 }
 
 enum fib6_walk_state {
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[David Ahern]

On 12/5/23 10:32 AM, Eric Dumazet wrote:
> Some elusive syzbot reports are hinting to fib6_info_release(),
> with a potential dangling f6i->gc_link anchor.
> 
> Add debug checks so that syzbot can catch the issue earlier eventually.
> 
> BUG: KASAN: slab-use-after-free in __hlist_del include/linux/list.h:990 [inline]
> BUG: KASAN: slab-use-after-free in hlist_del_init include/linux/list.h:1016 [inline]
> BUG: KASAN: slab-use-after-free in fib6_clean_expires_locked include/net/ip6_fib.h:533 [inline]
> BUG: KASAN: slab-use-after-free in fib6_purge_rt+0x986/0x9c0 net/ipv6/ip6_fib.c:1064
> Write of size 8 at addr ffff88802805a840 by task syz-executor.1/10057
> 

...

> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/net/ip6_fib.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

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

Re: [PATCH net-next] ipv6: add debug checks 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 Tue,  5 Dec 2023 17:32:50 +0000 you wrote:
> Some elusive syzbot reports are hinting to fib6_info_release(),
> with a potential dangling f6i->gc_link anchor.
> 
> Add debug checks so that syzbot can catch the issue earlier eventually.
> 
> BUG: KASAN: slab-use-after-free in __hlist_del include/linux/list.h:990 [inline]
> BUG: KASAN: slab-use-after-free in hlist_del_init include/linux/list.h:1016 [inline]
> BUG: KASAN: slab-use-after-free in fib6_clean_expires_locked include/net/ip6_fib.h:533 [inline]
> BUG: KASAN: slab-use-after-free in fib6_purge_rt+0x986/0x9c0 net/ipv6/ip6_fib.c:1064
> Write of size 8 at addr ffff88802805a840 by task syz-executor.1/10057
> 
> [...]

Here is the summary with links:
  - [net-next] ipv6: add debug checks in fib6_info_release()
    https://git.kernel.org/netdev/net-next/c/5a08d0065a91

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

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Eric Dumazet]

On Thu, Dec 7, 2023 at 4:10 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This patch was applied to netdev/net-next.git (main)
> by Jakub Kicinski <kuba@kernel.org>:
>
> On Tue,  5 Dec 2023 17:32:50 +0000 you wrote:
> > Some elusive syzbot reports are hinting to fib6_info_release(),
> > with a potential dangling f6i->gc_link anchor.
> >
> > Add debug checks so that syzbot can catch the issue earlier eventually.
> >
> > BUG: KASAN: slab-use-after-free in __hlist_del include/linux/list.h:990 [inline]
> > BUG: KASAN: slab-use-after-free in hlist_del_init include/linux/list.h:1016 [inline]
> > BUG: KASAN: slab-use-after-free in fib6_clean_expires_locked include/net/ip6_fib.h:533 [inline]
> > BUG: KASAN: slab-use-after-free in fib6_purge_rt+0x986/0x9c0 net/ipv6/ip6_fib.c:1064
> > Write of size 8 at addr ffff88802805a840 by task syz-executor.1/10057
> >
> > [...]
>
> Here is the summary with links:
>   - [net-next] ipv6: add debug checks in fib6_info_release()
>     https://git.kernel.org/netdev/net-next/c/5a08d0065a91

Nice, syzbot gave me exactly what I was looking for.

WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
fib6_info_release include/net/ip6_fib.h:332 [inline]
WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
ip6_route_info_create+0x1a1a/0x1f10 net/ipv6/route.c:3829
Modules linked in:
CPU: 0 PID: 5059 Comm: syz-executor256 Not tainted
6.7.0-rc3-syzkaller-00805-g5a08d0065a91 #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 11/10/2023
RIP: 0010:fib6_info_release include/net/ip6_fib.h:332 [inline]
RIP: 0010:ip6_route_info_create+0x1a1a/0x1f10 net/ipv6/route.c:3829
Code: 49 83 7f 40 00 75 28 e8 04 ae 50 f8 49 8d bf a0 00 00 00 48 c7
c6 c0 ae 37 89 e8 41 2c 3a f8 e9 65 f4 ff ff e8 e7 ad 50 f8 90 <0f> 0b
90 eb ad e8 dc ad 50 f8 90 0f 0b 90 eb cd e8 d1 ad 50 f8 e8
RSP: 0018:ffffc90003bdf8e0 EFLAGS: 00010293
RAX: 0000000000000000 RBX: 0000000000400000 RCX: ffffffff8936e418
RDX: ffff888026a58000 RSI: ffffffff8936e469 RDI: 0000000000000005
RBP: ffffc90003bdf9d0 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000000400000 R11: ffffffff81de4c35 R12: ffffffffffffffea
R13: ffff88802993242c R14: ffffc90003bdfac4 R15: ffff888029932400
FS: 00005555562b4380(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000004585c0 CR3: 000000007390d000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
ip6_route_add+0x26/0x1f0 net/ipv6/route.c:3843
ipv6_route_ioctl+0x3ff/0x590 net/ipv6/route.c:4467
inet6_ioctl+0x265/0x2b0 net/ipv6/af_inet6.c:575
sock_do_ioctl+0x113/0x270 net/socket.c:1220
sock_ioctl+0x22e/0x6b0 net/socket.c:1339
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:871 [inline]
__se_sys_ioctl fs/ioctl.c:857 [inline]
__x64_sys_ioctl+0x18f/0x210 fs/ioctl.c:857
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b
RIP: 0033:0x7f175790d369


Following commit seems buggy.

commit 3dec89b14d37ee635e772636dad3f09f78f1ab87
Author: Kui-Feng Lee <thinker.li@gmail.com>
Date:   Tue Aug 15 11:07:05 2023 -0700

    net/ipv6: Remove expired routes with a separated list of routes.

    FIB6 GC walks trees of fib6_tables to remove expired routes. Walking a tree
    can be expensive if the number of routes in a table is big, even if most of
    them are permanent. Checking routes in a separated list of routes having
    expiration will avoid this potential issue.

    Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
    Reviewed-by: David Ahern <dsahern@kernel.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Kui-Feng Lee]

On 12/7/23 05:29, Eric Dumazet wrote:
> On Thu, Dec 7, 2023 at 4:10 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>>
>> Hello:
>>
>> This patch was applied to netdev/net-next.git (main)
>> by Jakub Kicinski <kuba@kernel.org>:
>>
>> On Tue,  5 Dec 2023 17:32:50 +0000 you wrote:
>>> Some elusive syzbot reports are hinting to fib6_info_release(),
>>> with a potential dangling f6i->gc_link anchor.
>>>
>>> Add debug checks so that syzbot can catch the issue earlier eventually.
>>>
>>> BUG: KASAN: slab-use-after-free in __hlist_del include/linux/list.h:990 [inline]
>>> BUG: KASAN: slab-use-after-free in hlist_del_init include/linux/list.h:1016 [inline]
>>> BUG: KASAN: slab-use-after-free in fib6_clean_expires_locked include/net/ip6_fib.h:533 [inline]
>>> BUG: KASAN: slab-use-after-free in fib6_purge_rt+0x986/0x9c0 net/ipv6/ip6_fib.c:1064
>>> Write of size 8 at addr ffff88802805a840 by task syz-executor.1/10057
>>>
>>> [...]
>>
>> Here is the summary with links:
>>    - [net-next] ipv6: add debug checks in fib6_info_release()
>>      https://git.kernel.org/netdev/net-next/c/5a08d0065a91
> 
> Nice, syzbot gave me exactly what I was looking for.
> 
> WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
> fib6_info_release include/net/ip6_fib.h:332 [inline]
> WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
> ip6_route_info_create+0x1a1a/0x1f10 net/ipv6/route.c:3829
> Modules linked in:
> CPU: 0 PID: 5059 Comm: syz-executor256 Not tainted
> 6.7.0-rc3-syzkaller-00805-g5a08d0065a91 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine,
> BIOS Google 11/10/2023
> RIP: 0010:fib6_info_release include/net/ip6_fib.h:332 [inline]
> RIP: 0010:ip6_route_info_create+0x1a1a/0x1f10 net/ipv6/route.c:3829
> Code: 49 83 7f 40 00 75 28 e8 04 ae 50 f8 49 8d bf a0 00 00 00 48 c7
> c6 c0 ae 37 89 e8 41 2c 3a f8 e9 65 f4 ff ff e8 e7 ad 50 f8 90 <0f> 0b
> 90 eb ad e8 dc ad 50 f8 90 0f 0b 90 eb cd e8 d1 ad 50 f8 e8
> RSP: 0018:ffffc90003bdf8e0 EFLAGS: 00010293
> RAX: 0000000000000000 RBX: 0000000000400000 RCX: ffffffff8936e418
> RDX: ffff888026a58000 RSI: ffffffff8936e469 RDI: 0000000000000005
> RBP: ffffc90003bdf9d0 R08: 0000000000000005 R09: 0000000000000000
> R10: 0000000000400000 R11: ffffffff81de4c35 R12: ffffffffffffffea
> R13: ffff88802993242c R14: ffffc90003bdfac4 R15: ffff888029932400
> FS: 00005555562b4380(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000004585c0 CR3: 000000007390d000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> ip6_route_add+0x26/0x1f0 net/ipv6/route.c:3843
> ipv6_route_ioctl+0x3ff/0x590 net/ipv6/route.c:4467
> inet6_ioctl+0x265/0x2b0 net/ipv6/af_inet6.c:575
> sock_do_ioctl+0x113/0x270 net/socket.c:1220
> sock_ioctl+0x22e/0x6b0 net/socket.c:1339
> vfs_ioctl fs/ioctl.c:51 [inline]
> __do_sys_ioctl fs/ioctl.c:871 [inline]
> __se_sys_ioctl fs/ioctl.c:857 [inline]
> __x64_sys_ioctl+0x18f/0x210 fs/ioctl.c:857
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
> RIP: 0033:0x7f175790d369
> 
> 
> Following commit seems buggy.
> 
> commit 3dec89b14d37ee635e772636dad3f09f78f1ab87
> Author: Kui-Feng Lee <thinker.li@gmail.com>
> Date:   Tue Aug 15 11:07:05 2023 -0700
> 
>      net/ipv6: Remove expired routes with a separated list of routes.
> 
>      FIB6 GC walks trees of fib6_tables to remove expired routes. Walking a tree
>      can be expensive if the number of routes in a table is big, even if most of
>      them are permanent. Checking routes in a separated list of routes having
>      expiration will avoid this potential issue.
> 
>      Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>      Reviewed-by: David Ahern <dsahern@kernel.org>
>      Signed-off-by: David S. Miller <davem@davemloft.net>
> 


Thank you for raising my awareness!
I will look into it.

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Kui-Feng Lee]

On 12/7/23 05:29, Eric Dumazet wrote:
> On Thu, Dec 7, 2023 at 4:10 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>>
>> Hello:
>>
>> This patch was applied to netdev/net-next.git (main)
>> by Jakub Kicinski <kuba@kernel.org>:
>>
>> On Tue,  5 Dec 2023 17:32:50 +0000 you wrote:
>>> Some elusive syzbot reports are hinting to fib6_info_release(),
>>> with a potential dangling f6i->gc_link anchor.
>>>
>>> Add debug checks so that syzbot can catch the issue earlier eventually.
>>>
>>> BUG: KASAN: slab-use-after-free in __hlist_del include/linux/list.h:990 [inline]
>>> BUG: KASAN: slab-use-after-free in hlist_del_init include/linux/list.h:1016 [inline]
>>> BUG: KASAN: slab-use-after-free in fib6_clean_expires_locked include/net/ip6_fib.h:533 [inline]
>>> BUG: KASAN: slab-use-after-free in fib6_purge_rt+0x986/0x9c0 net/ipv6/ip6_fib.c:1064
>>> Write of size 8 at addr ffff88802805a840 by task syz-executor.1/10057
>>>
>>> [...]
>>
>> Here is the summary with links:
>>    - [net-next] ipv6: add debug checks in fib6_info_release()
>>      https://git.kernel.org/netdev/net-next/c/5a08d0065a91
> 
> Nice, syzbot gave me exactly what I was looking for.
> 
> WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
> fib6_info_release include/net/ip6_fib.h:332 [inline]
> WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
> ip6_route_info_create+0x1a1a/0x1f10 net/ipv6/route.c:3829
> Modules linked in:
> CPU: 0 PID: 5059 Comm: syz-executor256 Not tainted
> 6.7.0-rc3-syzkaller-00805-g5a08d0065a91 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine,
> BIOS Google 11/10/2023
> RIP: 0010:fib6_info_release include/net/ip6_fib.h:332 [inline]
> RIP: 0010:ip6_route_info_create+0x1a1a/0x1f10 net/ipv6/route.c:3829
> Code: 49 83 7f 40 00 75 28 e8 04 ae 50 f8 49 8d bf a0 00 00 00 48 c7
> c6 c0 ae 37 89 e8 41 2c 3a f8 e9 65 f4 ff ff e8 e7 ad 50 f8 90 <0f> 0b
> 90 eb ad e8 dc ad 50 f8 90 0f 0b 90 eb cd e8 d1 ad 50 f8 e8
> RSP: 0018:ffffc90003bdf8e0 EFLAGS: 00010293
> RAX: 0000000000000000 RBX: 0000000000400000 RCX: ffffffff8936e418
> RDX: ffff888026a58000 RSI: ffffffff8936e469 RDI: 0000000000000005
> RBP: ffffc90003bdf9d0 R08: 0000000000000005 R09: 0000000000000000
> R10: 0000000000400000 R11: ffffffff81de4c35 R12: ffffffffffffffea
> R13: ffff88802993242c R14: ffffc90003bdfac4 R15: ffff888029932400
> FS: 00005555562b4380(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000004585c0 CR3: 000000007390d000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> ip6_route_add+0x26/0x1f0 net/ipv6/route.c:3843
> ipv6_route_ioctl+0x3ff/0x590 net/ipv6/route.c:4467
> inet6_ioctl+0x265/0x2b0 net/ipv6/af_inet6.c:575
> sock_do_ioctl+0x113/0x270 net/socket.c:1220
> sock_ioctl+0x22e/0x6b0 net/socket.c:1339
> vfs_ioctl fs/ioctl.c:51 [inline]
> __do_sys_ioctl fs/ioctl.c:871 [inline]
> __se_sys_ioctl fs/ioctl.c:857 [inline]
> __x64_sys_ioctl+0x18f/0x210 fs/ioctl.c:857
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
> RIP: 0033:0x7f175790d369
> 
> 
> Following commit seems buggy.
> 
> commit 3dec89b14d37ee635e772636dad3f09f78f1ab87
> Author: Kui-Feng Lee <thinker.li@gmail.com>
> Date:   Tue Aug 15 11:07:05 2023 -0700
> 
>      net/ipv6: Remove expired routes with a separated list of routes.
> 
>      FIB6 GC walks trees of fib6_tables to remove expired routes. Walking a tree
>      can be expensive if the number of routes in a table is big, even if most of
>      them are permanent. Checking routes in a separated list of routes having
>      expiration will avoid this potential issue.
> 
>      Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>      Reviewed-by: David Ahern <dsahern@kernel.org>
>      Signed-off-by: David S. Miller <davem@davemloft.net>
> 

Do you happen to have a test program that can reproduce it?

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Eric Dumazet]

On Thu, Dec 7, 2023 at 7:06 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:

> Do you happen to have a test program that can reproduce it?

syzbot has a repro, let me release the bug.

Of course syzbot bisection points to my last patch.

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Kui-Feng Lee]

On 12/7/23 10:10, Eric Dumazet wrote:
> On Thu, Dec 7, 2023 at 7:06 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
> 
>> Do you happen to have a test program that can reproduce it?
> 
> syzbot has a repro, let me release the bug.
> 
> Of course syzbot bisection points to my last patch.

I just looked into the code.
The origin issue mentioned at the thread head should be something
related to a GC change I made. But, the warnings you added doesn't
catch the the error correctly.  According to your stacktrace


 > ip6_route_add+0x26/0x1f0 net/ipv6/route.c:3843
 > ipv6_route_ioctl+0x3ff/0x590 net/ipv6/route.c:4467
 > inet6_ioctl+0x265/0x2b0 net/ipv6/af_inet6.c:575
 > sock_do_ioctl+0x113/0x270 net/socket.c:1220
 > sock_ioctl+0x22e/0x6b0 net/socket.c:1339
 > vfs_ioctl fs/ioctl.c:51 [inline]
 > __do_sys_ioctl fs/ioctl.c:871 [inline]
 > __se_sys_ioctl fs/ioctl.c:857 [inline]
 > __x64_sys_ioctl+0x18f/0x210 fs/ioctl.c:857
 > do_syscall_x64 arch/x86/entry/common.c:51 [inline]
 > do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
 > entry_SYSCALL_64_after_hwframe+0x63/0x6b

and warning messages you provided

 > WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
 > fib6_info_release include/net/ip6_fib.h:332 [inline]
 > WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
 > ip6_route_info_create+0x1a1a/0x1f10 net/ipv6/route.c:3829

It takes place in ip6_route_info_create() to do error handling.
It can be fib6_has_expires() in fib6_info_release() in this case.

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Eric Dumazet]

On Thu, Dec 7, 2023 at 7:19 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>
>
>
> On 12/7/23 10:10, Eric Dumazet wrote:
> > On Thu, Dec 7, 2023 at 7:06 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
> >
> >> Do you happen to have a test program that can reproduce it?
> >
> > syzbot has a repro, let me release the bug.
> >
> > Of course syzbot bisection points to my last patch.
>
> I just looked into the code.
> The origin issue mentioned at the thread head should be something
> related to a GC change I made. But, the warnings you added doesn't
> catch the the error correctly.  According to your stacktrace
>
>
>  > ip6_route_add+0x26/0x1f0 net/ipv6/route.c:3843
>  > ipv6_route_ioctl+0x3ff/0x590 net/ipv6/route.c:4467
>  > inet6_ioctl+0x265/0x2b0 net/ipv6/af_inet6.c:575
>  > sock_do_ioctl+0x113/0x270 net/socket.c:1220
>  > sock_ioctl+0x22e/0x6b0 net/socket.c:1339
>  > vfs_ioctl fs/ioctl.c:51 [inline]
>  > __do_sys_ioctl fs/ioctl.c:871 [inline]
>  > __se_sys_ioctl fs/ioctl.c:857 [inline]
>  > __x64_sys_ioctl+0x18f/0x210 fs/ioctl.c:857
>  > do_syscall_x64 arch/x86/entry/common.c:51 [inline]
>  > do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
>  > entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
> and warning messages you provided
>
>  > WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
>  > fib6_info_release include/net/ip6_fib.h:332 [inline]
>  > WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
>  > ip6_route_info_create+0x1a1a/0x1f10 net/ipv6/route.c:3829
>
> It takes place in ip6_route_info_create() to do error handling.
> It can be fib6_has_expires() in fib6_info_release() in this case.

Feel free to amend the patch, but the issue is that we insert a fib
gc_link to a list,
then free the fi6 object without removing it first from the external list.

I added two different warnings, and removing one or both will still
keep the bug.

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[David Ahern]

On 12/7/23 11:22 AM, Eric Dumazet wrote:
> Feel free to amend the patch, but the issue is that we insert a fib
> gc_link to a list, then free the fi6 object without removing it first
> from the external list.

yes, move the insert down:

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b132feae3393..7257ba0e72b7 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3762,12 +3762,6 @@ static struct fib6_info
*ip6_route_info_create(struct fib6_config *cfg,
        if (cfg->fc_flags & RTF_ADDRCONF)
                rt->dst_nocount = true;

-       if (cfg->fc_flags & RTF_EXPIRES)
-               fib6_set_expires_locked(rt, jiffies +
-
clock_t_to_jiffies(cfg->fc_expires));
-       else
-               fib6_clean_expires_locked(rt);
-
        if (cfg->fc_protocol == RTPROT_UNSPEC)
                cfg->fc_protocol = RTPROT_BOOT;
        rt->fib6_protocol = cfg->fc_protocol;
@@ -3824,6 +3818,12 @@ static struct fib6_info
*ip6_route_info_create(struct fib6_config *cfg,
        } else
                rt->fib6_prefsrc.plen = 0;

+
+       if (cfg->fc_flags & RTF_EXPIRES)
+               fib6_set_expires_locked(rt, jiffies +
+
clock_t_to_jiffies(cfg->fc_expires));
+       else
+               fib6_clean_expires_locked(rt);
        return rt;
 out:
        fib6_info_release(rt);

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[David Ahern]

On 12/7/23 11:25 AM, David Ahern wrote:
> On 12/7/23 11:22 AM, Eric Dumazet wrote:
>> Feel free to amend the patch, but the issue is that we insert a fib
>> gc_link to a list, then free the fi6 object without removing it first
>> from the external list.
> 
> yes, move the insert down:
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index b132feae3393..7257ba0e72b7 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3762,12 +3762,6 @@ static struct fib6_info
> *ip6_route_info_create(struct fib6_config *cfg,
>         if (cfg->fc_flags & RTF_ADDRCONF)
>                 rt->dst_nocount = true;
> 
> -       if (cfg->fc_flags & RTF_EXPIRES)
> -               fib6_set_expires_locked(rt, jiffies +
> -
> clock_t_to_jiffies(cfg->fc_expires));
> -       else
> -               fib6_clean_expires_locked(rt);
> -
>         if (cfg->fc_protocol == RTPROT_UNSPEC)
>                 cfg->fc_protocol = RTPROT_BOOT;
>         rt->fib6_protocol = cfg->fc_protocol;
> @@ -3824,6 +3818,12 @@ static struct fib6_info
> *ip6_route_info_create(struct fib6_config *cfg,
>         } else
>                 rt->fib6_prefsrc.plen = 0;
> 
> +
> +       if (cfg->fc_flags & RTF_EXPIRES)
> +               fib6_set_expires_locked(rt, jiffies +
> +
> clock_t_to_jiffies(cfg->fc_expires));
> +       else
> +               fib6_clean_expires_locked(rt);
>         return rt;
>  out:
>         fib6_info_release(rt);


and then failure to insert into the table needs to remove it before the
release too.

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Kui-Feng Lee]

On 12/7/23 10:25, David Ahern wrote:
> On 12/7/23 11:22 AM, Eric Dumazet wrote:
>> Feel free to amend the patch, but the issue is that we insert a fib
>> gc_link to a list, then free the fi6 object without removing it first
>> from the external list.
> 
> yes, move the insert down:
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index b132feae3393..7257ba0e72b7 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3762,12 +3762,6 @@ static struct fib6_info
> *ip6_route_info_create(struct fib6_config *cfg,
>          if (cfg->fc_flags & RTF_ADDRCONF)
>                  rt->dst_nocount = true;
> 
> -       if (cfg->fc_flags & RTF_EXPIRES)
> -               fib6_set_expires_locked(rt, jiffies +
> -
> clock_t_to_jiffies(cfg->fc_expires));
> -       else
> -               fib6_clean_expires_locked(rt);
> -

fib6_set_expires_locked() here actually doesn't insert a fib gc_link
since rt->fib6_table is not assigned yet.  The gc_link will
be inserted by fib6_add() being called later.


>          if (cfg->fc_protocol == RTPROT_UNSPEC)
>                  cfg->fc_protocol = RTPROT_BOOT;
>          rt->fib6_protocol = cfg->fc_protocol;
> @@ -3824,6 +3818,12 @@ static struct fib6_info
> *ip6_route_info_create(struct fib6_config *cfg,
>          } else
>                  rt->fib6_prefsrc.plen = 0;
> 
> +
> +       if (cfg->fc_flags & RTF_EXPIRES)
> +               fib6_set_expires_locked(rt, jiffies +
> +
> clock_t_to_jiffies(cfg->fc_expires));
> +       else
> +               fib6_clean_expires_locked(rt);
>          return rt;
>   out:
>          fib6_info_release(rt);

However, this should fix the warning messages.

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Kui-Feng Lee]

On 12/7/23 10:36, Kui-Feng Lee wrote:
> 
> 
> On 12/7/23 10:25, David Ahern wrote:
>> On 12/7/23 11:22 AM, Eric Dumazet wrote:
>>> Feel free to amend the patch, but the issue is that we insert a fib
>>> gc_link to a list, then free the fi6 object without removing it first
>>> from the external list.
>>
>> yes, move the insert down:
>>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index b132feae3393..7257ba0e72b7 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -3762,12 +3762,6 @@ static struct fib6_info
>> *ip6_route_info_create(struct fib6_config *cfg,
>>          if (cfg->fc_flags & RTF_ADDRCONF)
>>                  rt->dst_nocount = true;
>>
>> -       if (cfg->fc_flags & RTF_EXPIRES)
>> -               fib6_set_expires_locked(rt, jiffies +
>> -
>> clock_t_to_jiffies(cfg->fc_expires));
>> -       else
>> -               fib6_clean_expires_locked(rt);
>> -
> 
> fib6_set_expires_locked() here actually doesn't insert a fib gc_link
> since rt->fib6_table is not assigned yet.  The gc_link will
> be inserted by fib6_add() being called later.
> 
> 
>>          if (cfg->fc_protocol == RTPROT_UNSPEC)
>>                  cfg->fc_protocol = RTPROT_BOOT;
>>          rt->fib6_protocol = cfg->fc_protocol;
>> @@ -3824,6 +3818,12 @@ static struct fib6_info
>> *ip6_route_info_create(struct fib6_config *cfg,
>>          } else
>>                  rt->fib6_prefsrc.plen = 0;
>>
>> +
>> +       if (cfg->fc_flags & RTF_EXPIRES)
>> +               fib6_set_expires_locked(rt, jiffies +
>> +
>> clock_t_to_jiffies(cfg->fc_expires));
>> +       else
>> +               fib6_clean_expires_locked(rt);
>>          return rt;
>>   out:
>>          fib6_info_release(rt);
> 
> However, this should fix the warning messages.
Just realize this cause inserting the gc_link twice.  fib6_add()
will try to add it again!

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Kui-Feng Lee]

On 12/7/23 10:40, Kui-Feng Lee wrote:
> 
> 
> On 12/7/23 10:36, Kui-Feng Lee wrote:
>>
>>
>> On 12/7/23 10:25, David Ahern wrote:
>>> On 12/7/23 11:22 AM, Eric Dumazet wrote:
>>>> Feel free to amend the patch, but the issue is that we insert a fib
>>>> gc_link to a list, then free the fi6 object without removing it first
>>>> from the external list.
>>>
>>> yes, move the insert down:
>>>
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index b132feae3393..7257ba0e72b7 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -3762,12 +3762,6 @@ static struct fib6_info
>>> *ip6_route_info_create(struct fib6_config *cfg,
>>>          if (cfg->fc_flags & RTF_ADDRCONF)
>>>                  rt->dst_nocount = true;
>>>
>>> -       if (cfg->fc_flags & RTF_EXPIRES)
>>> -               fib6_set_expires_locked(rt, jiffies +
>>> -
>>> clock_t_to_jiffies(cfg->fc_expires));
>>> -       else
>>> -               fib6_clean_expires_locked(rt);
>>> -
>>
>> fib6_set_expires_locked() here actually doesn't insert a fib gc_link
>> since rt->fib6_table is not assigned yet.  The gc_link will
>> be inserted by fib6_add() being called later.
>>
>>
>>>          if (cfg->fc_protocol == RTPROT_UNSPEC)
>>>                  cfg->fc_protocol = RTPROT_BOOT;
>>>          rt->fib6_protocol = cfg->fc_protocol;
>>> @@ -3824,6 +3818,12 @@ static struct fib6_info
>>> *ip6_route_info_create(struct fib6_config *cfg,
>>>          } else
>>>                  rt->fib6_prefsrc.plen = 0;
>>>
>>> +
>>> +       if (cfg->fc_flags & RTF_EXPIRES)
>>> +               fib6_set_expires_locked(rt, jiffies +
>>> +
>>> clock_t_to_jiffies(cfg->fc_expires));
>>> +       else
>>> +               fib6_clean_expires_locked(rt);
>>>          return rt;
>>>   out:
>>>          fib6_info_release(rt);
>>
>> However, this should fix the warning messages.
> Just realize this cause inserting the gc_link twice.  fib6_add()
> will try to add it again!

I made a minor change to the patch that fix warning messages
provided by David Ahern. Will send an official patch later.

--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3762,17 +3762,10 @@ static struct fib6_info 
*ip6_route_info_create(struct fib6_config *cfg,
         if (cfg->fc_flags & RTF_ADDRCONF)
                 rt->dst_nocount = true;

-       if (cfg->fc_flags & RTF_EXPIRES)
-               fib6_set_expires_locked(rt, jiffies +
- 
clock_t_to_jiffies(cfg->fc_expires));
-       else
-               fib6_clean_expires_locked(rt);
-
         if (cfg->fc_protocol == RTPROT_UNSPEC)
                 cfg->fc_protocol = RTPROT_BOOT;
         rt->fib6_protocol = cfg->fc_protocol;

-       rt->fib6_table = table;
         rt->fib6_metric = cfg->fc_metric;
         rt->fib6_type = cfg->fc_type ? : RTN_UNICAST;
         rt->fib6_flags = cfg->fc_flags & ~RTF_GATEWAY;
@@ -3824,6 +3817,17 @@ static struct fib6_info 
*ip6_route_info_create(struct fib6_config *cfg,
         } else
                 rt->fib6_prefsrc.plen = 0;

+       if (cfg->fc_flags & RTF_EXPIRES)
+               fib6_set_expires_locked(rt, jiffies +
+ 
clock_t_to_jiffies(cfg->fc_expires));
+       else
+               fib6_clean_expires_locked(rt);
+       /* Set fib6_table after fib6_set_expires_locked() to ensure the
+        * gc_link is not inserted until fib6_add() is called to insert the
+        * fib6_info to the fib.
+        */
+       rt->fib6_table = table;
+
         return rt;
  out:

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Eric Dumazet]

On Thu, Dec 7, 2023 at 8:00 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>
>
>
> On 12/7/23 10:40, Kui-Feng Lee wrote:
> >
> >
> > On 12/7/23 10:36, Kui-Feng Lee wrote:
> >>
> >>
> >> On 12/7/23 10:25, David Ahern wrote:
> >>> On 12/7/23 11:22 AM, Eric Dumazet wrote:
> >>>> Feel free to amend the patch, but the issue is that we insert a fib
> >>>> gc_link to a list, then free the fi6 object without removing it first
> >>>> from the external list.
> >>>
> >>> yes, move the insert down:
> >>>
> >>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> >>> index b132feae3393..7257ba0e72b7 100644
> >>> --- a/net/ipv6/route.c
> >>> +++ b/net/ipv6/route.c
> >>> @@ -3762,12 +3762,6 @@ static struct fib6_info
> >>> *ip6_route_info_create(struct fib6_config *cfg,
> >>>          if (cfg->fc_flags & RTF_ADDRCONF)
> >>>                  rt->dst_nocount = true;
> >>>
> >>> -       if (cfg->fc_flags & RTF_EXPIRES)
> >>> -               fib6_set_expires_locked(rt, jiffies +
> >>> -
> >>> clock_t_to_jiffies(cfg->fc_expires));
> >>> -       else
> >>> -               fib6_clean_expires_locked(rt);
> >>> -
> >>
> >> fib6_set_expires_locked() here actually doesn't insert a fib gc_link
> >> since rt->fib6_table is not assigned yet.  The gc_link will
> >> be inserted by fib6_add() being called later.
> >>
> >>
> >>>          if (cfg->fc_protocol == RTPROT_UNSPEC)
> >>>                  cfg->fc_protocol = RTPROT_BOOT;
> >>>          rt->fib6_protocol = cfg->fc_protocol;
> >>> @@ -3824,6 +3818,12 @@ static struct fib6_info
> >>> *ip6_route_info_create(struct fib6_config *cfg,
> >>>          } else
> >>>                  rt->fib6_prefsrc.plen = 0;
> >>>
> >>> +
> >>> +       if (cfg->fc_flags & RTF_EXPIRES)
> >>> +               fib6_set_expires_locked(rt, jiffies +
> >>> +
> >>> clock_t_to_jiffies(cfg->fc_expires));
> >>> +       else
> >>> +               fib6_clean_expires_locked(rt);
> >>>          return rt;
> >>>   out:
> >>>          fib6_info_release(rt);
> >>
> >> However, this should fix the warning messages.
> > Just realize this cause inserting the gc_link twice.  fib6_add()
> > will try to add it again!
>
> I made a minor change to the patch that fix warning messages
> provided by David Ahern. Will send an official patch later.
>
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3762,17 +3762,10 @@ static struct fib6_info
> *ip6_route_info_create(struct fib6_config *cfg,
>          if (cfg->fc_flags & RTF_ADDRCONF)
>                  rt->dst_nocount = true;
>
> -       if (cfg->fc_flags & RTF_EXPIRES)
> -               fib6_set_expires_locked(rt, jiffies +
> -
> clock_t_to_jiffies(cfg->fc_expires));
> -       else
> -               fib6_clean_expires_locked(rt);
> -
>          if (cfg->fc_protocol == RTPROT_UNSPEC)
>                  cfg->fc_protocol = RTPROT_BOOT;
>          rt->fib6_protocol = cfg->fc_protocol;
>
> -       rt->fib6_table = table;
>          rt->fib6_metric = cfg->fc_metric;
>          rt->fib6_type = cfg->fc_type ? : RTN_UNICAST;
>          rt->fib6_flags = cfg->fc_flags & ~RTF_GATEWAY;
> @@ -3824,6 +3817,17 @@ static struct fib6_info
> *ip6_route_info_create(struct fib6_config *cfg,
>          } else
>                  rt->fib6_prefsrc.plen = 0;
>
> +       if (cfg->fc_flags & RTF_EXPIRES)
> +               fib6_set_expires_locked(rt, jiffies +
> +
> clock_t_to_jiffies(cfg->fc_expires));
> +       else
> +               fib6_clean_expires_locked(rt);

Note that I do not see why we call fib6_clean_expires_locked() on a
freshly allocated object.

f6i->expires should already be zero...

> +       /* Set fib6_table after fib6_set_expires_locked() to ensure the
> +        * gc_link is not inserted until fib6_add() is called to insert the
> +        * fib6_info to the fib.
> +        */
> +       rt->fib6_table = table;
> +
>          return rt;
>   out:

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Kui-Feng Lee]

On 12/7/23 11:05, Eric Dumazet wrote:
> On Thu, Dec 7, 2023 at 8:00 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>
>>
>>
>> On 12/7/23 10:40, Kui-Feng Lee wrote:
>>>
>>>
>>> On 12/7/23 10:36, Kui-Feng Lee wrote:
>>>>
>>>>
>>>> On 12/7/23 10:25, David Ahern wrote:
>>>>> On 12/7/23 11:22 AM, Eric Dumazet wrote:
>>>>>> Feel free to amend the patch, but the issue is that we insert a fib
>>>>>> gc_link to a list, then free the fi6 object without removing it first
>>>>>> from the external list.
>>>>>
>>>>> yes, move the insert down:
>>>>>
>>>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>>>> index b132feae3393..7257ba0e72b7 100644
>>>>> --- a/net/ipv6/route.c
>>>>> +++ b/net/ipv6/route.c
>>>>> @@ -3762,12 +3762,6 @@ static struct fib6_info
>>>>> *ip6_route_info_create(struct fib6_config *cfg,
>>>>>           if (cfg->fc_flags & RTF_ADDRCONF)
>>>>>                   rt->dst_nocount = true;
>>>>>
>>>>> -       if (cfg->fc_flags & RTF_EXPIRES)
>>>>> -               fib6_set_expires_locked(rt, jiffies +
>>>>> -
>>>>> clock_t_to_jiffies(cfg->fc_expires));
>>>>> -       else
>>>>> -               fib6_clean_expires_locked(rt);
>>>>> -
>>>>
>>>> fib6_set_expires_locked() here actually doesn't insert a fib gc_link
>>>> since rt->fib6_table is not assigned yet.  The gc_link will
>>>> be inserted by fib6_add() being called later.
>>>>
>>>>
>>>>>           if (cfg->fc_protocol == RTPROT_UNSPEC)
>>>>>                   cfg->fc_protocol = RTPROT_BOOT;
>>>>>           rt->fib6_protocol = cfg->fc_protocol;
>>>>> @@ -3824,6 +3818,12 @@ static struct fib6_info
>>>>> *ip6_route_info_create(struct fib6_config *cfg,
>>>>>           } else
>>>>>                   rt->fib6_prefsrc.plen = 0;
>>>>>
>>>>> +
>>>>> +       if (cfg->fc_flags & RTF_EXPIRES)
>>>>> +               fib6_set_expires_locked(rt, jiffies +
>>>>> +
>>>>> clock_t_to_jiffies(cfg->fc_expires));
>>>>> +       else
>>>>> +               fib6_clean_expires_locked(rt);
>>>>>           return rt;
>>>>>    out:
>>>>>           fib6_info_release(rt);
>>>>
>>>> However, this should fix the warning messages.
>>> Just realize this cause inserting the gc_link twice.  fib6_add()
>>> will try to add it again!
>>
>> I made a minor change to the patch that fix warning messages
>> provided by David Ahern. Will send an official patch later.
>>
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -3762,17 +3762,10 @@ static struct fib6_info
>> *ip6_route_info_create(struct fib6_config *cfg,
>>           if (cfg->fc_flags & RTF_ADDRCONF)
>>                   rt->dst_nocount = true;
>>
>> -       if (cfg->fc_flags & RTF_EXPIRES)
>> -               fib6_set_expires_locked(rt, jiffies +
>> -
>> clock_t_to_jiffies(cfg->fc_expires));
>> -       else
>> -               fib6_clean_expires_locked(rt);
>> -
>>           if (cfg->fc_protocol == RTPROT_UNSPEC)
>>                   cfg->fc_protocol = RTPROT_BOOT;
>>           rt->fib6_protocol = cfg->fc_protocol;
>>
>> -       rt->fib6_table = table;
>>           rt->fib6_metric = cfg->fc_metric;
>>           rt->fib6_type = cfg->fc_type ? : RTN_UNICAST;
>>           rt->fib6_flags = cfg->fc_flags & ~RTF_GATEWAY;
>> @@ -3824,6 +3817,17 @@ static struct fib6_info
>> *ip6_route_info_create(struct fib6_config *cfg,
>>           } else
>>                   rt->fib6_prefsrc.plen = 0;
>>
>> +       if (cfg->fc_flags & RTF_EXPIRES)
>> +               fib6_set_expires_locked(rt, jiffies +
>> +
>> clock_t_to_jiffies(cfg->fc_expires));
>> +       else
>> +               fib6_clean_expires_locked(rt);
> 
> Note that I do not see why we call fib6_clean_expires_locked() on a
> freshly allocated object.
> 
> f6i->expires should already be zero...
> 

Agree! I will remove it as well.

>> +       /* Set fib6_table after fib6_set_expires_locked() to ensure the
>> +        * gc_link is not inserted until fib6_add() is called to insert the
>> +        * fib6_info to the fib.
>> +        */
>> +       rt->fib6_table = table;
>> +
>>           return rt;
>>    out:

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Eric Dumazet]

On Thu, Dec 7, 2023 at 8:08 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>
>
>
> On 12/7/23 11:05, Eric Dumazet wrote:
> > On Thu, Dec 7, 2023 at 8:00 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
> >>
> >>
> >>
> >> On 12/7/23 10:40, Kui-Feng Lee wrote:
> >>>
> >>>
> >>> On 12/7/23 10:36, Kui-Feng Lee wrote:
> >>>>
> >>>>
> >>>> On 12/7/23 10:25, David Ahern wrote:
> >>>>> On 12/7/23 11:22 AM, Eric Dumazet wrote:
> >>>>>> Feel free to amend the patch, but the issue is that we insert a fib
> >>>>>> gc_link to a list, then free the fi6 object without removing it first
> >>>>>> from the external list.
> >>>>>
> >>>>> yes, move the insert down:
> >>>>>
> >>>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> >>>>> index b132feae3393..7257ba0e72b7 100644
> >>>>> --- a/net/ipv6/route.c
> >>>>> +++ b/net/ipv6/route.c
> >>>>> @@ -3762,12 +3762,6 @@ static struct fib6_info
> >>>>> *ip6_route_info_create(struct fib6_config *cfg,
> >>>>>           if (cfg->fc_flags & RTF_ADDRCONF)
> >>>>>                   rt->dst_nocount = true;
> >>>>>
> >>>>> -       if (cfg->fc_flags & RTF_EXPIRES)
> >>>>> -               fib6_set_expires_locked(rt, jiffies +
> >>>>> -
> >>>>> clock_t_to_jiffies(cfg->fc_expires));
> >>>>> -       else
> >>>>> -               fib6_clean_expires_locked(rt);
> >>>>> -
> >>>>
> >>>> fib6_set_expires_locked() here actually doesn't insert a fib gc_link
> >>>> since rt->fib6_table is not assigned yet.  The gc_link will
> >>>> be inserted by fib6_add() being called later.
> >>>>
> >>>>
> >>>>>           if (cfg->fc_protocol == RTPROT_UNSPEC)
> >>>>>                   cfg->fc_protocol = RTPROT_BOOT;
> >>>>>           rt->fib6_protocol = cfg->fc_protocol;
> >>>>> @@ -3824,6 +3818,12 @@ static struct fib6_info
> >>>>> *ip6_route_info_create(struct fib6_config *cfg,
> >>>>>           } else
> >>>>>                   rt->fib6_prefsrc.plen = 0;
> >>>>>
> >>>>> +
> >>>>> +       if (cfg->fc_flags & RTF_EXPIRES)
> >>>>> +               fib6_set_expires_locked(rt, jiffies +
> >>>>> +
> >>>>> clock_t_to_jiffies(cfg->fc_expires));
> >>>>> +       else
> >>>>> +               fib6_clean_expires_locked(rt);
> >>>>>           return rt;
> >>>>>    out:
> >>>>>           fib6_info_release(rt);
> >>>>
> >>>> However, this should fix the warning messages.
> >>> Just realize this cause inserting the gc_link twice.  fib6_add()
> >>> will try to add it again!
> >>
> >> I made a minor change to the patch that fix warning messages
> >> provided by David Ahern. Will send an official patch later.
> >>
> >> --- a/net/ipv6/route.c
> >> +++ b/net/ipv6/route.c
> >> @@ -3762,17 +3762,10 @@ static struct fib6_info
> >> *ip6_route_info_create(struct fib6_config *cfg,
> >>           if (cfg->fc_flags & RTF_ADDRCONF)
> >>                   rt->dst_nocount = true;
> >>
> >> -       if (cfg->fc_flags & RTF_EXPIRES)
> >> -               fib6_set_expires_locked(rt, jiffies +
> >> -
> >> clock_t_to_jiffies(cfg->fc_expires));
> >> -       else
> >> -               fib6_clean_expires_locked(rt);
> >> -
> >>           if (cfg->fc_protocol == RTPROT_UNSPEC)
> >>                   cfg->fc_protocol = RTPROT_BOOT;
> >>           rt->fib6_protocol = cfg->fc_protocol;
> >>
> >> -       rt->fib6_table = table;
> >>           rt->fib6_metric = cfg->fc_metric;
> >>           rt->fib6_type = cfg->fc_type ? : RTN_UNICAST;
> >>           rt->fib6_flags = cfg->fc_flags & ~RTF_GATEWAY;
> >> @@ -3824,6 +3817,17 @@ static struct fib6_info
> >> *ip6_route_info_create(struct fib6_config *cfg,
> >>           } else
> >>                   rt->fib6_prefsrc.plen = 0;
> >>
> >> +       if (cfg->fc_flags & RTF_EXPIRES)
> >> +               fib6_set_expires_locked(rt, jiffies +
> >> +
> >> clock_t_to_jiffies(cfg->fc_expires));
> >> +       else
> >> +               fib6_clean_expires_locked(rt);
> >
> > Note that I do not see why we call fib6_clean_expires_locked() on a
> > freshly allocated object.
> >
> > f6i->expires should already be zero...
> >
>
> Agree! I will remove it as well.

Please also add the following, if really we keep fib6_has_expires()
current definition.

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);
        }

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Kui-Feng Lee]

On 12/7/23 10:22, Eric Dumazet wrote:
> On Thu, Dec 7, 2023 at 7:19 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>
>>
>>
>> On 12/7/23 10:10, Eric Dumazet wrote:
>>> On Thu, Dec 7, 2023 at 7:06 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>>
>>>> Do you happen to have a test program that can reproduce it?
>>>
>>> syzbot has a repro, let me release the bug.
>>>
>>> Of course syzbot bisection points to my last patch.
>>
>> I just looked into the code.
>> The origin issue mentioned at the thread head should be something
>> related to a GC change I made. But, the warnings you added doesn't
>> catch the the error correctly.  According to your stacktrace
>>
>>
>>   > ip6_route_add+0x26/0x1f0 net/ipv6/route.c:3843
>>   > ipv6_route_ioctl+0x3ff/0x590 net/ipv6/route.c:4467
>>   > inet6_ioctl+0x265/0x2b0 net/ipv6/af_inet6.c:575
>>   > sock_do_ioctl+0x113/0x270 net/socket.c:1220
>>   > sock_ioctl+0x22e/0x6b0 net/socket.c:1339
>>   > vfs_ioctl fs/ioctl.c:51 [inline]
>>   > __do_sys_ioctl fs/ioctl.c:871 [inline]
>>   > __se_sys_ioctl fs/ioctl.c:857 [inline]
>>   > __x64_sys_ioctl+0x18f/0x210 fs/ioctl.c:857
>>   > do_syscall_x64 arch/x86/entry/common.c:51 [inline]
>>   > do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
>>   > entry_SYSCALL_64_after_hwframe+0x63/0x6b
>>
>> and warning messages you provided
>>
>>   > WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
>>   > fib6_info_release include/net/ip6_fib.h:332 [inline]
>>   > WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
>>   > ip6_route_info_create+0x1a1a/0x1f10 net/ipv6/route.c:3829
>>
>> It takes place in ip6_route_info_create() to do error handling.
>> It can be fib6_has_expires() in fib6_info_release() in this case.
> 
> Feel free to amend the patch, but the issue is that we insert a fib
> gc_link to a list,
> then free the fi6 object without removing it first from the external list.
> 
> I added two different warnings, and removing one or both will still
> keep the bug.

The gc_link is not inserted here actually. (see my explanation in
another message.)

According to the messages in the thread head, it is an issue of dangling
pointer, right? If I read it correctly, the original issue is gc_link
pointing to a block of memory that is already free. Am I right?

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Eric Dumazet]

On Thu, Dec 7, 2023 at 7:46 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>
>
>
> On 12/7/23 10:22, Eric Dumazet wrote:
> > On Thu, Dec 7, 2023 at 7:19 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
> >>
> >>
> >>
> >> On 12/7/23 10:10, Eric Dumazet wrote:
> >>> On Thu, Dec 7, 2023 at 7:06 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
> >>>
> >>>> Do you happen to have a test program that can reproduce it?
> >>>
> >>> syzbot has a repro, let me release the bug.
> >>>
> >>> Of course syzbot bisection points to my last patch.
> >>
> >> I just looked into the code.
> >> The origin issue mentioned at the thread head should be something
> >> related to a GC change I made. But, the warnings you added doesn't
> >> catch the the error correctly.  According to your stacktrace
> >>
> >>
> >>   > ip6_route_add+0x26/0x1f0 net/ipv6/route.c:3843
> >>   > ipv6_route_ioctl+0x3ff/0x590 net/ipv6/route.c:4467
> >>   > inet6_ioctl+0x265/0x2b0 net/ipv6/af_inet6.c:575
> >>   > sock_do_ioctl+0x113/0x270 net/socket.c:1220
> >>   > sock_ioctl+0x22e/0x6b0 net/socket.c:1339
> >>   > vfs_ioctl fs/ioctl.c:51 [inline]
> >>   > __do_sys_ioctl fs/ioctl.c:871 [inline]
> >>   > __se_sys_ioctl fs/ioctl.c:857 [inline]
> >>   > __x64_sys_ioctl+0x18f/0x210 fs/ioctl.c:857
> >>   > do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> >>   > do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
> >>   > entry_SYSCALL_64_after_hwframe+0x63/0x6b
> >>
> >> and warning messages you provided
> >>
> >>   > WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
> >>   > fib6_info_release include/net/ip6_fib.h:332 [inline]
> >>   > WARNING: CPU: 0 PID: 5059 at include/net/ip6_fib.h:332
> >>   > ip6_route_info_create+0x1a1a/0x1f10 net/ipv6/route.c:3829
> >>
> >> It takes place in ip6_route_info_create() to do error handling.
> >> It can be fib6_has_expires() in fib6_info_release() in this case.
> >
> > Feel free to amend the patch, but the issue is that we insert a fib
> > gc_link to a list,
> > then free the fi6 object without removing it first from the external list.
> >
> > I added two different warnings, and removing one or both will still
> > keep the bug.
>
> The gc_link is not inserted here actually. (see my explanation in
> another message.)
>
> According to the messages in the thread head, it is an issue of dangling
> pointer, right? If I read it correctly, the original issue is gc_link
> pointing to a block of memory that is already free. Am I right?

Original issue is about gc_link corruption, or use-after-free

Perhaps fib6_has_expires()  should really be rewritten to use
hlist_unhashed(&f6i->gc_link)

Setting RTF_EXPIRES on f6i->fib6_flags seems redundant/risky in light
of all the syzbot reports I had lately...

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Kui-Feng Lee]

Hi Eric, could you also open a bug for this incident?


On 12/5/23 09:32, Eric Dumazet wrote:
> Some elusive syzbot reports are hinting to fib6_info_release(),
> with a potential dangling f6i->gc_link anchor.
> 
> Add debug checks so that syzbot can catch the issue earlier eventually.
> 
> BUG: KASAN: slab-use-after-free in __hlist_del include/linux/list.h:990 [inline]
> BUG: KASAN: slab-use-after-free in hlist_del_init include/linux/list.h:1016 [inline]
> BUG: KASAN: slab-use-after-free in fib6_clean_expires_locked include/net/ip6_fib.h:533 [inline]
> BUG: KASAN: slab-use-after-free in fib6_purge_rt+0x986/0x9c0 net/ipv6/ip6_fib.c:1064
> Write of size 8 at addr ffff88802805a840 by task syz-executor.1/10057
> 
> CPU: 1 PID: 10057 Comm: syz-executor.1 Not tainted 6.7.0-rc2-syzkaller-00029-g9b6de136b5f0 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/10/2023
> Call Trace:
> <TASK>
> __dump_stack lib/dump_stack.c:88 [inline]
> dump_stack_lvl+0xd9/0x1b0 lib/dump_stack.c:106
> print_address_description mm/kasan/report.c:364 [inline]
> print_report+0xc4/0x620 mm/kasan/report.c:475
> kasan_report+0xda/0x110 mm/kasan/report.c:588
> __hlist_del include/linux/list.h:990 [inline]
> hlist_del_init include/linux/list.h:1016 [inline]
> fib6_clean_expires_locked include/net/ip6_fib.h:533 [inline]
> fib6_purge_rt+0x986/0x9c0 net/ipv6/ip6_fib.c:1064
> fib6_del_route net/ipv6/ip6_fib.c:1993 [inline]
> fib6_del+0xa7a/0x1750 net/ipv6/ip6_fib.c:2038
> __ip6_del_rt net/ipv6/route.c:3866 [inline]
> ip6_del_rt+0xf7/0x200 net/ipv6/route.c:3881
> ndisc_router_discovery+0x295b/0x3560 net/ipv6/ndisc.c:1372
> ndisc_rcv+0x3de/0x5f0 net/ipv6/ndisc.c:1856
> icmpv6_rcv+0x1470/0x19c0 net/ipv6/icmp.c:979
> ip6_protocol_deliver_rcu+0x170/0x13e0 net/ipv6/ip6_input.c:438
> ip6_input_finish+0x14f/0x2f0 net/ipv6/ip6_input.c:483
> NF_HOOK include/linux/netfilter.h:314 [inline]
> NF_HOOK include/linux/netfilter.h:308 [inline]
> ip6_input+0xa1/0xc0 net/ipv6/ip6_input.c:492
> ip6_mc_input+0x48b/0xf40 net/ipv6/ip6_input.c:586
> dst_input include/net/dst.h:461 [inline]
> ip6_rcv_finish net/ipv6/ip6_input.c:79 [inline]
> NF_HOOK include/linux/netfilter.h:314 [inline]
> NF_HOOK include/linux/netfilter.h:308 [inline]
> ipv6_rcv+0x24e/0x380 net/ipv6/ip6_input.c:310
> __netif_receive_skb_one_core+0x115/0x180 net/core/dev.c:5529
> __netif_receive_skb+0x1f/0x1b0 net/core/dev.c:5643
> netif_receive_skb_internal net/core/dev.c:5729 [inline]
> netif_receive_skb+0x133/0x700 net/core/dev.c:5788
> tun_rx_batched+0x429/0x780 drivers/net/tun.c:1579
> tun_get_user+0x29e3/0x3bc0 drivers/net/tun.c:2002
> tun_chr_write_iter+0xe8/0x210 drivers/net/tun.c:2048
> call_write_iter include/linux/fs.h:2020 [inline]
> new_sync_write fs/read_write.c:491 [inline]
> vfs_write+0x64f/0xdf0 fs/read_write.c:584
> ksys_write+0x12f/0x250 fs/read_write.c:637
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
> RIP: 0033:0x7f38e387b82f
> Code: 89 54 24 18 48 89 74 24 10 89 7c 24 08 e8 b9 80 02 00 48 8b 54 24 18 48 8b 74 24 10 41 89 c0 8b 7c 24 08 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 31 44 89 c7 48 89 44 24 08 e8 0c 81 02 00 48
> RSP: 002b:00007f38e45c9090 EFLAGS: 00000293 ORIG_RAX: 0000000000000001
> RAX: ffffffffffffffda RBX: 00007f38e399bf80 RCX: 00007f38e387b82f
> RDX: 00000000000003b6 RSI: 0000000020000680 RDI: 00000000000000c8
> RBP: 00007f38e38c847a R08: 0000000000000000 R09: 0000000000000000
> R10: 00000000000003b6 R11: 0000000000000293 R12: 0000000000000000
> R13: 000000000000000b R14: 00007f38e399bf80 R15: 00007f38e3abfa48
> </TASK>
> 
> Allocated by task 10044:
> kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
> kasan_set_track+0x25/0x30 mm/kasan/common.c:52
> ____kasan_kmalloc mm/kasan/common.c:374 [inline]
> __kasan_kmalloc+0xa2/0xb0 mm/kasan/common.c:383
> kasan_kmalloc include/linux/kasan.h:198 [inline]
> __do_kmalloc_node mm/slab_common.c:1007 [inline]
> __kmalloc+0x59/0x90 mm/slab_common.c:1020
> kmalloc include/linux/slab.h:604 [inline]
> kzalloc include/linux/slab.h:721 [inline]
> fib6_info_alloc+0x40/0x160 net/ipv6/ip6_fib.c:155
> ip6_route_info_create+0x337/0x1e70 net/ipv6/route.c:3749
> ip6_route_add+0x26/0x150 net/ipv6/route.c:3843
> rt6_add_route_info+0x2e7/0x4b0 net/ipv6/route.c:4316
> rt6_route_rcv+0x76c/0xbf0 net/ipv6/route.c:985
> ndisc_router_discovery+0x138b/0x3560 net/ipv6/ndisc.c:1529
> ndisc_rcv+0x3de/0x5f0 net/ipv6/ndisc.c:1856
> icmpv6_rcv+0x1470/0x19c0 net/ipv6/icmp.c:979
> ip6_protocol_deliver_rcu+0x170/0x13e0 net/ipv6/ip6_input.c:438
> ip6_input_finish+0x14f/0x2f0 net/ipv6/ip6_input.c:483
> NF_HOOK include/linux/netfilter.h:314 [inline]
> NF_HOOK include/linux/netfilter.h:308 [inline]
> ip6_input+0xa1/0xc0 net/ipv6/ip6_input.c:492
> ip6_mc_input+0x48b/0xf40 net/ipv6/ip6_input.c:586
> dst_input include/net/dst.h:461 [inline]
> ip6_rcv_finish net/ipv6/ip6_input.c:79 [inline]
> NF_HOOK include/linux/netfilter.h:314 [inline]
> NF_HOOK include/linux/netfilter.h:308 [inline]
> ipv6_rcv+0x24e/0x380 net/ipv6/ip6_input.c:310
> __netif_receive_skb_one_core+0x115/0x180 net/core/dev.c:5529
> __netif_receive_skb+0x1f/0x1b0 net/core/dev.c:5643
> netif_receive_skb_internal net/core/dev.c:5729 [inline]
> netif_receive_skb+0x133/0x700 net/core/dev.c:5788
> tun_rx_batched+0x429/0x780 drivers/net/tun.c:1579
> tun_get_user+0x29e3/0x3bc0 drivers/net/tun.c:2002
> tun_chr_write_iter+0xe8/0x210 drivers/net/tun.c:2048
> call_write_iter include/linux/fs.h:2020 [inline]
> new_sync_write fs/read_write.c:491 [inline]
> vfs_write+0x64f/0xdf0 fs/read_write.c:584
> ksys_write+0x12f/0x250 fs/read_write.c:637
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
> 
> Freed by task 5123:
> kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
> kasan_set_track+0x25/0x30 mm/kasan/common.c:52
> kasan_save_free_info+0x2b/0x40 mm/kasan/generic.c:522
> ____kasan_slab_free mm/kasan/common.c:236 [inline]
> ____kasan_slab_free+0x15b/0x1b0 mm/kasan/common.c:200
> kasan_slab_free include/linux/kasan.h:164 [inline]
> slab_free_hook mm/slub.c:1800 [inline]
> slab_free_freelist_hook+0x114/0x1e0 mm/slub.c:1826
> slab_free mm/slub.c:3809 [inline]
> __kmem_cache_free+0xc0/0x180 mm/slub.c:3822
> rcu_do_batch kernel/rcu/tree.c:2158 [inline]
> rcu_core+0x819/0x1680 kernel/rcu/tree.c:2431
> __do_softirq+0x21a/0x8de kernel/softirq.c:553
> 
> Last potentially related work creation:
> kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
> __kasan_record_aux_stack+0xbc/0xd0 mm/kasan/generic.c:492
> __call_rcu_common.constprop.0+0x9a/0x7a0 kernel/rcu/tree.c:2681
> fib6_info_release include/net/ip6_fib.h:332 [inline]
> fib6_info_release include/net/ip6_fib.h:329 [inline]
> rt6_route_rcv+0xa4e/0xbf0 net/ipv6/route.c:997
> ndisc_router_discovery+0x138b/0x3560 net/ipv6/ndisc.c:1529
> ndisc_rcv+0x3de/0x5f0 net/ipv6/ndisc.c:1856
> icmpv6_rcv+0x1470/0x19c0 net/ipv6/icmp.c:979
> ip6_protocol_deliver_rcu+0x170/0x13e0 net/ipv6/ip6_input.c:438
> ip6_input_finish+0x14f/0x2f0 net/ipv6/ip6_input.c:483
> NF_HOOK include/linux/netfilter.h:314 [inline]
> NF_HOOK include/linux/netfilter.h:308 [inline]
> ip6_input+0xa1/0xc0 net/ipv6/ip6_input.c:492
> ip6_mc_input+0x48b/0xf40 net/ipv6/ip6_input.c:586
> dst_input include/net/dst.h:461 [inline]
> ip6_rcv_finish net/ipv6/ip6_input.c:79 [inline]
> NF_HOOK include/linux/netfilter.h:314 [inline]
> NF_HOOK include/linux/netfilter.h:308 [inline]
> ipv6_rcv+0x24e/0x380 net/ipv6/ip6_input.c:310
> __netif_receive_skb_one_core+0x115/0x180 net/core/dev.c:5529
> __netif_receive_skb+0x1f/0x1b0 net/core/dev.c:5643
> netif_receive_skb_internal net/core/dev.c:5729 [inline]
> netif_receive_skb+0x133/0x700 net/core/dev.c:5788
> tun_rx_batched+0x429/0x780 drivers/net/tun.c:1579
> tun_get_user+0x29e3/0x3bc0 drivers/net/tun.c:2002
> tun_chr_write_iter+0xe8/0x210 drivers/net/tun.c:2048
> call_write_iter include/linux/fs.h:2020 [inline]
> new_sync_write fs/read_write.c:491 [inline]
> vfs_write+0x64f/0xdf0 fs/read_write.c:584
> ksys_write+0x12f/0x250 fs/read_write.c:637
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
> 
> Second to last potentially related work creation:
> kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
> __kasan_record_aux_stack+0xbc/0xd0 mm/kasan/generic.c:492
> insert_work+0x38/0x230 kernel/workqueue.c:1647
> __queue_work+0xcdc/0x11f0 kernel/workqueue.c:1803
> call_timer_fn+0x193/0x590 kernel/time/timer.c:1700
> expire_timers kernel/time/timer.c:1746 [inline]
> __run_timers+0x585/0xb20 kernel/time/timer.c:2022
> run_timer_softirq+0x58/0xd0 kernel/time/timer.c:2035
> __do_softirq+0x21a/0x8de kernel/softirq.c:553
> 
> The buggy address belongs to the object at ffff88802805a800
> which belongs to the cache kmalloc-512 of size 512
> The buggy address is located 64 bytes inside of
> freed 512-byte region [ffff88802805a800, ffff88802805aa00)
> 
> The buggy address belongs to the physical page:
> page:ffffea0000a01600 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x28058
> head:ffffea0000a01600 order:2 entire_mapcount:0 nr_pages_mapped:0 pincount:0
> flags: 0xfff00000000840(slab|head|node=0|zone=1|lastcpupid=0x7ff)
> page_type: 0xffffffff()
> raw: 00fff00000000840 ffff888013041c80 ffffea0001e02600 dead000000000002
> raw: 0000000000000000 0000000000100010 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
> page_owner tracks the page as allocated
> page last allocated via order 2, migratetype Unmovable, gfp_mask 0x1d20c0(__GFP_IO|__GFP_FS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC|__GFP_HARDWALL), pid 18706, tgid 18699 (syz-executor.2), ts 999991973280, free_ts 996884464281
> set_page_owner include/linux/page_owner.h:31 [inline]
> post_alloc_hook+0x2d0/0x350 mm/page_alloc.c:1537
> prep_new_page mm/page_alloc.c:1544 [inline]
> get_page_from_freelist+0xa25/0x36d0 mm/page_alloc.c:3312
> __alloc_pages+0x22e/0x2420 mm/page_alloc.c:4568
> alloc_pages_mpol+0x258/0x5f0 mm/mempolicy.c:2133
> alloc_slab_page mm/slub.c:1870 [inline]
> allocate_slab mm/slub.c:2017 [inline]
> new_slab+0x283/0x3c0 mm/slub.c:2070
> ___slab_alloc+0x979/0x1500 mm/slub.c:3223
> __slab_alloc.constprop.0+0x56/0xa0 mm/slub.c:3322
> __slab_alloc_node mm/slub.c:3375 [inline]
> slab_alloc_node mm/slub.c:3468 [inline]
> __kmem_cache_alloc_node+0x131/0x310 mm/slub.c:3517
> __do_kmalloc_node mm/slab_common.c:1006 [inline]
> __kmalloc+0x49/0x90 mm/slab_common.c:1020
> kmalloc include/linux/slab.h:604 [inline]
> kzalloc include/linux/slab.h:721 [inline]
> copy_splice_read+0x1ac/0x8f0 fs/splice.c:338
> vfs_splice_read fs/splice.c:992 [inline]
> vfs_splice_read+0x2ea/0x3b0 fs/splice.c:962
> splice_direct_to_actor+0x2a5/0xa30 fs/splice.c:1069
> do_splice_direct+0x1af/0x280 fs/splice.c:1194
> do_sendfile+0xb3e/0x1310 fs/read_write.c:1254
> __do_sys_sendfile64 fs/read_write.c:1322 [inline]
> __se_sys_sendfile64 fs/read_write.c:1308 [inline]
> __x64_sys_sendfile64+0x1d6/0x220 fs/read_write.c:1308
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
> page last free stack trace:
> reset_page_owner include/linux/page_owner.h:24 [inline]
> free_pages_prepare mm/page_alloc.c:1137 [inline]
> free_unref_page_prepare+0x4fa/0xaa0 mm/page_alloc.c:2347
> free_unref_page_list+0xe6/0xb40 mm/page_alloc.c:2533
> release_pages+0x32a/0x14f0 mm/swap.c:1042
> tlb_batch_pages_flush+0x9a/0x190 mm/mmu_gather.c:98
> tlb_flush_mmu_free mm/mmu_gather.c:293 [inline]
> tlb_flush_mmu mm/mmu_gather.c:300 [inline]
> tlb_finish_mmu+0x14b/0x6f0 mm/mmu_gather.c:392
> exit_mmap+0x38b/0xa70 mm/mmap.c:3321
> __mmput+0x12a/0x4d0 kernel/fork.c:1349
> mmput+0x62/0x70 kernel/fork.c:1371
> exit_mm kernel/exit.c:567 [inline]
> do_exit+0x9ad/0x2ae0 kernel/exit.c:858
> do_group_exit+0xd4/0x2a0 kernel/exit.c:1021
> get_signal+0x23be/0x2790 kernel/signal.c:2904
> arch_do_signal_or_restart+0x90/0x7f0 arch/x86/kernel/signal.c:309
> exit_to_user_mode_loop kernel/entry/common.c:168 [inline]
> exit_to_user_mode_prepare+0x121/0x240 kernel/entry/common.c:204
> irqentry_exit_to_user_mode+0xa/0x40 kernel/entry/common.c:309
> asm_sysvec_apic_timer_interrupt+0x1a/0x20 arch/x86/include/asm/idtentry.h:645
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>   include/net/ip6_fib.h | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index 1ba9f4ddf2f6db6c6ebf0a0675ca74fea2273fd9..e1e7a894863a7891610ce5afb2034473cc208d3e 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -328,8 +328,11 @@ 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))
> +	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);
> +	}
>   }
>   
>   enum fib6_walk_state {

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Eric Dumazet]

On Fri, Dec 8, 2023 at 12:02 AM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>
> Hi Eric, could you also open a bug for this incident?

What are you asking for exactly ?

We have thousands of syzbot bugs, it is done by a bot, not a human.

Re: [PATCH net-next] ipv6: add debug checks in fib6_info_release()

[Kui-Feng Lee]

On 12/8/23 01:18, Eric Dumazet wrote:
> On Fri, Dec 8, 2023 at 12:02 AM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>
>> Hi Eric, could you also open a bug for this incident?
> 
> What are you asking for exactly ?
> 
> We have thousands of syzbot bugs, it is done by a bot, not a human.

You mentioned "let me release the bug." [1] Then we have [2].

By the way, although I am not fixing the issue described by [2],
I am using that email to talk with syzbot.

[1] 
https://lore.kernel.org/all/CANn89iKpM33oQ+2dwoLHzZvECAjwiKJTR3cDM64nE6VvZA99Sg@mail.gmail.com/
[2] https://lore.kernel.org/all/000000000000cb5b07060bef7ac0@google.com/