| Message ID | 20240815142103.2253886-2-tariqt@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixes for IPsec over bonding | expand |
On Thu, Aug 15, 2024 at 05:21:01PM +0300, Tariq Toukan wrote: > From: Jianbo Liu <jianbol@nvidia.com> > > Add this implementation for bonding, so hardware resources can be > freed from the active slave after xfrm state is deleted. The netdev > used to invoke xdo_dev_state_free callback, is saved in the xfrm state > (xs->xso.real_dev), which is also the bond's active slave. > > And call it when deleting all SAs from old active real interface while > switching current active slave. > > Fixes: 9a5605505d9c ("bonding: Add struct bond_ipesc to manage SA") > Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > Signed-off-by: Tariq Toukan <tariqt@nvidia.com> > Reviewed-by: Hangbin Liu <liuhangbin@gmail.com> > --- > drivers/net/bonding/bond_main.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 1cd92c12e782..eb5e43860670 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -581,6 +581,8 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) > __func__); > } else { > slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs); > + if (slave->dev->xfrmdev_ops->xdo_dev_state_free) > + slave->dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs); > } > ipsec->xs->xso.real_dev = NULL; > } > @@ -588,6 +590,35 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) > rcu_read_unlock(); > } > > +static void bond_ipsec_free_sa(struct xfrm_state *xs) > +{ > + struct net_device *bond_dev = xs->xso.dev; > + struct net_device *real_dev; > + struct bonding *bond; > + struct slave *slave; > + > + if (!bond_dev) > + return; > + > + rcu_read_lock(); > + bond = netdev_priv(bond_dev); > + slave = rcu_dereference(bond->curr_active_slave); > + real_dev = slave ? slave->dev : NULL; > + rcu_read_unlock(); As I replied in https://lore.kernel.org/netdev/ZrwgRaDc1Vo0Jhcj@Laptop-X1/, > + > + if (!slave) > + return; > + > + if (!xs->xso.real_dev) > + return; > + > + WARN_ON(xs->xso.real_dev != real_dev); > + > + if (real_dev && real_dev->xfrmdev_ops && > + real_dev->xfrmdev_ops->xdo_dev_state_free) > + real_dev->xfrmdev_ops->xdo_dev_state_free(xs); How do you make sure the slave not freed after rcu_read_unlock()? Thanks Hangbin
On Fri, 2024-08-16 at 11:07 +0800, Hangbin Liu wrote: > On Thu, Aug 15, 2024 at 05:21:01PM +0300, Tariq Toukan wrote: > > From: Jianbo Liu <jianbol@nvidia.com> > > > > Add this implementation for bonding, so hardware resources can be > > freed from the active slave after xfrm state is deleted. The netdev > > used to invoke xdo_dev_state_free callback, is saved in the xfrm > > state > > (xs->xso.real_dev), which is also the bond's active slave. > > > > And call it when deleting all SAs from old active real interface > > while > > switching current active slave. > > > > Fixes: 9a5605505d9c ("bonding: Add struct bond_ipesc to manage SA") > > Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > > Signed-off-by: Tariq Toukan <tariqt@nvidia.com> > > Reviewed-by: Hangbin Liu <liuhangbin@gmail.com> > > --- > > drivers/net/bonding/bond_main.c | 32 > > ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/drivers/net/bonding/bond_main.c > > b/drivers/net/bonding/bond_main.c > > index 1cd92c12e782..eb5e43860670 100644 > > --- a/drivers/net/bonding/bond_main.c > > +++ b/drivers/net/bonding/bond_main.c > > @@ -581,6 +581,8 @@ static void bond_ipsec_del_sa_all(struct > > bonding *bond) > > __func__); > > } else { > > slave->dev->xfrmdev_ops- > > >xdo_dev_state_delete(ipsec->xs); > > + if (slave->dev->xfrmdev_ops- > > >xdo_dev_state_free) > > + slave->dev->xfrmdev_ops- > > >xdo_dev_state_free(ipsec->xs); > > } > > ipsec->xs->xso.real_dev = NULL; > > } > > @@ -588,6 +590,35 @@ static void bond_ipsec_del_sa_all(struct > > bonding *bond) > > rcu_read_unlock(); > > } > > > > +static void bond_ipsec_free_sa(struct xfrm_state *xs) > > +{ > > + struct net_device *bond_dev = xs->xso.dev; > > + struct net_device *real_dev; > > + struct bonding *bond; > > + struct slave *slave; > > + > > + if (!bond_dev) > > + return; > > + > > + rcu_read_lock(); > > + bond = netdev_priv(bond_dev); > > + slave = rcu_dereference(bond->curr_active_slave); > > + real_dev = slave ? slave->dev : NULL; > > + rcu_read_unlock(); > > As I replied in > https://lore.kernel.org/netdev/ZrwgRaDc1Vo0Jhcj@Laptop-X1/, > As I replied, the RCU lock is to protect the reading of the content pointed by curr_active_slave, not the slave->dev itself. > > + > > + if (!slave) > > + return; > > + > > + if (!xs->xso.real_dev) > > + return; > > + > > + WARN_ON(xs->xso.real_dev != real_dev); > > + > > + if (real_dev && real_dev->xfrmdev_ops && > > + real_dev->xfrmdev_ops->xdo_dev_state_free) > > + real_dev->xfrmdev_ops->xdo_dev_state_free(xs); > > How do you make sure the slave not freed after rcu_read_unlock()? > So do you want to move rcu_read_unlock after xdo_dev_state_free, just like what you did in your patches? Thanks! Jianbo
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 1cd92c12e782..eb5e43860670 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -581,6 +581,8 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) __func__); } else { slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs); + if (slave->dev->xfrmdev_ops->xdo_dev_state_free) + slave->dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs); } ipsec->xs->xso.real_dev = NULL; } @@ -588,6 +590,35 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) rcu_read_unlock(); } +static void bond_ipsec_free_sa(struct xfrm_state *xs) +{ + struct net_device *bond_dev = xs->xso.dev; + struct net_device *real_dev; + struct bonding *bond; + struct slave *slave; + + if (!bond_dev) + return; + + 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) + return; + + if (!xs->xso.real_dev) + return; + + WARN_ON(xs->xso.real_dev != real_dev); + + if (real_dev && real_dev->xfrmdev_ops && + real_dev->xfrmdev_ops->xdo_dev_state_free) + real_dev->xfrmdev_ops->xdo_dev_state_free(xs); +} + /** * bond_ipsec_offload_ok - can this packet use the xfrm hw offload * @skb: current data packet @@ -632,6 +663,7 @@ static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs) static const struct xfrmdev_ops bond_xfrmdev_ops = { .xdo_dev_state_add = bond_ipsec_add_sa, .xdo_dev_state_delete = bond_ipsec_del_sa, + .xdo_dev_state_free = bond_ipsec_free_sa, .xdo_dev_offload_ok = bond_ipsec_offload_ok, }; #endif /* CONFIG_XFRM_OFFLOAD */