| Message ID | 20240729124406.1824592-5-tariqt@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixes for IPsec over bonding | expand |
On 7/29/24 14:44, Tariq Toukan wrote: > From: Jianbo Liu <jianbol@nvidia.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 spin lock and such xfrmdev ops may sleep, minor nit: missing 'a' here ^^ > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 763d807be311..bced29813478 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -439,38 +439,33 @@ static int bond_ipsec_add_sa(struct xfrm_state *xs, > rcu_read_lock(); > bond = netdev_priv(bond_dev); > slave = rcu_dereference(bond->curr_active_slave); Is even this caller always under RTNL lock? if so it would be better replace rcu_dereference() with rtnl_dereference() and drop the rcu lock, so it's clear that real_dev can't go away here. Similar question for bond_ipsec_delete_sa, below. > - if (!slave) { > - rcu_read_unlock(); > + real_dev = slave ? slave->dev : NULL; > + rcu_read_unlock(); > + if (!real_dev) > return -ENODEV; > - } > > - real_dev = slave->dev; > if (!real_dev->xfrmdev_ops || > !real_dev->xfrmdev_ops->xdo_dev_state_add || > netif_is_bond_master(real_dev)) { > NL_SET_ERR_MSG_MOD(extack, "Slave does not support ipsec offload"); > - rcu_read_unlock(); > return -EINVAL; > } > > ipsec = kmalloc(sizeof(*ipsec), GFP_ATOMIC); I guess at this point you can use GFP_KERNEL here. [...] > @@ -482,34 +477,44 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) > 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; > + rcu_read_unlock(); You can drop the rcu read lock/unlock here. [...] > @@ -569,14 +574,13 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) > struct slave *slave; > > rcu_read_lock(); > - slave = rcu_dereference(bond->curr_active_slave); > - if (!slave) { > - rcu_read_unlock(); > + slave = rtnl_dereference(bond->curr_active_slave); > + real_dev = slave ? slave->dev : NULL; > + rcu_read_unlock(); Same here. Thanks, Paolo
On Tue, 2024-07-30 at 13:28 +0200, Paolo Abeni wrote: > > > On 7/29/24 14:44, Tariq Toukan wrote: > > From: Jianbo Liu <jianbol@nvidia.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 spin lock and such xfrmdev ops may > > sleep, > > minor nit: missing 'a' here ^^ OK, thanks. > > > diff --git a/drivers/net/bonding/bond_main.c > > b/drivers/net/bonding/bond_main.c > > index 763d807be311..bced29813478 100644 > > --- a/drivers/net/bonding/bond_main.c > > +++ b/drivers/net/bonding/bond_main.c > > @@ -439,38 +439,33 @@ static int bond_ipsec_add_sa(struct > > xfrm_state *xs, > > rcu_read_lock(); > > bond = netdev_priv(bond_dev); > > slave = rcu_dereference(bond->curr_active_slave); > > Is even this caller always under RTNL lock? if so it would be better > replace rcu_dereference() with rtnl_dereference() and drop the rcu > lock, so it's clear that real_dev can't go away here. > > Similar question for bond_ipsec_delete_sa, below. > No, I don't think they are called under RTNL lock. > > - if (!slave) { > > - rcu_read_unlock(); > > + real_dev = slave ? slave->dev : NULL; > > + rcu_read_unlock(); > > + if (!real_dev) > > return -ENODEV; > > - } > > > > - real_dev = slave->dev; > > if (!real_dev->xfrmdev_ops || > > !real_dev->xfrmdev_ops->xdo_dev_state_add || > > netif_is_bond_master(real_dev)) { > > NL_SET_ERR_MSG_MOD(extack, "Slave does not support > > ipsec offload"); > > - rcu_read_unlock(); > > return -EINVAL; > > } > > > > ipsec = kmalloc(sizeof(*ipsec), GFP_ATOMIC); > > I guess at this point you can use GFP_KERNEL here. > Good point, thanks. > [...] > > @@ -482,34 +477,44 @@ static void bond_ipsec_add_sa_all(struct > > bonding *bond) > > 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; > > + rcu_read_unlock(); > > You can drop the rcu read lock/unlock here. Yes, I will drop rcu read lock/unlock for these 4 functions. > > [...] > > @@ -569,14 +574,13 @@ static void bond_ipsec_del_sa_all(struct > > bonding *bond) > > struct slave *slave; > > > > rcu_read_lock(); > > - slave = rcu_dereference(bond->curr_active_slave); > > - if (!slave) { > > - rcu_read_unlock(); > > + slave = rtnl_dereference(bond->curr_active_slave); > > + real_dev = slave ? slave->dev : NULL; > > + rcu_read_unlock(); > > Same here. > > Thanks, > > Paolo >
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 763d807be311..bced29813478 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -439,38 +439,33 @@ static int bond_ipsec_add_sa(struct xfrm_state *xs, rcu_read_lock(); bond = netdev_priv(bond_dev); slave = rcu_dereference(bond->curr_active_slave); - if (!slave) { - rcu_read_unlock(); + real_dev = slave ? slave->dev : NULL; + rcu_read_unlock(); + if (!real_dev) return -ENODEV; - } - real_dev = slave->dev; if (!real_dev->xfrmdev_ops || !real_dev->xfrmdev_ops->xdo_dev_state_add || netif_is_bond_master(real_dev)) { NL_SET_ERR_MSG_MOD(extack, "Slave does not support ipsec offload"); - rcu_read_unlock(); return -EINVAL; } ipsec = kmalloc(sizeof(*ipsec), GFP_ATOMIC); - if (!ipsec) { - rcu_read_unlock(); + if (!ipsec) return -ENOMEM; - } xs->xso.real_dev = real_dev; err = real_dev->xfrmdev_ops->xdo_dev_state_add(xs, extack); if (!err) { ipsec->xs = xs; INIT_LIST_HEAD(&ipsec->list); - spin_lock_bh(&bond->ipsec_lock); + mutex_lock(&bond->ipsec_lock); list_add(&ipsec->list, &bond->ipsec_list); - spin_unlock_bh(&bond->ipsec_lock); + mutex_unlock(&bond->ipsec_lock); } else { kfree(ipsec); } - rcu_read_unlock(); return err; } @@ -482,34 +477,44 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) 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; + rcu_read_unlock(); + 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; + + 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); } /** @@ -530,6 +535,8 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs) rcu_read_lock(); bond = netdev_priv(bond_dev); slave = rcu_dereference(bond->curr_active_slave); + real_dev = slave ? slave->dev : NULL; + rcu_read_unlock(); if (!slave) goto out; @@ -537,7 +544,6 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs) if (!xs->xso.real_dev) goto out; - real_dev = slave->dev; WARN_ON(xs->xso.real_dev != real_dev); if (!real_dev->xfrmdev_ops || @@ -549,7 +555,7 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs) real_dev->xfrmdev_ops->xdo_dev_state_delete(xs); out: - spin_lock_bh(&bond->ipsec_lock); + mutex_lock(&bond->ipsec_lock); list_for_each_entry(ipsec, &bond->ipsec_list, list) { if (ipsec->xs == xs) { list_del(&ipsec->list); @@ -557,8 +563,7 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs) break; } } - spin_unlock_bh(&bond->ipsec_lock); - rcu_read_unlock(); + mutex_unlock(&bond->ipsec_lock); } static void bond_ipsec_del_sa_all(struct bonding *bond) @@ -569,14 +574,13 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) struct slave *slave; rcu_read_lock(); - slave = rcu_dereference(bond->curr_active_slave); - if (!slave) { - rcu_read_unlock(); + slave = rtnl_dereference(bond->curr_active_slave); + real_dev = slave ? slave->dev : NULL; + rcu_read_unlock(); + if (!real_dev) return; - } - real_dev = slave->dev; - spin_lock_bh(&bond->ipsec_lock); + mutex_lock(&bond->ipsec_lock); list_for_each_entry(ipsec, &bond->ipsec_list, list) { if (!ipsec->xs->xso.real_dev) continue; @@ -594,8 +598,7 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) } ipsec->xs->xso.real_dev = NULL; } - spin_unlock_bh(&bond->ipsec_lock); - rcu_read_unlock(); + mutex_unlock(&bond->ipsec_lock); } static void bond_ipsec_free_sa(struct xfrm_state *xs) @@ -5902,7 +5905,7 @@ void bond_setup(struct net_device *bond_dev) /* set up xfrm device ops (only supported in active-backup right now) */ bond_dev->xfrmdev_ops = &bond_xfrmdev_ops; INIT_LIST_HEAD(&bond->ipsec_list); - spin_lock_init(&bond->ipsec_lock); + mutex_init(&bond->ipsec_lock); #endif /* CONFIG_XFRM_OFFLOAD */ /* don't acquire bond device's netif_tx_lock when transmitting */ @@ -5951,6 +5954,10 @@ static void bond_uninit(struct net_device *bond_dev) __bond_release_one(bond_dev, slave->dev, true, true); netdev_info(bond_dev, "Released all slaves\n"); +#ifdef CONFIG_XFRM_OFFLOAD + mutex_destroy(&bond->ipsec_lock); +#endif /* CONFIG_XFRM_OFFLOAD */ + bond_set_slave_arr(bond, NULL, NULL); list_del_rcu(&bond->bond_list); diff --git a/include/net/bonding.h b/include/net/bonding.h index b61fb1aa3a56..8bb5f016969f 100644 --- a/include/net/bonding.h +++ b/include/net/bonding.h @@ -260,7 +260,7 @@ struct bonding { #ifdef CONFIG_XFRM_OFFLOAD struct list_head ipsec_list; /* protecting ipsec_list */ - spinlock_t ipsec_lock; + struct mutex ipsec_lock; #endif /* CONFIG_XFRM_OFFLOAD */ struct bpf_prog *xdp_prog; };