linux-kernel - Re: [PATCHv2 net 1/3] bonding: move mutex lock to a work queue for XFRM GC tasks

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] [day] [month] [year] [list]

Message-ID: <f0dbb1cfdda52597b068aa785a876b48020ad975.camel@nvidia.com>
Date: Wed, 26 Feb 2025 14:05:21 +0000
From: Cosmin Ratiu <cratiu@...dia.com>
To: "liuhangbin@...il.com" <liuhangbin@...il.com>
CC: "shuah@...nel.org" <shuah@...nel.org>, "andrew+netdev@...n.ch"
	<andrew+netdev@...n.ch>, "davem@...emloft.net" <davem@...emloft.net>, Jianbo
 Liu <jianbol@...dia.com>, "jarod@...hat.com" <jarod@...hat.com>,
	"razor@...ckwall.org" <razor@...ckwall.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "jv@...sburgh.net"
	<jv@...sburgh.net>, "horms@...nel.org" <horms@...nel.org>, "kuba@...nel.org"
	<kuba@...nel.org>, Tariq Toukan <tariqt@...dia.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "steffen.klassert@...unet.com"
	<steffen.klassert@...unet.com>, "linux-kselftest@...r.kernel.org"
	<linux-kselftest@...r.kernel.org>
Subject: Re: [PATCHv2 net 1/3] bonding: move mutex lock to a work queue for
 XFRM GC tasks

On Wed, 2025-02-26 at 12:07 +0000, Hangbin Liu wrote:
> 
> During bonding testing, we also found a case that would trigger
> the WARN_ON(xs->xso.real_dev != real_dev).
> 
> If we create active-backup mode bonding and create ipsec tunnel over
> bonding device, then remove bonding device. There is a possibility
> that
> the bond call bond_ipsec_del_sa_all() to delete the ipsec state
> first,
> then change active slave to another interface.
> 
> At the same time, ipsec gc was called and then bond_ipsec_free_sa().
> This will cause the xs->xso.real_dev != active_slave as the failover
> triggered. The call traces looks like:
> [..]
> 
> This seems like another situation that could not simply fit
> 3. "if (xs->xso.real_dev != real_dev), goto out.
> I'm not sure what's the xs->km.state should be during
> xfrm_state_gc_task().
> Is it also set to XFRM_STATE_DEAD, because I didn't see it.

XFRM_STATE_DEAD is set in __xfrm_state_delete() (and other places for
what seems like error conditions), plus there's a WARN_ON(x->km.state
!= XFRM_STATE_DEAD) in __xfrm_state_destroy(). This last function is
the main way xfrm states are destroyed, besides xfrm_dev_state_flush
and xfrm_state_find (where xfrm_state_delete + xfrm_dev_state_free are
used directly).
So I am pretty sure that when bond .xdo_dev_state_free() is called via
either one of the above three mechanisms, the state should be
XFRM_STATE_DEAD. But maybe I'm missing something.

> 
> Especially if the bond change active slave and xfrm_state_gc_task()
> run
> in parallel, like
> 
>   bond_ipsec_del_sa_all()
>                                   xfrm_state_gc_task()
>                                   xfrm_dev_state_free()
>                                   bond_ipsec_free_sa()
>   bond_ipsec_add_sa_all()
> 
> If the xs->km.state is not XFRM_STATE_DEAD. How to avoid the
> WARN_ON(xs->xso.real_dev != real_dev) in bond_ipsec_free_sa()
> and how to make bond_ipsec_add_sa_all() not added the entry again.

I am proposing you change this WARN_ON to an if, avoid calling
xdo_dev_state_free on real_dev in that case and just remove the entry
from bond->ipsec.

Cosmin.