netdev - Re: [PATCH net] l2tp: make session IDR and tunnel session list coherent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240719084149.GC616453@kernel.org>
Date: Fri, 19 Jul 2024 09:41:49 +0100
From: Simon Horman <horms@...nel.org>
To: James Chapman <jchapman@...alix.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, tparkin@...alix.com,
	samuel.thibault@...-lyon.org, thorsten.blum@...lux.com
Subject: Re: [PATCH net] l2tp: make session IDR and tunnel session list
 coherent

On Thu, Jul 18, 2024 at 02:43:48PM +0100, James Chapman wrote:
> Modify l2tp_session_register and l2tp_session_unhash so that the
> session IDR and tunnel session lists remain coherent. To do so, hold
> the session IDR lock and the tunnel's session list lock when making
> any changes to either list.
> 
> Without this change, a rare race condition could hit the WARN_ON_ONCE
> in l2tp_session_unhash if a thread replaced the IDR entry while
> another thread was registering the same ID.
> 
>  [ 7126.151795][T17511] WARNING: CPU: 3 PID: 17511 at net/l2tp/l2tp_core.c:1282 l2tp_session_delete.part.0+0x87e/0xbc0
>  [ 7126.163754][T17511]  ? show_regs+0x93/0xa0
>  [ 7126.164157][T17511]  ? __warn+0xe5/0x3c0
>  [ 7126.164536][T17511]  ? l2tp_session_delete.part.0+0x87e/0xbc0
>  [ 7126.165070][T17511]  ? report_bug+0x2e1/0x500
>  [ 7126.165486][T17511]  ? l2tp_session_delete.part.0+0x87e/0xbc0
>  [ 7126.166013][T17511]  ? handle_bug+0x99/0x130
>  [ 7126.166428][T17511]  ? exc_invalid_op+0x35/0x80
>  [ 7126.166890][T17511]  ? asm_exc_invalid_op+0x1a/0x20
>  [ 7126.167372][T17511]  ? l2tp_session_delete.part.0+0x87d/0xbc0
>  [ 7126.167900][T17511]  ? l2tp_session_delete.part.0+0x87e/0xbc0
>  [ 7126.168429][T17511]  ? __local_bh_enable_ip+0xa4/0x120
>  [ 7126.168917][T17511]  l2tp_session_delete+0x40/0x50
>  [ 7126.169369][T17511]  pppol2tp_release+0x1a1/0x3f0
>  [ 7126.169817][T17511]  __sock_release+0xb3/0x270
>  [ 7126.170247][T17511]  ? __pfx_sock_close+0x10/0x10
>  [ 7126.170697][T17511]  sock_close+0x1c/0x30
>  [ 7126.171087][T17511]  __fput+0x40b/0xb90
>  [ 7126.171470][T17511]  task_work_run+0x16c/0x260
>  [ 7126.171897][T17511]  ? __pfx_task_work_run+0x10/0x10
>  [ 7126.172362][T17511]  ? srso_alias_return_thunk+0x5/0xfbef5
>  [ 7126.172863][T17511]  ? do_raw_spin_unlock+0x174/0x230
>  [ 7126.173348][T17511]  do_exit+0xaae/0x2b40
>  [ 7126.173730][T17511]  ? srso_alias_return_thunk+0x5/0xfbef5
>  [ 7126.174235][T17511]  ? __pfx_lock_release+0x10/0x10
>  [ 7126.174690][T17511]  ? srso_alias_return_thunk+0x5/0xfbef5
>  [ 7126.175190][T17511]  ? do_raw_spin_lock+0x12c/0x2b0
>  [ 7126.175650][T17511]  ? __pfx_do_exit+0x10/0x10
>  [ 7126.176072][T17511]  ? _raw_spin_unlock_irq+0x23/0x50
>  [ 7126.176543][T17511]  do_group_exit+0xd3/0x2a0
>  [ 7126.176990][T17511]  __x64_sys_exit_group+0x3e/0x50
>  [ 7126.177456][T17511]  x64_sys_call+0x1821/0x1830
>  [ 7126.177895][T17511]  do_syscall_64+0xcb/0x250
>  [ 7126.178317][T17511]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Fixes: aa5e17e1f5ec ("l2tp: store l2tpv3 sessions in per-net IDR")
> Signed-off-by: James Chapman <jchapman@...alix.com>
> Signed-off-by: Tom Parkin <tparkin@...alix.com>

Thanks James,

I agree that this addresses the issue described.
And, FWIIW, I also checked that the locking order is
consistent with that before this patch as for no reason
in particular I was concerned about deadlocks.

Reviewed-by: Simon Horman <horms@...nel.org>