Re: [PATCH 1/1] bonding: crypto offload enabled, non-offload slave failover, rekey failed

[David Dai]

On Thu, 2026-09-10 at 01:02 +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 2
> 
> - [Medium] Incomplete error-code mapping in `bond_ipsec_add_sa()`:
> the
>   sibling failure path that fires when the bond has no current
> active…
> - [Low] Missing `Fixes:` tag / unclear target tree for a user-visible
>   behaviour fix.
> - [Low] No in-tree test covers the behaviour this patch introduces.
> 
> --
> 
> bonding: crypto offload enabled, non-offload slave failover, rekey
> failed
> 
> This changes bond_ipsec_add_sa() to return -EOPNOTSUPP instead of -
> EINVAL
> when the current active slave has no xdo_dev_state_add, so that xfrm
> falls
> back to a software SA instead of failing the SA add outright.  The
> reported
> symptom is a child SA rekey failing after failover from an offload
> capable
> slave to a non-offload capable one.
> 
> No Link: tags were present in the commit header.
> 
> > diff --git a/drivers/net/bonding/bond_main.c
> > b/drivers/net/bonding/bond_main.c
> > index a9bff7663eeca..de2489c3d9bf2 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -490,7 +490,7 @@ static int bond_ipsec_add_sa(struct net_device
> > *bond_dev,
> >  	    !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");
> > -		err = -EINVAL;
> > +		err = -EOPNOTSUPP;
> >  		goto out;
> >  	}
> 
> [Severity: Low]
> Should this carry a Fixes: tag, and which tree does it target?

Yes, I need to add this line when I resubmit it in v2 with:
Fixes: 18cb261afd7b ("bonding: support hardware encryption offload to
slaves")

> 
> The change fixes a user visible failure (child SA rekey is rejected
> while a
> non-offload slave is active), so it reads like a net fix rather than
> a
> net-next cleanup, but neither a Fixes: tag nor a [PATCH net] /
> [PATCH net-next] marker is present.
> 
Yes, it's a net fix. I need to use this in the next submit:
[PATCH net v2 1/1] bonding: crypto offload enabled, non-offload slave
failover, rekey failed

> git blame on the changed line at the baseline points at
> 2aeeef906d5a52
> ("bonding: change ipsec_lock from spin lock to mutex"), which only
> moved the
> line.  The enclosing capability check goes back to 18cb261afd7bf5
> ("bonding: support hardware crypto offload") by way of
> 907ed83a7583e8.  Would
> one of those be the right anchor for stable backports?
> 
> Also, the Tested-by: line names the same person as the Signed-off-
> by:.  Is
> that intentional?
> 
My original intention is to imply this patch has been tested before
submission. So it was intentional. If it's redundant, I can remove it.

> [Severity: Medium]
> Should the sibling "no active slave" path in the same function be
> converted
> too?  Just above this hunk, bond_ipsec_add_sa() still returns a fatal
> errno
> when there is no current active slave:
> 
> drivers/net/bonding/bond_main.c:bond_ipsec_add_sa() {
> 	slave = rcu_dereference(bond->curr_active_slave);
> 	real_dev = slave ? slave->dev : NULL;
> 	netdev_hold(real_dev, &tracker, GFP_ATOMIC);
> 	rcu_read_unlock();
> 	if (!real_dev) {
> 		err = -ENODEV;
> 		goto out;
> 	}
> 	...
> }
> 
> The consumer only whitelists -EOPNOTSUPP for the silent software
> fallback:
> 
> net/xfrm/xfrm_device.c:xfrm_dev_state_add() {
> 	...
> 	if ((err != -EOPNOTSUPP && !is_packet_offload) ||
> is_packet_offload) {
> 		NL_SET_ERR_MSG_WEAK(extack, "Device failed to
> offload this state");
> 		return err;
> 	}
> 	...
> }
> 
> and xfrm_state_construct() then kills the freshly allocated state:
> 
> net/xfrm/xfrm_user.c:xfrm_state_construct() {
> 	...
> 	err = xfrm_dev_state_add(net, x,
> nla_data(attrs[XFRMA_OFFLOAD_DEV]), extack);
> 	if (err)
> 		goto error;
> 	...
> error:
> 	x->km.state = XFRM_STATE_DEAD;
> 	xfrm_state_put(x);
> }
> 
> So with crypto offload requested and -ENODEV returned, the NEWSA
> still fails
> hard and no software SA is created.  That is reachable when every
> slave is
> down, in the window where bond_select_active_slave() has installed a
> NULL
> curr_active_slave, or trivially with a bond that has no slaves yet:
> 
> 	ip link add bond0 type bond
> 	ip xfrm state add ... offload dev bond0 dir out
> 
> Given the commit message says "network traffic is never interrupted,
> always
> keeps going", does the -ENODEV case leave the same failure mode
> reachable in
> a narrower window?
> 
Before my patch, bond0 had ipsec crypto offload enabled,
non-offload capable eth2 failover to be active slave, the ipsec traffic
will use SW path, no interruption, no problem.
However when strongswan service, or swanctl tool does child SA rekey,
it failed.

My patch deals with the child SA rekey failure issue when non-offload
capable eth2 is active slave.
By changing return code from -EINVAL to -EOPNOTSUPP, xfrm will then
create a SA without crypto offload when non-offload capable eth2 is the
active slave.
To keep the traffic going, at any time, either one of the slave
(offload capable interface eth1 or non-offload capable interface eth2)
is up at any give time.
So real_dev is a valid pointer. Traffic is not interrupted.

I need to delete the last line in my v1 patch comment to avoid
confusion:
"This way, network traffic is never interrupted, always keeps going.".

W.r.t. "ip xfrm state add ... offload dev bond0 dir out",
my understanding is child SA created by ip xfrm command will never
expire. There is no need/way to do child SA rekey either.

W.r.t. the sibling check that real_slave is NULL condition,
If real_dev is NULL, no slave is available to handle traffic.
In my opinion, it's a fatal error. Setting err to -ENODEV is correct.
If it happens, child SA rekey should fail hard.

> [Severity: Low]
> Is there a plan to extend the existing selftest to cover the new
> fallback?
> 
No, not in this patch submission.
The current bond_ipsec_offload.sh uses "ip xfrm state add ..." to set
child SA keys, which will never expire. ip command can't do child SA
reky.

This patch deals with child SA rekey when non-offload capable interface
eth2 is the active slave, which requires ipsec applications like
libreswan, strongswan, etc...
Different linux distros have different ipsec applications.
If I can find a universal way to be tested in the selftest later,
will address it in a separate patch.

> tools/testing/selftests/drivers/net/bonding/bond_ipsec_offload.sh
> enslaves
> two netdevsim ports in setup_bond():
> 
> 	echo "0 2" | ip netns exec "$ns" tee
> /sys/bus/netdevsim/new_device >/dev/null
> 
> Both provide xdo_dev_state_add, and the failover leg only moves the
> active
> slave between those two before re-running test_offload().  No leg
> enslaves a
> veth or dummy device, fails over to it, and then adds a new SA to
> check that
> it is accepted with software fallback.
> 
> As it stands the script passes identically before and after this
> change, and
> would keep passing if the fallback later regressed back to a hard
> error.