diff mbox series

[v2,net] ipmr: tune the ipmr_can_free_table() checks.

Message ID 7d13b21f439acd027e510890ba4b353994bad058.1733129879.git.pabeni@redhat.com (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series [v2,net] ipmr: tune the ipmr_can_free_table() checks. | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 41 this patch: 41
netdev/build_tools success Errors and warnings before: 0 (+0) this patch: 0 (+0)
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 73 this patch: 73
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 4561 this patch: 4561
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 27 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 1 this patch: 1
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-12-02--12-00 (tests: 760)

Commit Message

Paolo Abeni Dec. 2, 2024, 8:59 a.m. UTC 
Eric reported a syzkaller-triggered splat caused by recent ipmr changes:

WARNING: CPU: 2 PID: 6041 at net/ipv6/ip6mr.c:419
ip6mr_free_table+0xbd/0x120 net/ipv6/ip6mr.c:419
Modules linked in:
CPU: 2 UID: 0 PID: 6041 Comm: syz-executor183 Not tainted
6.12.0-syzkaller-10681-g65ae975e97d5 #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
RIP: 0010:ip6mr_free_table+0xbd/0x120 net/ipv6/ip6mr.c:419
Code: 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 80 3c
02 00 75 58 49 83 bc 24 c0 0e 00 00 00 74 09 e8 44 ef a9 f7 90 <0f> 0b
90 e8 3b ef a9 f7 48 8d 7b 38 e8 12 a3 96 f7 48 89 df be 0f
RSP: 0018:ffffc90004267bd8 EFLAGS: 00010293
RAX: 0000000000000000 RBX: ffff88803c710000 RCX: ffffffff89e4d844
RDX: ffff88803c52c880 RSI: ffffffff89e4d87c RDI: ffff88803c578ec0
RBP: 0000000000000001 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000001 R12: ffff88803c578000
R13: ffff88803c710000 R14: ffff88803c710008 R15: dead000000000100
FS: 00007f7a855ee6c0(0000) GS:ffff88806a800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f7a85689938 CR3: 000000003c492000 CR4: 0000000000352ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
ip6mr_rules_exit+0x176/0x2d0 net/ipv6/ip6mr.c:283
ip6mr_net_exit_batch+0x53/0xa0 net/ipv6/ip6mr.c:1388
ops_exit_list+0x128/0x180 net/core/net_namespace.c:177
setup_net+0x4fe/0x860 net/core/net_namespace.c:394
copy_net_ns+0x2b4/0x6b0 net/core/net_namespace.c:500
create_new_namespaces+0x3ea/0xad0 kernel/nsproxy.c:110
unshare_nsproxy_namespaces+0xc0/0x1f0 kernel/nsproxy.c:228
ksys_unshare+0x45d/0xa40 kernel/fork.c:3334
__do_sys_unshare kernel/fork.c:3405 [inline]
__se_sys_unshare kernel/fork.c:3403 [inline]
__x64_sys_unshare+0x31/0x40 kernel/fork.c:3403
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f7a856332d9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 51 18 00 00 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 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f7a855ee238 EFLAGS: 00000246 ORIG_RAX: 0000000000000110
RAX: ffffffffffffffda RBX: 00007f7a856bd308 RCX: 00007f7a856332d9
RDX: 00007f7a8560f8c6 RSI: 0000000000000000 RDI: 0000000062040200
RBP: 00007f7a856bd300 R08: 00007fff932160a7 R09: 00007f7a855ee6c0
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f7a856bd30c
R13: 0000000000000000 R14: 00007fff93215fc0 R15: 00007fff932160a8
</TASK>

The root cause is a network namespace creation failing after successful
initialization of the ipmr subsystem. Such a case is not currently
matched by the ipmr_can_free_table() helper.

New namespaces are zeroed on allocation and inserted into net ns list
only after successful creation; when deleting an ipmr table, the list
next pointer can be NULL only on netns initialization failure.

Update the ipmr_can_free_table() checks leveraging such condition.

Reported-by: Eric Dumazet <edumazet@google.com>
Fixes: 11b6e701bce9 ("ipmr: add debug check for mr table cleanup")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
 - move the netns init completed check in a new helper
---
 include/net/net_namespace.h | 5 +++++
 net/ipv4/ipmr.c             | 2 +-
 net/ipv6/ip6mr.c            | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

Comments

Eric Dumazet Dec. 2, 2024, 9:11 a.m. UTC | #1 
On Mon, Dec 2, 2024 at 9:59 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Eric reported a syzkaller-triggered splat caused by recent ipmr changes:
>
> WARNING: CPU: 2 PID: 6041 at net/ipv6/ip6mr.c:419
> ip6mr_free_table+0xbd/0x120 net/ipv6/ip6mr.c:419
> Modules linked in:
> CPU: 2 UID: 0 PID: 6041 Comm: syz-executor183 Not tainted
> 6.12.0-syzkaller-10681-g65ae975e97d5 #0
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
> 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
> RIP: 0010:ip6mr_free_table+0xbd/0x120 net/ipv6/ip6mr.c:419
> Code: 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 80 3c
> 02 00 75 58 49 83 bc 24 c0 0e 00 00 00 74 09 e8 44 ef a9 f7 90 <0f> 0b
> 90 e8 3b ef a9 f7 48 8d 7b 38 e8 12 a3 96 f7 48 89 df be 0f
> RSP: 0018:ffffc90004267bd8 EFLAGS: 00010293
> RAX: 0000000000000000 RBX: ffff88803c710000 RCX: ffffffff89e4d844
> RDX: ffff88803c52c880 RSI: ffffffff89e4d87c RDI: ffff88803c578ec0
> RBP: 0000000000000001 R08: 0000000000000005 R09: 0000000000000000
> R10: 0000000000000001 R11: 0000000000000001 R12: ffff88803c578000
> R13: ffff88803c710000 R14: ffff88803c710008 R15: dead000000000100
> FS: 00007f7a855ee6c0(0000) GS:ffff88806a800000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f7a85689938 CR3: 000000003c492000 CR4: 0000000000352ef0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> ip6mr_rules_exit+0x176/0x2d0 net/ipv6/ip6mr.c:283
> ip6mr_net_exit_batch+0x53/0xa0 net/ipv6/ip6mr.c:1388
> ops_exit_list+0x128/0x180 net/core/net_namespace.c:177
> setup_net+0x4fe/0x860 net/core/net_namespace.c:394
> copy_net_ns+0x2b4/0x6b0 net/core/net_namespace.c:500
> create_new_namespaces+0x3ea/0xad0 kernel/nsproxy.c:110
> unshare_nsproxy_namespaces+0xc0/0x1f0 kernel/nsproxy.c:228
> ksys_unshare+0x45d/0xa40 kernel/fork.c:3334
> __do_sys_unshare kernel/fork.c:3405 [inline]
> __se_sys_unshare kernel/fork.c:3403 [inline]
> __x64_sys_unshare+0x31/0x40 kernel/fork.c:3403
> do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f7a856332d9
> Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 51 18 00 00 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 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007f7a855ee238 EFLAGS: 00000246 ORIG_RAX: 0000000000000110
> RAX: ffffffffffffffda RBX: 00007f7a856bd308 RCX: 00007f7a856332d9
> RDX: 00007f7a8560f8c6 RSI: 0000000000000000 RDI: 0000000062040200
> RBP: 00007f7a856bd300 R08: 00007fff932160a7 R09: 00007f7a855ee6c0
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007f7a856bd30c
> R13: 0000000000000000 R14: 00007fff93215fc0 R15: 00007fff932160a8
> </TASK>
>
> The root cause is a network namespace creation failing after successful
> initialization of the ipmr subsystem. Such a case is not currently
> matched by the ipmr_can_free_table() helper.
>
> New namespaces are zeroed on allocation and inserted into net ns list
> only after successful creation; when deleting an ipmr table, the list
> next pointer can be NULL only on netns initialization failure.
>
> Update the ipmr_can_free_table() checks leveraging such condition.
>
> Reported-by: Eric Dumazet <edumazet@google.com>
> Fixes: 11b6e701bce9 ("ipmr: add debug check for mr table cleanup")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
>  - move the netns init completed check in a new helper
> ---
>  include/net/net_namespace.h | 5 +++++
>  net/ipv4/ipmr.c             | 2 +-
>  net/ipv6/ip6mr.c            | 2 +-
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index 873c0f9fdac6..ac301c7d48a4 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -325,6 +325,11 @@ static inline int check_net(const struct net *net)
>  #define net_drop_ns NULL
>  #endif
>
> +/* Returns true if the netns initialization is completed successfully */
> +static inline bool net_initialized(struct net *net)
> +{
> +       return net->list.next;

It is unclear what lock protects this read (or context it can be used)

Perhaps we could make clear no lock is needed (and add a const qual)

static inline bool net_initialized(const struct net *net)
{
       return READ_ONCE(net->list.next);
}
Paolo Abeni Dec. 2, 2024, 9:48 a.m. UTC | #2 
On 12/2/24 10:11, Eric Dumazet wrote:
> On Mon, Dec 2, 2024 at 9:59 AM Paolo Abeni <pabeni@redhat.com> wrote:
>> Eric reported a syzkaller-triggered splat caused by recent ipmr changes:
>>
>> WARNING: CPU: 2 PID: 6041 at net/ipv6/ip6mr.c:419
>> ip6mr_free_table+0xbd/0x120 net/ipv6/ip6mr.c:419
>> Modules linked in:
>> CPU: 2 UID: 0 PID: 6041 Comm: syz-executor183 Not tainted
>> 6.12.0-syzkaller-10681-g65ae975e97d5 #0
>> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
>> 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
>> RIP: 0010:ip6mr_free_table+0xbd/0x120 net/ipv6/ip6mr.c:419
>> Code: 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 80 3c
>> 02 00 75 58 49 83 bc 24 c0 0e 00 00 00 74 09 e8 44 ef a9 f7 90 <0f> 0b
>> 90 e8 3b ef a9 f7 48 8d 7b 38 e8 12 a3 96 f7 48 89 df be 0f
>> RSP: 0018:ffffc90004267bd8 EFLAGS: 00010293
>> RAX: 0000000000000000 RBX: ffff88803c710000 RCX: ffffffff89e4d844
>> RDX: ffff88803c52c880 RSI: ffffffff89e4d87c RDI: ffff88803c578ec0
>> RBP: 0000000000000001 R08: 0000000000000005 R09: 0000000000000000
>> R10: 0000000000000001 R11: 0000000000000001 R12: ffff88803c578000
>> R13: ffff88803c710000 R14: ffff88803c710008 R15: dead000000000100
>> FS: 00007f7a855ee6c0(0000) GS:ffff88806a800000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00007f7a85689938 CR3: 000000003c492000 CR4: 0000000000352ef0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>> <TASK>
>> ip6mr_rules_exit+0x176/0x2d0 net/ipv6/ip6mr.c:283
>> ip6mr_net_exit_batch+0x53/0xa0 net/ipv6/ip6mr.c:1388
>> ops_exit_list+0x128/0x180 net/core/net_namespace.c:177
>> setup_net+0x4fe/0x860 net/core/net_namespace.c:394
>> copy_net_ns+0x2b4/0x6b0 net/core/net_namespace.c:500
>> create_new_namespaces+0x3ea/0xad0 kernel/nsproxy.c:110
>> unshare_nsproxy_namespaces+0xc0/0x1f0 kernel/nsproxy.c:228
>> ksys_unshare+0x45d/0xa40 kernel/fork.c:3334
>> __do_sys_unshare kernel/fork.c:3405 [inline]
>> __se_sys_unshare kernel/fork.c:3403 [inline]
>> __x64_sys_unshare+0x31/0x40 kernel/fork.c:3403
>> do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>> do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>> entry_SYSCALL_64_after_hwframe+0x77/0x7f
>> RIP: 0033:0x7f7a856332d9
>> Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 51 18 00 00 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 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
>> RSP: 002b:00007f7a855ee238 EFLAGS: 00000246 ORIG_RAX: 0000000000000110
>> RAX: ffffffffffffffda RBX: 00007f7a856bd308 RCX: 00007f7a856332d9
>> RDX: 00007f7a8560f8c6 RSI: 0000000000000000 RDI: 0000000062040200
>> RBP: 00007f7a856bd300 R08: 00007fff932160a7 R09: 00007f7a855ee6c0
>> R10: 0000000000000000 R11: 0000000000000246 R12: 00007f7a856bd30c
>> R13: 0000000000000000 R14: 00007fff93215fc0 R15: 00007fff932160a8
>> </TASK>
>>
>> The root cause is a network namespace creation failing after successful
>> initialization of the ipmr subsystem. Such a case is not currently
>> matched by the ipmr_can_free_table() helper.
>>
>> New namespaces are zeroed on allocation and inserted into net ns list
>> only after successful creation; when deleting an ipmr table, the list
>> next pointer can be NULL only on netns initialization failure.
>>
>> Update the ipmr_can_free_table() checks leveraging such condition.
>>
>> Reported-by: Eric Dumazet <edumazet@google.com>
>> Fixes: 11b6e701bce9 ("ipmr: add debug check for mr table cleanup")
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>> v1 -> v2:
>>  - move the netns init completed check in a new helper
>> ---
>>  include/net/net_namespace.h | 5 +++++
>>  net/ipv4/ipmr.c             | 2 +-
>>  net/ipv6/ip6mr.c            | 2 +-
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
>> index 873c0f9fdac6..ac301c7d48a4 100644
>> --- a/include/net/net_namespace.h
>> +++ b/include/net/net_namespace.h
>> @@ -325,6 +325,11 @@ static inline int check_net(const struct net *net)
>>  #define net_drop_ns NULL
>>  #endif
>>
>> +/* Returns true if the netns initialization is completed successfully */
>> +static inline bool net_initialized(struct net *net)
>> +{
>> +       return net->list.next;
> 
> It is unclear what lock protects this read (or context it can be used)
> 
> Perhaps we could make clear no lock is needed (and add a const qual)
> 
> static inline bool net_initialized(const struct net *net)
> {
>        return READ_ONCE(net->list.next);
> }

Thanks, I will do in v3.

Paolo
diff mbox series

Patch

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 873c0f9fdac6..ac301c7d48a4 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -325,6 +325,11 @@  static inline int check_net(const struct net *net)
 #define net_drop_ns NULL
 #endif
 
+/* Returns true if the netns initialization is completed successfully */
+static inline bool net_initialized(struct net *net)
+{
+	return net->list.next;
+}
 
 static inline void __netns_tracker_alloc(struct net *net,
 					 netns_tracker *tracker,
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index c5b8ec5c0a8c..99d8faa508e5 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -122,7 +122,7 @@  static void ipmr_expire_process(struct timer_list *t);
 
 static bool ipmr_can_free_table(struct net *net)
 {
-	return !check_net(net) || !net->ipv4.mr_rules_ops;
+	return !check_net(net) || !net_initialized(net);
 }
 
 static struct mr_table *ipmr_mr_table_iter(struct net *net,
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 7f1902ac3586..578ff1336afe 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -110,7 +110,7 @@  static void ipmr_expire_process(struct timer_list *t);
 
 static bool ip6mr_can_free_table(struct net *net)
 {
-	return !check_net(net) || !net->ipv6.mr6_rules_ops;
+	return !check_net(net) || !net_initialized(net);
 }
 
 static struct mr_table *ip6mr_mr_table_iter(struct net *net,