| 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
| ||
|
Message-ID: <0ed0935e51c244086529a43aa6ccf599e5b3bc52.camel@nvidia.com>
Date: Thu, 8 Aug 2024 10:05:26 +0000
From: Jianbo Liu <jianbol@...dia.com>
To: Tariq Toukan <tariqt@...dia.com>, "liuhangbin@...il.com"
<liuhangbin@...il.com>
CC: "davem@...emloft.net" <davem@...emloft.net>, Leon Romanovsky
<leonro@...dia.com>, "andy@...yhouse.net" <andy@...yhouse.net>, Gal Pressman
<gal@...dia.com>, "jv@...sburgh.net" <jv@...sburgh.net>, "kuba@...nel.org"
<kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
"edumazet@...gle.com" <edumazet@...gle.com>, Saeed Mahameed
<saeedm@...dia.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net V3 3/3] bonding: change ipsec_lock from spin lock to
mutex
On Thu, 2024-08-08 at 17:34 +0800, Hangbin Liu wrote:
> On Mon, Aug 05, 2024 at 08:03:57AM +0300, Tariq Toukan wrote:
> > From: Jianbo Liu <jianbol@...dia.com>
> >
> > In the cited commit, bond->ipsec_lock is added to protect
> > ipsec_list,
> > hence xdo_dev_state_add and xdo_dev_state_delete are called inside
> > this lock. As ipsec_lock is a spin lock and such xfrmdev ops may
> > sleep,
> > "scheduling while atomic" will be triggered when changing bond's
> > active slave.
> >
> > [ 101.055189] BUG: scheduling while atomic: bash/902/0x00000200
> > [ 101.055726] Modules linked in:
> > [ 101.058211] CPU: 3 PID: 902 Comm: bash Not tainted 6.9.0-rc4+ #1
> > [ 101.058760] Hardware name:
> > [ 101.059434] Call Trace:
> > [ 101.059436] <TASK>
> > [ 101.060873] dump_stack_lvl+0x51/0x60
> > [ 101.061275] __schedule_bug+0x4e/0x60
> > [ 101.061682] __schedule+0x612/0x7c0
> > [ 101.062078] ? __mod_timer+0x25c/0x370
> > [ 101.062486] schedule+0x25/0xd0
> > [ 101.062845] schedule_timeout+0x77/0xf0
> > [ 101.063265] ? asm_common_interrupt+0x22/0x40
> > [ 101.063724] ? __bpf_trace_itimer_state+0x10/0x10
> > [ 101.064215] __wait_for_common+0x87/0x190
> > [ 101.064648] ? usleep_range_state+0x90/0x90
> > [ 101.065091] cmd_exec+0x437/0xb20 [mlx5_core]
> > [ 101.065569] mlx5_cmd_do+0x1e/0x40 [mlx5_core]
> > [ 101.066051] mlx5_cmd_exec+0x18/0x30 [mlx5_core]
> > [ 101.066552] mlx5_crypto_create_dek_key+0xea/0x120 [mlx5_core]
> > [ 101.067163] ? bonding_sysfs_store_option+0x4d/0x80 [bonding]
> > [ 101.067738] ? kmalloc_trace+0x4d/0x350
> > [ 101.068156] mlx5_ipsec_create_sa_ctx+0x33/0x100 [mlx5_core]
> > [ 101.068747] mlx5e_xfrm_add_state+0x47b/0xaa0 [mlx5_core]
> > [ 101.069312] bond_change_active_slave+0x392/0x900 [bonding]
> > [ 101.069868] bond_option_active_slave_set+0x1c2/0x240 [bonding]
> > [ 101.070454] __bond_opt_set+0xa6/0x430 [bonding]
> > [ 101.070935] __bond_opt_set_notify+0x2f/0x90 [bonding]
> > [ 101.071453] bond_opt_tryset_rtnl+0x72/0xb0 [bonding]
> > [ 101.071965] bonding_sysfs_store_option+0x4d/0x80 [bonding]
> > [ 101.072567] kernfs_fop_write_iter+0x10c/0x1a0
> > [ 101.073033] vfs_write+0x2d8/0x400
> > [ 101.073416] ? alloc_fd+0x48/0x180
> > [ 101.073798] ksys_write+0x5f/0xe0
> > [ 101.074175] do_syscall_64+0x52/0x110
> > [ 101.074576] entry_SYSCALL_64_after_hwframe+0x4b/0x53
> >
> > As bond_ipsec_add_sa_all and bond_ipsec_del_sa_all are only called
> > from bond_change_active_slave, which requires holding the RTNL
> > lock.
> > And bond_ipsec_add_sa and bond_ipsec_del_sa are xfrm state
> > xdo_dev_state_add and xdo_dev_state_delete APIs, which are in user
> > context. So ipsec_lock doesn't have to be spin lock, change it to
> > mutex, and thus the above issue can be resolved.
> >
> > Fixes: 9a5605505d9c ("bonding: Add struct bond_ipesc to manage SA")
> > Signed-off-by: Jianbo Liu <jianbol@...dia.com>
> > Signed-off-by: Tariq Toukan <tariqt@...dia.com>
> > ---
> > drivers/net/bonding/bond_main.c | 75 +++++++++++++++++------------
> > ----
> > include/net/bonding.h | 2 +-
> > 2 files changed, 40 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/net/bonding/bond_main.c
> > b/drivers/net/bonding/bond_main.c
> > index e550b1c08fdb..56764f1c39b8 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -481,35 +476,43 @@ static void bond_ipsec_add_sa_all(struct
> > bonding *bond)
> > struct bond_ipsec *ipsec;
> > struct slave *slave;
> >
> > - rcu_read_lock();
> > - slave = rcu_dereference(bond->curr_active_slave);
> > - if (!slave)
> > - goto out;
> > + slave = rtnl_dereference(bond->curr_active_slave);
> > + real_dev = slave ? slave->dev : NULL;
> > + if (!real_dev)
> > + return;
> >
> > - real_dev = slave->dev;
> > + mutex_lock(&bond->ipsec_lock);
> > if (!real_dev->xfrmdev_ops ||
> > !real_dev->xfrmdev_ops->xdo_dev_state_add ||
> > netif_is_bond_master(real_dev)) {
> > - spin_lock_bh(&bond->ipsec_lock);
> > if (!list_empty(&bond->ipsec_list))
> > slave_warn(bond_dev, real_dev,
> > "%s: no slave
> > xdo_dev_state_add\n",
> > __func__);
> > - spin_unlock_bh(&bond->ipsec_lock);
> > goto out;
> > }
> >
> > - spin_lock_bh(&bond->ipsec_lock);
> > list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> > + struct net_device *dev = ipsec->xs->xso.real_dev;
> > +
> > + /* If new state is added before ipsec_lock acquired
> > */
> > + if (dev) {
> > + if (dev == real_dev)
> > + continue;
> Hi Jianbo,
>
> Why we skip the deleting here if dev == real_dev? What if the state
Here the bond active slave is updated. If dev == real_dev, the state
(should be newly added) is offloaded to new active, so no need to
delete and add back again.
> is added again on the same slave? From the previous logic it looks we
Why is it added to the same slave? It's not the active one.
> don't check and do over write for the same device.
>
> Thanks
> Hangbin
> > + dev->xfrmdev_ops-
> > >xdo_dev_state_delete(ipsec->xs);
> > + if (dev->xfrmdev_ops->xdo_dev_state_free)
> > + dev->xfrmdev_ops-
> > >xdo_dev_state_free(ipsec->xs);
> > + }
> > +
> > ipsec->xs->xso.real_dev = real_dev;
> > if (real_dev->xfrmdev_ops->xdo_dev_state_add(ipsec-
> > >xs, NULL)) {
> > slave_warn(bond_dev, real_dev, "%s: failed
> > to add SA\n", __func__);
> > ipsec->xs->xso.real_dev = NULL;
> > }
> > }
> > - spin_unlock_bh(&bond->ipsec_lock);
> > out:
> > - rcu_read_unlock();
> > + mutex_unlock(&bond->ipsec_lock);
> > }
Powered by blists - more mailing lists