Re: [PATCH net v5 1/3] xfrm: run the receive callback before dropping the skb->dev reference

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] The tunnel-mode (decaps) success exit of xfrm_input() in
  net/xfrm/xfrm_input.c still runs `if (async) dev_put(dev);` before…

--

xfrm: run the receive callback before dropping the skb->dev reference

On the drop path of xfrm_input(), this commit moves xfrm_rcv_cb() so it
runs before the async skb->dev reference is released. It also changes
the early drop for a state that is no longer valid: instead of calling
dev_put() itself, that path now leaves the reference to the drop label.

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 5ed87d51392a..94de4f698c33 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c

[ ... ]

> @@ -779,9 +779,9 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  drop_unlock:
>  	spin_unlock(&x->lock);
>  drop:
> +	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
>  	if (async)
>  		dev_put(dev);
> -	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
>  	kfree_skb(skb);
>  	return 0;
>  }

[Severity: High]
The commit message says the new order matches what "the success path
does". Is that true of the tunnel mode (decaps) exit of xfrm_input()?
That exit still drops the async reference before it passes the skb to
gro_cells_receive():

net/xfrm/xfrm_input.c:xfrm_input() {
    ...
	if (decaps) {
		...
		if (async)
			dev_put(dev);
		gro_cells_receive(&gro_cells, skb);
		rcu_read_unlock();
		return 0;
    ...
}

gro_cells_receive() then dereferences skb->dev:

net/core/gro_cells.c:gro_cells_receive() {
	struct net_device *dev = skb->dev;
	...
	if (unlikely(!(dev->flags & IFF_UP)))
		goto drop;
	...
drop:
	dev_core_stats_rx_dropped_inc(dev);
	...
}

In plain policy-based ESP/AH tunnel mode, no xfrmi or vti callback
replaces skb->dev. So skb->dev is the same lower device whose reference
was just released.

Suppose an async crypto completion runs while the lower device is being
unregistered:

esp_input_done()
  xfrm_input_resume()
    xfrm_input(skb, nexthdr, 0, -1)

Can the device refcount reach 1 after this dev_put()?

netdev_wait_allrefs_any() in net/core/dev.c returns as soon as
netdev_refcnt_read(dev) == 1. netdev_run_todo() then frees the device
without waiting for a grace period, so the rcu_read_lock() held here
does not seem to keep the device alive.

If the device is going away, IFF_UP is already clear. In that case
gro_cells_receive() would take its drop branch and write the core stats
of a net_device that may already be freed.

This ordering came in with 1c428b038400, the commit named in the Fixes:
tag, and 8045c0df98d4 kept it. It also seems to survive the later commit
"xfrm: hold the tunnel device reference across the input path", which
only adds tdev handling around gro_cells_receive().

Since dev is already saved in a local variable, could the decaps branch
also move the if (async) dev_put(dev) below gro_cells_receive()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019153247.89957-1-q.h.hack.winter%40gmail.com