About ip6_dst_destroy()

About ip6_dst_destroy()

[Eric Dumazet]

David

I am staring at ip6_dst_destroy() and the last part makes really no sense to me.

How rcu_read_lock()/rcu_read_unnlock() can help in a writer side ???

Changlog of a68886a691804d3f6d479ebf6825480fbafb6a00 ("net/ipv6: Make from in rt6_info rcu protected")
does not make sense either.

<quote>
    There is a race window when a FIB entry is deleted and the 'from' on the
    pcpu route is dropped and the pcpu route hits a cookie check. Handle
    this race using rcu on from.
</quote>

Re: About ip6_dst_destroy()

[David Ahern]

On 4/27/19 5:56 PM, Eric Dumazet wrote:
> David
> 
> I am staring at ip6_dst_destroy() and the last part makes really no sense to me.
> 
> How rcu_read_lock()/rcu_read_unnlock() can help in a writer side ???
> 
> Changlog of a68886a691804d3f6d479ebf6825480fbafb6a00 ("net/ipv6: Make from in rt6_info rcu protected")
> does not make sense either.
> 
> <quote>
>     There is a race window when a FIB entry is deleted and the 'from' on the
>     pcpu route is dropped and the pcpu route hits a cookie check. Handle
>     this race using rcu on from.
> </quote>
> 

A FIB entry (fib6_info) is deleted, but resources are not cleaned up as
there are outstanding references to the entry. Specifically, the
references are the 'from' on pcpu routes. Commit (93531c6743157) added
code to release those references as otherwise there is nothing that
forces it. Further testing hit the condition noted in a68886a69180.

I presume you are asking about ip6_dst_destroy vs all of the other
'from' references because of the fib6_info_release - which would result
in an underflow when it is released twice. I guess something like a
rmb() / wmb() pair is needed for this case.

Re: About ip6_dst_destroy()

[Eric Dumazet]

On 4/27/19 8:22 PM, David Ahern wrote:
> On 4/27/19 5:56 PM, Eric Dumazet wrote:
>> David
>>
>> I am staring at ip6_dst_destroy() and the last part makes really no sense to me.
>>
>> How rcu_read_lock()/rcu_read_unnlock() can help in a writer side ???
>>
>> Changlog of a68886a691804d3f6d479ebf6825480fbafb6a00 ("net/ipv6: Make from in rt6_info rcu protected")
>> does not make sense either.
>>
>> <quote>
>>     There is a race window when a FIB entry is deleted and the 'from' on the
>>     pcpu route is dropped and the pcpu route hits a cookie check. Handle
>>     this race using rcu on from.
>> </quote>
>>
> 
> A FIB entry (fib6_info) is deleted, but resources are not cleaned up as
> there are outstanding references to the entry. Specifically, the
> references are the 'from' on pcpu routes. Commit (93531c6743157) added
> code to release those references as otherwise there is nothing that
> forces it. Further testing hit the condition noted in a68886a69180.
> 
> I presume you are asking about ip6_dst_destroy vs all of the other
> 'from' references because of the fib6_info_release - which would result
> in an underflow when it is released twice. I guess something like a
> rmb() / wmb() pair is needed for this case.

I do not see how rmb/wmb pair will help.

Writers need to use a stronger synchronization between themselves. 

This can be some spinlock, a xchg() or cmpxchg()

The problem here is that nothing prevent ip6_dst_destroy() being called concurrently
with another writer like fib6_drop_pcpu_from()

fib6_drop_pcpu_from() uses &table->tb6_lock, which is not held in ip6_dst_destroy()

I will submit a patch switching all writers to xchg()

Re: About ip6_dst_destroy()

[Eric Dumazet]

On 4/28/19 7:57 AM, Eric Dumazet wrote:
> 
> 
> On 4/27/19 8:22 PM, David Ahern wrote:
>> On 4/27/19 5:56 PM, Eric Dumazet wrote:
>>> David
>>>
>>> I am staring at ip6_dst_destroy() and the last part makes really no sense to me.
>>>
>>> How rcu_read_lock()/rcu_read_unnlock() can help in a writer side ???
>>>
>>> Changlog of a68886a691804d3f6d479ebf6825480fbafb6a00 ("net/ipv6: Make from in rt6_info rcu protected")
>>> does not make sense either.
>>>
>>> <quote>
>>>     There is a race window when a FIB entry is deleted and the 'from' on the
>>>     pcpu route is dropped and the pcpu route hits a cookie check. Handle
>>>     this race using rcu on from.
>>> </quote>
>>>
>>
>> A FIB entry (fib6_info) is deleted, but resources are not cleaned up as
>> there are outstanding references to the entry. Specifically, the
>> references are the 'from' on pcpu routes. Commit (93531c6743157) added
>> code to release those references as otherwise there is nothing that
>> forces it. Further testing hit the condition noted in a68886a69180.
>>
>> I presume you are asking about ip6_dst_destroy vs all of the other
>> 'from' references because of the fib6_info_release - which would result
>> in an underflow when it is released twice. I guess something like a
>> rmb() / wmb() pair is needed for this case.
> 
> I do not see how rmb/wmb pair will help.
> 
> Writers need to use a stronger synchronization between themselves. 
> 
> This can be some spinlock, a xchg() or cmpxchg()
> 
> The problem here is that nothing prevent ip6_dst_destroy() being called concurrently
> with another writer like fib6_drop_pcpu_from()
> 
> fib6_drop_pcpu_from() uses &table->tb6_lock, which is not held in ip6_dst_destroy()
> 
> I will submit a patch switching all writers to xchg()
> 

Here is a typical trace :


BUG: KASAN: user-memory-access in atomic_dec_and_test include/asm-generic/atomic-instrumented.h:747 [inline]
BUG: KASAN: user-memory-access in fib6_info_release include/net/ip6_fib.h:294 [inline]
BUG: KASAN: user-memory-access in fib6_info_release include/net/ip6_fib.h:292 [inline]
BUG: KASAN: user-memory-access in fib6_drop_pcpu_from net/ipv6/ip6_fib.c:927 [inline]
BUG: KASAN: user-memory-access in fib6_purge_rt+0x4f6/0x670 net/ipv6/ip6_fib.c:960
Write of size 4 at addr 0000000000ffffb4 by task syz-executor.1/7649

CPU: 0 PID: 7649 Comm: syz-executor.1 Not tainted 5.1.0-rc6+ #183
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x172/0x1f0 lib/dump_stack.c:113
 kasan_report.cold+0x5/0x40 mm/kasan/report.c:321
 check_memory_region_inline mm/kasan/generic.c:185 [inline]
 check_memory_region+0x123/0x190 mm/kasan/generic.c:191
 kasan_check_write+0x14/0x20 mm/kasan/common.c:108
 atomic_dec_and_test include/asm-generic/atomic-instrumented.h:747 [inline]
 fib6_info_release include/net/ip6_fib.h:294 [inline]
 fib6_info_release include/net/ip6_fib.h:292 [inline]
 fib6_drop_pcpu_from net/ipv6/ip6_fib.c:927 [inline]
 fib6_purge_rt+0x4f6/0x670 net/ipv6/ip6_fib.c:960
 fib6_del_route net/ipv6/ip6_fib.c:1813 [inline]
 fib6_del+0xac2/0x10a0 net/ipv6/ip6_fib.c:1844
 fib6_clean_node+0x3a8/0x590 net/ipv6/ip6_fib.c:2006
 fib6_walk_continue+0x495/0x900 net/ipv6/ip6_fib.c:1928
 fib6_walk+0x9d/0x100 net/ipv6/ip6_fib.c:1976
 fib6_clean_tree+0xe0/0x120 net/ipv6/ip6_fib.c:2055
 __fib6_clean_all+0x118/0x2a0 net/ipv6/ip6_fib.c:2071
 fib6_clean_all+0x2b/0x40 net/ipv6/ip6_fib.c:2082
 rt6_sync_down_dev+0x134/0x150 net/ipv6/route.c:4057
 rt6_disable_ip+0x27/0x5f0 net/ipv6/route.c:4062
 addrconf_ifdown+0xa2/0x1220 net/ipv6/addrconf.c:3705
 addrconf_notify+0x19a/0x2260 net/ipv6/addrconf.c:3630
 notifier_call_chain+0xc7/0x240 kernel/notifier.c:93
 __raw_notifier_call_chain kernel/notifier.c:394 [inline]
 raw_notifier_call_chain+0x2e/0x40 kernel/notifier.c:401
 call_netdevice_notifiers_info+0x3f/0x90 net/core/dev.c:1753
 call_netdevice_notifiers_extack net/core/dev.c:1765 [inline]
 call_netdevice_notifiers net/core/dev.c:1779 [inline]
 dev_close_many+0x33f/0x6f0 net/core/dev.c:1522
 rollback_registered_many+0x43b/0xfd0 net/core/dev.c:8177
 rollback_registered+0x109/0x1d0 net/core/dev.c:8242
 unregister_netdevice_queue net/core/dev.c:9289 [inline]
 unregister_netdevice_queue+0x1ee/0x2c0 net/core/dev.c:9282
 unregister_netdevice include/linux/netdevice.h:2658 [inline]
 __tun_detach+0xd5b/0x1000 drivers/net/tun.c:727
 tun_detach drivers/net/tun.c:744 [inline]
 tun_chr_close+0xe0/0x180 drivers/net/tun.c:3443
 __fput+0x2e5/0x8d0 fs/file_table.c:278
 ____fput+0x16/0x20 fs/file_table.c:309
 task_work_run+0x14a/0x1c0 kernel/task_work.c:113
 exit_task_work include/linux/task_work.h:22 [inline]
 do_exit+0x90a/0x2fa0 kernel/exit.c:876
 do_group_exit+0x135/0x370 kernel/exit.c:980
 __do_sys_exit_group kernel/exit.c:991 [inline]
 __se_sys_exit_group kernel/exit.c:989 [inline]
 __x64_sys_exit_group+0x44/0x50 kernel/exit.c:989
 do_syscall_64+0x103/0x610 arch/x86/entry/common.c:290
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x458da9
Code: ad b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffeafc2a6a8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 000000000000001c RCX: 0000000000458da9
RDX: 0000000000412a80 RSI: 0000000000a54ef0 RDI: 0000000000000043
RBP: 00000000004be552 R08: 000000000000000c R09: 000000000004c0d1
R10: 0000000002341940 R11: 0000000000000246 R12: 00000000ffffffff
R13: 00007ffeafc2a7f0 R14: 000000000004c065 R15: 00007ffeafc2a800