| Message ID | 20240805050357.2004888-2-tariqt@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixes for IPsec over bonding | expand |
On Mon, Aug 05, 2024 at 08:03:55AM +0300, Tariq Toukan wrote: > From: Jianbo Liu <jianbol@nvidia.com> > > Add this implementation for bonding, so hardware resources can be > freed after xfrm state is deleted. > > And call it when deleting all SAs from old active real interface. > > 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> > --- > 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(); > + > + 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); Do we need to check netif_is_bond_master(slave->dev) here? Thanks Hangbin
On Tue, 2024-08-06 at 16:45 +0800, Hangbin Liu wrote: > On Mon, Aug 05, 2024 at 08:03:55AM +0300, Tariq Toukan wrote: > > From: Jianbo Liu <jianbol@nvidia.com> > > > > Add this implementation for bonding, so hardware resources can be > > freed after xfrm state is deleted. > > > > And call it when deleting all SAs from old active real interface. > > > > 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> > > --- > > 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(); > > + > > + 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); > > Do we need to check netif_is_bond_master(slave->dev) here? Probably no need, because we will not call xdo_dev_state_free only. It is called after xdo_dev_state_delete. > > Thanks > Hangbin
On Mon, Aug 05, 2024 at 08:03:55AM +0300, Tariq Toukan wrote: > From: Jianbo Liu <jianbol@nvidia.com> > > Add this implementation for bonding, so hardware resources can be > freed after xfrm state is deleted. > > And call it when deleting all SAs from old active real interface. > > 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> > --- > 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(); > + > + 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 */ > -- > 2.44.0 > Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
On Mon, 5 Aug 2024 08:03:55 +0300 Tariq Toukan wrote: > +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; can xs->xso.dev be NULL during the dev_free_state callback? > + rcu_read_lock(); > + bond = netdev_priv(bond_dev); > + slave = rcu_dereference(bond->curr_active_slave); > + real_dev = slave ? slave->dev : NULL; > + rcu_read_unlock(); What's holding onto real_dev once you drop the rcu lock here?
On Mon, 2024-08-12 at 17:48 -0700, Jakub Kicinski wrote: > On Mon, 5 Aug 2024 08:03:55 +0300 Tariq Toukan wrote: > > +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; > > can xs->xso.dev be NULL during the dev_free_state callback? Shouldn't be NULL because bond_ipsec_del_sa is called before and xs is removed from bond->ipsec_list, so xs->xso.dev is kept unless it's cleared in dev's xdo_dev_state_delete callback. > > > + rcu_read_lock(); > > + bond = netdev_priv(bond_dev); > > + slave = rcu_dereference(bond->curr_active_slave); > > + real_dev = slave ? slave->dev : NULL; > > + rcu_read_unlock(); > > What's holding onto real_dev once you drop the rcu lock here? I think it should be xfrm state (and bond device). Thanks! Jianbo
On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote: > > > + rcu_read_lock(); > > > + bond = netdev_priv(bond_dev); > > > + slave = rcu_dereference(bond->curr_active_slave); > > > + real_dev = slave ? slave->dev : NULL; > > > + rcu_read_unlock(); > > > > What's holding onto real_dev once you drop the rcu lock here? > > I think it should be xfrm state (and bond device). Please explain it in the commit message in more certain terms.
On Tue, 2024-08-13 at 07:14 -0700, Jakub Kicinski wrote: > On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote: > > > > + rcu_read_lock(); > > > > + bond = netdev_priv(bond_dev); > > > > + slave = rcu_dereference(bond->curr_active_slave); > > > > + real_dev = slave ? slave->dev : NULL; > > > > + rcu_read_unlock(); > > > > > > What's holding onto real_dev once you drop the rcu lock here? > > > > I think it should be xfrm state (and bond device). > > Please explain it in the commit message in more certain terms. Sorry, I don't understand. The real_dev is saved in xs->xso.real_dev, and also bond's slave. It's straightforward. What else do I need to explain? Thanks! Jianbo
On Wed, Aug 14, 2024 at 02:03:58AM +0000, Jianbo Liu wrote: > On Tue, 2024-08-13 at 07:14 -0700, Jakub Kicinski wrote: > > On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote: > > > > > + rcu_read_lock(); > > > > > + bond = netdev_priv(bond_dev); > > > > > + slave = rcu_dereference(bond->curr_active_slave); > > > > > + real_dev = slave ? slave->dev : NULL; > > > > > + rcu_read_unlock(); > > > > > > > > What's holding onto real_dev once you drop the rcu lock here? > > > > > > I think it should be xfrm state (and bond device). > > > > Please explain it in the commit message in more certain terms. > > Sorry, I don't understand. The real_dev is saved in xs->xso.real_dev, > and also bond's slave. It's straightforward. What else do I need to > explain? I think Jakub means you need to make sure the real_dev is not freed during xfrmdev_ops. See bond_ipsec_add_sa(). You unlock it too early and later xfrmdev_ops is not protected. Hangbin
On Wed, 2024-08-14 at 11:11 +0800, Hangbin Liu wrote: > On Wed, Aug 14, 2024 at 02:03:58AM +0000, Jianbo Liu wrote: > > On Tue, 2024-08-13 at 07:14 -0700, Jakub Kicinski wrote: > > > On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote: > > > > > > + rcu_read_lock(); > > > > > > + bond = netdev_priv(bond_dev); > > > > > > + slave = rcu_dereference(bond->curr_active_slave); > > > > > > + real_dev = slave ? slave->dev : NULL; > > > > > > + rcu_read_unlock(); > > > > > > > > > > What's holding onto real_dev once you drop the rcu lock > > > > > here? > > > > > > > > I think it should be xfrm state (and bond device). > > > > > > Please explain it in the commit message in more certain terms. > > > > Sorry, I don't understand. The real_dev is saved in xs- > > >xso.real_dev, > > and also bond's slave. It's straightforward. What else do I need to > > explain? > > I think Jakub means you need to make sure the real_dev is not freed > during > xfrmdev_ops. See bond_ipsec_add_sa(). You unlock it too early and > later > xfrmdev_ops is not protected. This RCU lock is to protect the reading of curr_active_slave, which is pointing to a big stuct - slave struct, so there is no error to get real_dev from slave->dev. Thanks! Jianbo
On Thu, Aug 15, 2024 at 01:23:05AM +0000, Jianbo Liu wrote: > On Wed, 2024-08-14 at 11:11 +0800, Hangbin Liu wrote: > > On Wed, Aug 14, 2024 at 02:03:58AM +0000, Jianbo Liu wrote: > > > On Tue, 2024-08-13 at 07:14 -0700, Jakub Kicinski wrote: > > > > On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote: > > > > > > > + rcu_read_lock(); > > > > > > > + bond = netdev_priv(bond_dev); > > > > > > > + slave = rcu_dereference(bond->curr_active_slave); > > > > > > > + real_dev = slave ? slave->dev : NULL; > > > > > > > + rcu_read_unlock(); > > > > > > > > > > > > What's holding onto real_dev once you drop the rcu lock > > > > > > here? > > > > > > > > > > I think it should be xfrm state (and bond device). > > > > > > > > Please explain it in the commit message in more certain terms. > > > > > > Sorry, I don't understand. The real_dev is saved in xs- > > > >xso.real_dev, > > > and also bond's slave. It's straightforward. What else do I need to > > > explain? > > > > I think Jakub means you need to make sure the real_dev is not freed > > during > > xfrmdev_ops. See bond_ipsec_add_sa(). You unlock it too early and > > later > > xfrmdev_ops is not protected. > > This RCU lock is to protect the reading of curr_active_slave, which is > pointing to a big stuct - slave struct, so there is no error to get > real_dev from slave->dev. It's not about getting real_dev from slave->dev. As Jakub said, What's holding on real_dev once you drop the rcu lock? Thanks Hangbin
On Thu, 2024-08-15 at 11:34 +0800, Hangbin Liu wrote: > On Thu, Aug 15, 2024 at 01:23:05AM +0000, Jianbo Liu wrote: > > On Wed, 2024-08-14 at 11:11 +0800, Hangbin Liu wrote: > > > On Wed, Aug 14, 2024 at 02:03:58AM +0000, Jianbo Liu wrote: > > > > On Tue, 2024-08-13 at 07:14 -0700, Jakub Kicinski wrote: > > > > > On Tue, 13 Aug 2024 02:58:12 +0000 Jianbo Liu wrote: > > > > > > > > + rcu_read_lock(); > > > > > > > > + bond = netdev_priv(bond_dev); > > > > > > > > + slave = rcu_dereference(bond- > > > > > > > > >curr_active_slave); > > > > > > > > + real_dev = slave ? slave->dev : NULL; > > > > > > > > + rcu_read_unlock(); > > > > > > > > > > > > > > What's holding onto real_dev once you drop the rcu lock > > > > > > > here? > > > > > > > > > > > > I think it should be xfrm state (and bond device). > > > > > > > > > > Please explain it in the commit message in more certain > > > > > terms. > > > > > > > > Sorry, I don't understand. The real_dev is saved in xs- > > > > > xso.real_dev, > > > > and also bond's slave. It's straightforward. What else do I > > > > need to > > > > explain? > > > > > > I think Jakub means you need to make sure the real_dev is not > > > freed > > > during > > > xfrmdev_ops. See bond_ipsec_add_sa(). You unlock it too early and > > > later > > > xfrmdev_ops is not protected. > > > > This RCU lock is to protect the reading of curr_active_slave, which > > is > > pointing to a big stuct - slave struct, so there is no error to get > > real_dev from slave->dev. > > It's not about getting real_dev from slave->dev. As Jakub said, > What's holding > on real_dev once you drop the rcu lock? > As you mentioned the lock, I explained what's it used for, so we will not mix basic concepts and make things complicated. As for Jakub's question, I already answered. And I'm waiting for his reply so I can better undestand how to modify if there is any. 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 */