Re: [PATCH ipsec v2 2/2] xfrm: use cached offload ifindex when migrating an SA

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch's only AI-disclosure trailer, 'Assisted-by: LLM', sits
  below the '---' separator, after 'Signed-off-by: Cen Zhang'.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the patch now
  reads the cached xso->ifindex as a stable snapshot while the rest of…

--

xfrm: use cached offload ifindex when migrating an SA

This patch changes xso_to_xuo() to use the ifindex cached in the offload
record instead of dereferencing xso->dev. This closes a NULL dereference
when xfrm_dev_state_flush() races with XFRM_MSG_MIGRATE_STATE. It also
annotates the xso.dev check in xfrm_do_migrate_state() with READ_ONCE().

> Fixes: a9d155ea9b44 ("xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration")
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
>
> ---
> Assisted-by: LLM

[Severity: Low]
This isn't a bug, but the Assisted-by tag is below the "---" separator,
so git am and b4 drop it when the patch is applied. The applied commit
ends at the Signed-off-by line and has no Assisted-by trailer.

Documentation/process/submitting-patches.rst ("Using Assisted-by:") asks
contributors to acknowledge tool use with an Assisted-by tag. Earlier
xfrm commits such as c4a5f0071cc6 ("xfrm: cache the offload ifindex for
netlink dumps") put it in the regular tag block.

Could the tag be moved above the "---" line, next to Fixes: and
Signed-off-by:?

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 781d4ec565aae..1cea24380797d 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1240,7 +1240,7 @@ static void xso_to_xuo_ifindex(const struct xfrm_dev_offload *xso, int ifindex,
>  static void xso_to_xuo(const struct xfrm_dev_offload *xso,
>  		       struct xfrm_user_offload *xuo)
>  {
> -	xso_to_xuo_ifindex(xso, xso->dev->ifindex, xuo);
> +	xso_to_xuo_ifindex(xso, READ_ONCE(xso->ifindex), xuo);
>  }
>  #endif
>  
> @@ -3462,7 +3462,7 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	} else {
>  		bool inherit_offload = !(um->flags & XFRM_MIGRATE_STATE_CLEAR_OFFLOAD);
>  
> -		if (inherit_offload && x->xso.dev) {
> +		if (inherit_offload && READ_ONCE(x->xso.dev)) {
>  			xso_to_xuo(&x->xso, &xuo);
>  			m.xuo = &xuo;
>  		}

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. The
patch now treats the cached ifindex as a stable snapshot, but the rest of
the inherited xuo still comes from fields that teardown changes.

The dev check and xso_to_xuo() run without x->lock.
xso_to_xuo_ifindex() does plain reads of xso->dir and xso->type:

net/xfrm/xfrm_user.c:xso_to_xuo_ifindex() {
	xuo->ifindex = ifindex;
	if (xso->dir == XFRM_DEV_OFFLOAD_IN)
		xuo->flags = XFRM_OFFLOAD_INBOUND;
	if (xso->type == XFRM_DEV_OFFLOAD_PACKET)
		xuo->flags |= XFRM_OFFLOAD_PACKET;
}

At the same time, under RTNL, xfrm_dev_state_flush()->xfrm_dev_state_free()
does:

net/xfrm/xfrm_state.c:xfrm_dev_state_free() {
	...
		WRITE_ONCE(xso->dev, NULL);
		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
		netdev_put(dev, &xso->dev_tracker);
	...
}

Migration holds xfrm_cfg_mutex and the notifier holds RTNL. Nothing
orders the plain store and the plain load of xso->type.

Would KCSAN report this as a data race?

Suppose the flush lands between the READ_ONCE(x->xso.dev) check and the
type read. The xuo then gets the valid cached ifindex but loses
XFRM_OFFLOAD_PACKET. xfrm_state_migrate_create() then tries
xfrm_dev_state_add() on the clone in crypto offload mode.

As far as I can tell, this is contained. xfrm_dev_state_flush() calls
xfrm_state_delete() before xfrm_dev_state_free(), so this later locked
check in xfrm_do_migrate_state() throws away the clone:

	spin_lock_bh(&x->lock);
	if (x->km.state != XFRM_STATE_VALID) {
		spin_unlock_bh(&x->lock);
		NL_SET_ERR_MSG(extack, "State already deleted");
		err = -ESRCH;
		goto out_xc;
	}

The dump path has the same plain reads of dir and type, through
copy_user_state_offload(). The xso->dev NULL dereference described in the
commit message does look fixed.

Should the dir and type reads be annotated too, or the snapshot taken
under x->lock, so the inherited xuo stays consistent?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/pm-xfrm-core-objects-candidate-0050-v4-3c6b8c5369a802e9be0e-0%40gmail.com