| Message ID | 20240820004840.510412-3-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Bonding: support new xfrm state offload functions | expand |
Hi Hangbin, (adding Steffen since we're getting a bit into details of IPsec) 2024-08-20, 08:48:39 +0800, Hangbin Liu wrote: > Currently, users can see that bonding supports IPSec HW offload via ethtool. > However, this functionality does not work with NICs like Mellanox cards when > ESN (Extended Sequence Numbers) is enabled, as ESN functions are not yet > supported. This patch adds ESN support to the bonding IPSec device offload, > ensuring proper functionality with NICs that support ESN. > > Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > drivers/net/bonding/bond_main.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 560e3416f6f5..24747fceef66 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -651,10 +651,35 @@ static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs) > return err; > } > > +/** > + * bond_advance_esn_state - ESN support for IPSec HW offload > + * @xs: pointer to transformer state struct > + **/ > +static void bond_advance_esn_state(struct xfrm_state *xs) > +{ > + struct net_device *real_dev; > + > + rcu_read_lock(); > + real_dev = bond_ipsec_dev(xs); > + if (!real_dev) > + goto out; > + > + if (!real_dev->xfrmdev_ops || > + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { > + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); xdo_dev_state_advance_esn is called on the receive path for every packet when ESN is enabled (xfrm_input -> xfrm_replay_advance -> xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to be ratelimited. But this CB is required to make offload with ESN work. If it's not implemented on a lower device, I expect things will break. I wonder what the best behavior would be: - just warn (this patch) -- but things will break for users - don't allow mixing devices that support ESN with devices that don't in the same bond (really bad if the user doesn't care about ESN) - don't allow enslaving devices that don't support ESN if an xfrm state using ESN has been added to the bond, and don't allow creating ESN states if the bond has one non-ESN slave - disable re-offloading the state if we have to migrate it from an ESN-enabled to a non-ESN device (when changing the active slave) -- and fall back to the (slow) SW implementation - other options that I'm not thinking about? Sorry I'm only noticing this at v3 :(
On Tue, Aug 20, 2024 at 05:33:58PM +0200, Sabrina Dubroca wrote: > > + if (!real_dev->xfrmdev_ops || > > + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { > > + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); > > xdo_dev_state_advance_esn is called on the receive path for every > packet when ESN is enabled (xfrm_input -> xfrm_replay_advance -> > xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to > be ratelimited. You are right. Warn on adding/deleting is OK. But during packets receiving we need to limit it. > > > But this CB is required to make offload with ESN work. If it's not > implemented on a lower device, I expect things will break. I wonder > what the best behavior would be: > > - just warn (this patch) -- but things will break for users I would prefer this way, which is what we do currently. The warn could let user know the ESN is not supported. They should use ESN supported device or disable it. Thanks Hangbin
On Tue, Aug 20, 2024 at 05:33:58PM +0200, Sabrina Dubroca wrote: > Hi Hangbin, > > (adding Steffen since we're getting a bit into details of IPsec) > > 2024-08-20, 08:48:39 +0800, Hangbin Liu wrote: > > Currently, users can see that bonding supports IPSec HW offload via ethtool. > > However, this functionality does not work with NICs like Mellanox cards when > > ESN (Extended Sequence Numbers) is enabled, as ESN functions are not yet > > supported. This patch adds ESN support to the bonding IPSec device offload, > > ensuring proper functionality with NICs that support ESN. > > > > Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org> > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > --- > > drivers/net/bonding/bond_main.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > index 560e3416f6f5..24747fceef66 100644 > > --- a/drivers/net/bonding/bond_main.c > > +++ b/drivers/net/bonding/bond_main.c > > @@ -651,10 +651,35 @@ static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs) > > return err; > > } > > > > +/** > > + * bond_advance_esn_state - ESN support for IPSec HW offload > > + * @xs: pointer to transformer state struct > > + **/ > > +static void bond_advance_esn_state(struct xfrm_state *xs) > > +{ > > + struct net_device *real_dev; > > + > > + rcu_read_lock(); > > + real_dev = bond_ipsec_dev(xs); > > + if (!real_dev) > > + goto out; > > + > > + if (!real_dev->xfrmdev_ops || > > + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { > > + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); > > xdo_dev_state_advance_esn is called on the receive path for every > packet when ESN is enabled (xfrm_input -> xfrm_replay_advance -> > xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to > be ratelimited. How does xfrm_state offload work on bonding? Does every slave have its own negotiated SA?
On Wed, Aug 21, 2024 at 02:30:41PM +0200, Steffen Klassert wrote: > > > +/** > > > + * bond_advance_esn_state - ESN support for IPSec HW offload > > > + * @xs: pointer to transformer state struct > > > + **/ > > > +static void bond_advance_esn_state(struct xfrm_state *xs) > > > +{ > > > + struct net_device *real_dev; > > > + > > > + rcu_read_lock(); > > > + real_dev = bond_ipsec_dev(xs); > > > + if (!real_dev) > > > + goto out; > > > + > > > + if (!real_dev->xfrmdev_ops || > > > + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { > > > + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); > > > > xdo_dev_state_advance_esn is called on the receive path for every > > packet when ESN is enabled (xfrm_input -> xfrm_replay_advance -> > > xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to > > be ratelimited. > > How does xfrm_state offload work on bonding? > Does every slave have its own negotiated SA? Yes and no. Bonding only supports xfrm offload with active-backup mode. So only current active slave keep the SA. When active slave changes, the sa on previous slave is deleted and re-added on new active slave. Thanks Hangbin
2024-08-21, 21:26:00 +0800, Hangbin Liu wrote: > On Wed, Aug 21, 2024 at 02:30:41PM +0200, Steffen Klassert wrote: > > > > +/** > > > > + * bond_advance_esn_state - ESN support for IPSec HW offload > > > > + * @xs: pointer to transformer state struct > > > > + **/ > > > > +static void bond_advance_esn_state(struct xfrm_state *xs) > > > > +{ > > > > + struct net_device *real_dev; > > > > + > > > > + rcu_read_lock(); > > > > + real_dev = bond_ipsec_dev(xs); > > > > + if (!real_dev) > > > > + goto out; > > > > + > > > > + if (!real_dev->xfrmdev_ops || > > > > + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { > > > > + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); > > > > > > xdo_dev_state_advance_esn is called on the receive path for every > > > packet when ESN is enabled (xfrm_input -> xfrm_replay_advance -> > > > xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to > > > be ratelimited. > > > > How does xfrm_state offload work on bonding? > > Does every slave have its own negotiated SA? > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only > current active slave keep the SA. When active slave changes, the sa on > previous slave is deleted and re-added on new active slave. It's the same SA, there's no DELSA+NEWSA when we change the active slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform the driver/HW), and only a single NEWSA to install the offloaded SA on the bond device (which calls the active slave's xdo_dev_state_add).
On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote: > 2024-08-21, 21:26:00 +0800, Hangbin Liu wrote: > > On Wed, Aug 21, 2024 at 02:30:41PM +0200, Steffen Klassert wrote: > > > > > +/** > > > > > + * bond_advance_esn_state - ESN support for IPSec HW offload > > > > > + * @xs: pointer to transformer state struct > > > > > + **/ > > > > > +static void bond_advance_esn_state(struct xfrm_state *xs) > > > > > +{ > > > > > + struct net_device *real_dev; > > > > > + > > > > > + rcu_read_lock(); > > > > > + real_dev = bond_ipsec_dev(xs); > > > > > + if (!real_dev) > > > > > + goto out; > > > > > + > > > > > + if (!real_dev->xfrmdev_ops || > > > > > + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { > > > > > + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); > > > > > > > > xdo_dev_state_advance_esn is called on the receive path for every > > > > packet when ESN is enabled (xfrm_input -> xfrm_replay_advance -> > > > > xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to > > > > be ratelimited. > > > > > > How does xfrm_state offload work on bonding? > > > Does every slave have its own negotiated SA? > > > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only > > current active slave keep the SA. When active slave changes, the sa on > > previous slave is deleted and re-added on new active slave. > > It's the same SA, there's no DELSA+NEWSA when we change the active > slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform > the driver/HW), and only a single NEWSA to install the offloaded SA on > the bond device (which calls the active slave's xdo_dev_state_add). Maybe I miss something, but how is the sequence number, replay window etc. transfered from the old to the new active slave?
2024-08-21, 16:03:01 +0200, Steffen Klassert wrote: > On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote: > > 2024-08-21, 21:26:00 +0800, Hangbin Liu wrote: > > > On Wed, Aug 21, 2024 at 02:30:41PM +0200, Steffen Klassert wrote: > > > > > > +/** > > > > > > + * bond_advance_esn_state - ESN support for IPSec HW offload > > > > > > + * @xs: pointer to transformer state struct > > > > > > + **/ > > > > > > +static void bond_advance_esn_state(struct xfrm_state *xs) > > > > > > +{ > > > > > > + struct net_device *real_dev; > > > > > > + > > > > > > + rcu_read_lock(); > > > > > > + real_dev = bond_ipsec_dev(xs); > > > > > > + if (!real_dev) > > > > > > + goto out; > > > > > > + > > > > > > + if (!real_dev->xfrmdev_ops || > > > > > > + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { > > > > > > + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); > > > > > > > > > > xdo_dev_state_advance_esn is called on the receive path for every > > > > > packet when ESN is enabled (xfrm_input -> xfrm_replay_advance -> > > > > > xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to > > > > > be ratelimited. > > > > > > > > How does xfrm_state offload work on bonding? > > > > Does every slave have its own negotiated SA? > > > > > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only > > > current active slave keep the SA. When active slave changes, the sa on > > > previous slave is deleted and re-added on new active slave. > > > > It's the same SA, there's no DELSA+NEWSA when we change the active > > slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform > > the driver/HW), and only a single NEWSA to install the offloaded SA on > > the bond device (which calls the active slave's xdo_dev_state_add). > > Maybe I miss something, but how is the sequence number, replay window > etc. transfered from the old to the new active slave? With crypto offload, the stack sees the headers so it manages to keep track and update its data, so it should have no problem feeding it back to the next driver? Note that if something in that area is broken, it would be broken regardless of ESN.
On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote: > > > > > + if (!real_dev->xfrmdev_ops || > > > > > + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { > > > > > + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); > > > > > > > > xdo_dev_state_advance_esn is called on the receive path for every > > > > packet when ESN is enabled (xfrm_input -> xfrm_replay_advance -> > > > > xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to > > > > be ratelimited. > > > > > > How does xfrm_state offload work on bonding? > > > Does every slave have its own negotiated SA? > > > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only > > current active slave keep the SA. When active slave changes, the sa on > > previous slave is deleted and re-added on new active slave. > > It's the same SA, there's no DELSA+NEWSA when we change the active > slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform > the driver/HW), and only a single NEWSA to install the offloaded SA on > the bond device (which calls the active slave's xdo_dev_state_add). Yes, thanks for the clarification. The SA is not changed, we just delete it on old active slave slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs); And add to now one. ipsec->xs->xso.real_dev = slave->dev; slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL) Thanks Hangbin
On Thu, Aug 22, 2024 at 08:33:17AM +0800, Hangbin Liu wrote: > On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote: > > > > > > + if (!real_dev->xfrmdev_ops || > > > > > > + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { > > > > > > + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); > > > > > > > > > > xdo_dev_state_advance_esn is called on the receive path for every > > > > > packet when ESN is enabled (xfrm_input -> xfrm_replay_advance -> > > > > > xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to > > > > > be ratelimited. > > > > > > > > How does xfrm_state offload work on bonding? > > > > Does every slave have its own negotiated SA? > > > > > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only > > > current active slave keep the SA. When active slave changes, the sa on > > > previous slave is deleted and re-added on new active slave. > > > > It's the same SA, there's no DELSA+NEWSA when we change the active > > slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform > > the driver/HW), and only a single NEWSA to install the offloaded SA on > > the bond device (which calls the active slave's xdo_dev_state_add). > > Yes, thanks for the clarification. The SA is not changed, we just delete it > on old active slave > > slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs); > > And add to now one. > > ipsec->xs->xso.real_dev = slave->dev; > slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL) Using the same key on two different devices is very dangerous. Counter mode algorithms have the requirement that the IV must not repeat. If you use the same key on two devices, you can't guarantee that. If both devices use an internal counter (initialized to one) to generate the IV, then the IV repeats for the mumber of packets that were already sent on the old device. The algorithm is cryptographically broken in that case. Instead of moving the existing state, it is better to request a rekey. Maybe by setting the old state to 'expired' and then send a km_state_expired() message.
On Wed, Aug 21, 2024 at 07:20:46PM +0200, Sabrina Dubroca wrote: > 2024-08-21, 16:03:01 +0200, Steffen Klassert wrote: > > > > Maybe I miss something, but how is the sequence number, replay window > > etc. transfered from the old to the new active slave? > > With crypto offload, the stack sees the headers so it manages to keep > track and update its data, so it should have no problem feeding it > back to the next driver? Right, I was thinking about the packet offload case.
Hi Steffen, On Thu, Aug 22, 2024 at 09:10:47AM +0200, Steffen Klassert wrote: > > Yes, thanks for the clarification. The SA is not changed, we just delete it > > on old active slave > > > > slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs); > > > > And add to now one. > > > > ipsec->xs->xso.real_dev = slave->dev; > > slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL) > > Using the same key on two different devices is very dangerous. > Counter mode algorithms have the requirement that the IV > must not repeat. If you use the same key on two devices, > you can't guarantee that. If both devices use an internal > counter (initialized to one) to generate the IV, then the > IV repeats for the mumber of packets that were already > sent on the old device. The algorithm is cryptographically > broken in that case. > > Instead of moving the existing state, it is better to > request a rekey. Maybe by setting the old state to > 'expired' and then send a km_state_expired() message. Thanks for your comments. I'm not familiar with IPsec state. Do you mean something like diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index f74bacf071fc..8a51d0812564 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -477,6 +477,7 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) struct net_device *bond_dev = bond->dev; struct bond_ipsec *ipsec; struct slave *slave; + struct km_event c; rcu_read_lock(); slave = rcu_dereference(bond->curr_active_slave); @@ -498,6 +499,13 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) spin_lock_bh(&bond->ipsec_lock); list_for_each_entry(ipsec, &bond->ipsec_list, list) { ipsec->xs->xso.real_dev = slave->dev; + + ipsec->xs->km.state = XFRM_STATE_VALID; + c.data.hard = 1; + c.portid = 0; + c.event = XFRM_MSG_NEWSA; + km_state_notify(x, &c); + if (slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)) { slave_warn(bond_dev, slave->dev, "%s: failed to add SA\n", __func__); ipsec->xs->xso.real_dev = NULL; @@ -580,6 +588,8 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) "%s: no slave xdo_dev_state_delete\n", __func__); } else { + ipsec->xs->km.state = XFRM_STATE_EXPIRED; + km_state_expired(ipsec->xs, 1, 0); slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs); } }
2024-08-22, 09:10:47 +0200, Steffen Klassert wrote: > On Thu, Aug 22, 2024 at 08:33:17AM +0800, Hangbin Liu wrote: > > On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote: > > > > > > > + if (!real_dev->xfrmdev_ops || > > > > > > > + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { > > > > > > > + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); > > > > > > > > > > > > xdo_dev_state_advance_esn is called on the receive path for every > > > > > > packet when ESN is enabled (xfrm_input -> xfrm_replay_advance -> > > > > > > xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to > > > > > > be ratelimited. > > > > > > > > > > How does xfrm_state offload work on bonding? > > > > > Does every slave have its own negotiated SA? > > > > > > > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only > > > > current active slave keep the SA. When active slave changes, the sa on > > > > previous slave is deleted and re-added on new active slave. > > > > > > It's the same SA, there's no DELSA+NEWSA when we change the active > > > slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform > > > the driver/HW), and only a single NEWSA to install the offloaded SA on > > > the bond device (which calls the active slave's xdo_dev_state_add). > > > > Yes, thanks for the clarification. The SA is not changed, we just delete it > > on old active slave > > > > slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs); > > > > And add to now one. > > > > ipsec->xs->xso.real_dev = slave->dev; > > slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL) > > Using the same key on two different devices is very dangerous. It's only used by one device at a time, we only support offload with "active-backup" mode, where only the current active slave can send packets. > Counter mode algorithms have the requirement that the IV > must not repeat. If you use the same key on two devices, > you can't guarantee that. If both devices use an internal > counter (initialized to one) to generate the IV, then the > IV repeats for the mumber of packets that were already > sent on the old device. The algorithm is cryptographically > broken in that case. Aren't they basing the IV on the sequence number filled in the header? If not, then I guess this stuff has been broken since 2020 :( (18cb261afd7b ("bonding: support hardware encryption offload to slaves")) > Instead of moving the existing state, it is better to > request a rekey. Maybe by setting the old state to > 'expired' and then send a km_state_expired() message. But then you're going to drop packets during the whole rekey?
On Thu, Aug 22, 2024 at 10:39:11AM +0200, Sabrina Dubroca wrote: > 2024-08-22, 09:10:47 +0200, Steffen Klassert wrote: > > On Thu, Aug 22, 2024 at 08:33:17AM +0800, Hangbin Liu wrote: > > > On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote: > > > > > > > > + if (!real_dev->xfrmdev_ops || > > > > > > > > + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { > > > > > > > > + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); > > > > > > > > > > > > > > xdo_dev_state_advance_esn is called on the receive path for every > > > > > > > packet when ESN is enabled (xfrm_input -> xfrm_replay_advance -> > > > > > > > xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to > > > > > > > be ratelimited. > > > > > > > > > > > > How does xfrm_state offload work on bonding? > > > > > > Does every slave have its own negotiated SA? > > > > > > > > > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only > > > > > current active slave keep the SA. When active slave changes, the sa on > > > > > previous slave is deleted and re-added on new active slave. > > > > > > > > It's the same SA, there's no DELSA+NEWSA when we change the active > > > > slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform > > > > the driver/HW), and only a single NEWSA to install the offloaded SA on > > > > the bond device (which calls the active slave's xdo_dev_state_add). > > > > > > Yes, thanks for the clarification. The SA is not changed, we just delete it > > > on old active slave > > > > > > slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs); > > > > > > And add to now one. > > > > > > ipsec->xs->xso.real_dev = slave->dev; > > > slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL) > > > > Using the same key on two different devices is very dangerous. > > It's only used by one device at a time, we only support offload with > "active-backup" mode, where only the current active slave can send > packets. > > > Counter mode algorithms have the requirement that the IV > > must not repeat. If you use the same key on two devices, > > you can't guarantee that. If both devices use an internal > > counter (initialized to one) to generate the IV, then the > > IV repeats for the mumber of packets that were already > > sent on the old device. The algorithm is cryptographically > > broken in that case. > > Aren't they basing the IV on the sequence number filled in the header? > If not, then I guess this stuff has been broken since 2020 :( > (18cb261afd7b ("bonding: support hardware encryption offload to slaves")) Linux does that, but it is not guaranteed that other devices do that too. It is perfectly Ok to use some internal counter (or anything elase that does not repeat) to generate the IV. > > > Instead of moving the existing state, it is better to > > request a rekey. Maybe by setting the old state to > > 'expired' and then send a km_state_expired() message. > > But then you're going to drop packets during the whole rekey? Yes, I know. That would be the downside of that.
On Thu, Aug 22, 2024 at 04:33:36PM +0800, Hangbin Liu wrote: > Hi Steffen, > On Thu, Aug 22, 2024 at 09:10:47AM +0200, Steffen Klassert wrote: > > > Yes, thanks for the clarification. The SA is not changed, we just delete it > > > on old active slave > > > > > > slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs); > > > > > > And add to now one. > > > > > > ipsec->xs->xso.real_dev = slave->dev; > > > slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL) > > > > Using the same key on two different devices is very dangerous. > > Counter mode algorithms have the requirement that the IV > > must not repeat. If you use the same key on two devices, > > you can't guarantee that. If both devices use an internal > > counter (initialized to one) to generate the IV, then the > > IV repeats for the mumber of packets that were already > > sent on the old device. The algorithm is cryptographically > > broken in that case. > > > > Instead of moving the existing state, it is better to > > request a rekey. Maybe by setting the old state to > > 'expired' and then send a km_state_expired() message. > > Thanks for your comments. I'm not familiar with IPsec state. > Do you mean something like > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index f74bacf071fc..8a51d0812564 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -477,6 +477,7 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) > struct net_device *bond_dev = bond->dev; > struct bond_ipsec *ipsec; > struct slave *slave; > + struct km_event c; > > rcu_read_lock(); > slave = rcu_dereference(bond->curr_active_slave); > @@ -498,6 +499,13 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) > spin_lock_bh(&bond->ipsec_lock); > list_for_each_entry(ipsec, &bond->ipsec_list, list) { > ipsec->xs->xso.real_dev = slave->dev; > + > + ipsec->xs->km.state = XFRM_STATE_VALID; > + c.data.hard = 1; > + c.portid = 0; > + c.event = XFRM_MSG_NEWSA; > + km_state_notify(x, &c); The xfrm stack does that already when inserting the state. > + > if (slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)) { > slave_warn(bond_dev, slave->dev, "%s: failed to add SA\n", __func__); > ipsec->xs->xso.real_dev = NULL; > @@ -580,6 +588,8 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) > "%s: no slave xdo_dev_state_delete\n", > __func__); > } else { > + ipsec->xs->km.state = XFRM_STATE_EXPIRED; I think you also need to set 'x->km.dying = 1'. > + km_state_expired(ipsec->xs, 1, 0); Please test this at least with libreswan and strongswan. The state is actually not expired, so not sure if the IKE daemons behave as we want in that case. Downside of this approach is that you loose some packets until the new SA is negotiated, as Sabrina mentioned.
Hi Steffen, On Fri, Aug 23, 2024 at 10:24:46AM +0200, Steffen Klassert wrote: > > Thanks for your comments. I'm not familiar with IPsec state. > > Do you mean something like > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > index f74bacf071fc..8a51d0812564 100644 > > --- a/drivers/net/bonding/bond_main.c > > +++ b/drivers/net/bonding/bond_main.c > > @@ -477,6 +477,7 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) > > struct net_device *bond_dev = bond->dev; > > struct bond_ipsec *ipsec; > > struct slave *slave; > > + struct km_event c; > > > > rcu_read_lock(); > > slave = rcu_dereference(bond->curr_active_slave); > > @@ -498,6 +499,13 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) > > spin_lock_bh(&bond->ipsec_lock); > > list_for_each_entry(ipsec, &bond->ipsec_list, list) { > > ipsec->xs->xso.real_dev = slave->dev; > > + > > + ipsec->xs->km.state = XFRM_STATE_VALID; > > + c.data.hard = 1; > > + c.portid = 0; > > + c.event = XFRM_MSG_NEWSA; > > + km_state_notify(x, &c); > > The xfrm stack does that already when inserting the state. Thanks for this info. > > > + > > if (slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)) { > > slave_warn(bond_dev, slave->dev, "%s: failed to add SA\n", __func__); > > ipsec->xs->xso.real_dev = NULL; > > @@ -580,6 +588,8 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) > > "%s: no slave xdo_dev_state_delete\n", > > __func__); > > } else { > > + ipsec->xs->km.state = XFRM_STATE_EXPIRED; > > I think you also need to set 'x->km.dying = 1'. > > > + km_state_expired(ipsec->xs, 1, 0); > > Please test this at least with libreswan and strongswan. The state is > actually not expired, so not sure if the IKE daemons behave as we want > in that case. OK, this fix should be not related the current patch set. I will post this fix after more tests. Thanks Hangbin > > Downside of this approach is that you loose some packets until the new > SA is negotiated, as Sabrina mentioned.
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 560e3416f6f5..24747fceef66 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -651,10 +651,35 @@ static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs) return err; } +/** + * bond_advance_esn_state - ESN support for IPSec HW offload + * @xs: pointer to transformer state struct + **/ +static void bond_advance_esn_state(struct xfrm_state *xs) +{ + struct net_device *real_dev; + + rcu_read_lock(); + real_dev = bond_ipsec_dev(xs); + if (!real_dev) + goto out; + + if (!real_dev->xfrmdev_ops || + !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) { + pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name); + goto out; + } + + real_dev->xfrmdev_ops->xdo_dev_state_advance_esn(xs); +out: + rcu_read_unlock(); +} + 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_offload_ok = bond_ipsec_offload_ok, + .xdo_dev_state_advance_esn = bond_advance_esn_state, }; #endif /* CONFIG_XFRM_OFFLOAD */