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

[netdev-bot+sashiko@kernel.org]

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?

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.

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?

[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?

[Severity: Low]
Is there a plan to extend the existing selftest to cover the new fallback?

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.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903220123.475685-1-zdai%40linux.ibm.com