Re: [PATCH] net: ixgbe: drop the SA reference if secpath_set() fails

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (2) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch fixes the SA reference leak in only one of the two
  copies of this Rx code.

Pre-existing issues:
- [High] ixgbe_ipsec_del_sa() unlinks the rx_sa from the RCU hash table
  with hash_del_rcu(&rsa->hlist) (line 783) and then zeroes the same…
- [High] ixgbe_ipsec_find_rx_state() finds rsa->xs only under
  rcu_read_lock() and takes its reference with xfrm_state_hold(), which…

--

net: ixgbe: drop the SA reference if secpath_set() fails

ixgbe_ipsec_find_rx_state() takes a reference on an xfrm_state. With this
patch, ixgbe_ipsec_rx() releases that reference when secpath_set() fails
to allocate the skb extension. Before this patch the reference was leaked
on that error path.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> index bd397b3d7deab..e44d5eed9ab59 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> @@ -1197,8 +1197,10 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>  		return;
>  
>  	sp = secpath_set(skb);
> -	if (unlikely(!sp))
> +	if (unlikely(!sp)) {
> +		xfrm_state_put(xs);
>  		return;
> +	}

[Severity: Medium]
Does ixgbevf need the same fix? The VF driver has a copy of this Rx code
in drivers/net/ethernet/intel/ixgbevf/ipsec.c. After this patch,
ixgbevf_ipsec_rx() still returns without dropping the reference:

drivers/net/ethernet/intel/ixgbevf/ipsec.c:ixgbevf_ipsec_rx() {
    ...
	sp = secpath_set(skb);
	if (unlikely(!sp))
		return;
    ...
}

ixgbevf_ipsec_find_rx_state() takes the reference the same way the PF
helper does:

			ret = rsa->xs;
			xfrm_state_hold(ret);
			break;

secpath_set()->skb_ext_add() is a GFP_ATOMIC allocation. Suppose a VF gets
inline-offloaded ESP/AH traffic while memory is tight. Would one
xfrm_state reference leak each time the allocation fails? If so, the state
would never be freed after the SA is deleted.

Could ixgbevf get a matching fix with its own Fixes: tag?

>  
>  	sp->xvec[sp->len++] = xs;
>  	sp->olen++;

[Severity: High]
This isn't a bug introduced by this patch, but is the rx_sa entry walked
by ixgbe_ipsec_find_rx_state() safe against ixgbe_ipsec_del_sa()?

ixgbe_ipsec_del_sa() unlinks the entry and zeroes it straight away. There
is no grace period in between:

drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c:ixgbe_ipsec_del_sa() {
    ...
		hash_del_rcu(&rsa->hlist);
    ...
		memset(rsa, 0, sizeof(struct rx_sa));
		ipsec->num_rx_sa--;
    ...
}

The NAPI reader runs under rcu_read_lock() and loads rsa->mode and
rsa->xs with plain loads:

ixgbe_ipsec_find_rx_state() {
    ...
		if (rsa->mode & IXGBE_RXTXMOD_VF)
			continue;
		if (spi == rsa->xs->id.spi &&
		    ((ip4 && *daddr == rsa->xs->id.daddr.a4) ||
    ...
}

Suppose a reader picked up rsa from the bucket before hash_del_rcu(). Can
it then see rsa->mode == 0, pass the VF check, and dereference
rsa->xs == NULL in softirq? If ixgbe_ipsec_add_sa() reuses the slot, the
reader could also see a mix of old and new SA fields.

The trigger would be an SA deletion (XFRM_MSG_DELSA, IKE rekey or
lifetime expiry) racing with Rx traffic whose SPI hashes to the same
bucket.

ixgbevf_ipsec_del_sa() has the same pattern:

		hash_del_rcu(&ipsec->rx_tbl[sa_idx].hlist);
		memset(&ipsec->rx_tbl[sa_idx], 0, sizeof(struct rx_sa));

[Severity: High]
This is a pre-existing issue, but is xfrm_state_hold() safe on an
xfrm_state that was found only under rcu_read_lock()?

ixgbe_ipsec_find_rx_state() does this:

			ret = rsa->xs;
			xfrm_state_hold(ret);

xfrm_state_hold() is an unconditional refcount_inc(). The rx_sa entry
holds no reference of its own on xs. __xfrm_state_delete() calls
xfrm_dev_state_delete() and then drops the reference right away:

net/xfrm/xfrm_state.c:__xfrm_state_delete() {
    ...
		xfrm_dev_state_delete(x);

		xfrm_state_delete_tunnel(x);
		...
		xfrm_state_put(x);
    ...
}

Can the refcount reach zero while a NAPI reader that has already loaded
rsa->xs is still in its RCU section? If so, refcount_inc() would run on 0.
That warns and saturates, but the pointer still ends up in sp->xvec[]
above.

xfrm_state_gc_task() frees the state after synchronize_rcu(). That waits
for RCU readers, but not for skbs that hold the secpath.

Suppose the skb outlives the grace period, for example when RPS queues it
to another CPU's backlog. Would xfrm_input() and
secpath_put()->xfrm_state_put() then touch freed memory?

The xfrm core handles this case with xfrm_state_hold_rcu(), which uses
refcount_inc_not_zero(). Should the driver take its reference with an
inc-not-zero as well? ixgbevf_ipsec_find_rx_state() has the same pattern.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103100534.1310039-1-vulab%40iscas.ac.cn