netdev - Re: [PATCH ipsec 2/2] xfrm: state: use a consistent pcpu_id in xfrm_state

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: <aDQJ+kt5c0trlfo5@gauss3.secunet.de>
Date: Mon, 26 May 2025 08:28:10 +0200
From: Steffen Klassert <steffen.klassert@...unet.com>
To: Sabrina Dubroca <sd@...asysnail.net>
CC: <netdev@...r.kernel.org>, Antony Antony <antony.antony@...unet.com>,
	Tobias Brunner <tobias@...ongswan.org>, Florian Westphal <fw@...len.de>
Subject: Re: [PATCH ipsec 2/2] xfrm: state: use a consistent pcpu_id in
 xfrm_state_find

On Fri, May 23, 2025 at 05:11:18PM +0200, Sabrina Dubroca wrote:
> If we get preempted during xfrm_state_find, we could run
> xfrm_state_look_at using a different pcpu_id than the one
> xfrm_state_find saw. This could lead to ignoring states that should
> have matched, and triggering acquires on a CPU that already has a pcpu
> state.
> 
>     xfrm_state_find starts on CPU1
>     pcpu_id = 1
>     lookup starts
>     <preemption, we're now on CPU2>
>     xfrm_state_look_at pcpu_id = 2
>        finds a state
> found:
>     best->pcpu_num != pcpu_id (2 != 1)
>     if (!x && !error && !acquire_in_progress) {
>         ...
>         xfrm_state_alloc
>         xfrm_init_tempstate
>         ...
> 
> This can be avoided by passing the original pcpu_id down to all
> xfrm_state_look_at() calls.
> 
> Also switch to raw_smp_processor_id, disabling preempting just to
> re-enable it immediately doesn't really make sense.
> 
> Fixes: 1ddf9916ac09 ("xfrm: Add support for per cpu xfrm state handling.")
> Signed-off-by: Sabrina Dubroca <sd@...asysnail.net>
> ---
>  net/xfrm/xfrm_state.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index ff6813ecc6df..3dc78ef2bf7d 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1307,14 +1307,8 @@ static void xfrm_hash_grow_check(struct net *net, int have_hash_collision)
>  static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
>  			       const struct flowi *fl, unsigned short family,
>  			       struct xfrm_state **best, int *acq_in_progress,
> -			       int *error)
> +			       int *error, unsigned int pcpu_id)
>  {
> -	/* We need the cpu id just as a lookup key,
> -	 * we don't require it to be stable.
> -	 */
> -	unsigned int pcpu_id = get_cpu();
> -	put_cpu();
> -
>  	/* Resolution logic:
>  	 * 1. There is a valid state with matching selector. Done.
>  	 * 2. Valid state with inappropriate selector. Skip.
> @@ -1381,8 +1375,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  	/* We need the cpu id just as a lookup key,
>  	 * we don't require it to be stable.
>  	 */
> -	pcpu_id = get_cpu();
> -	put_cpu();
> +	pcpu_id = raw_smp_processor_id();

This codepath can be taken from the forwarding path with preemtion
disabled. raw_smp_processor_id will trigger a warning in that case,
maybe better to use raw_cpu_ptr?

Thanks!