Re: [PATCHv3 net-next 2/3] bonding: Add ESN support to IPSec HW offload

[Sabrina Dubroca <sd@queasysnail.net>]

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.

-- 
Sabrina