From: Simon Horman <horms@kernel.org>
To: James Chapman <jchapman@katalix.com>
Cc: netdev@vger.kernel.org, davem@davemloft.net, edumazet@google.com,
kuba@kernel.org, pabeni@redhat.com, tparkin@katalix.com,
samuel.thibault@ens-lyon.org, thorsten.blum@toblux.com
Subject: Re: [PATCH net] l2tp: make session IDR and tunnel session list coherent
Date: Fri, 19 Jul 2024 09:41:49 +0100 [thread overview]
Message-ID: <20240719084149.GC616453@kernel.org> (raw)
In-Reply-To: <20240718134348.289865-1-jchapman@katalix.com>
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@katalix.com>
> Signed-off-by: Tom Parkin <tparkin@katalix.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@kernel.org>
next prev parent reply other threads:[~2024-07-19 8:41 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-18 13:43 [PATCH net] l2tp: make session IDR and tunnel session list coherent James Chapman
2024-07-19 8:41 ` Simon Horman [this message]
2024-07-23 9:13 ` Paolo Abeni
2024-07-23 9:30 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240719084149.GC616453@kernel.org \
--to=horms@kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=jchapman@katalix.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=samuel.thibault@ens-lyon.org \
--cc=thorsten.blum@toblux.com \
--cc=tparkin@katalix.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).