Re: [PATCH net v4 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] This patch makes the async drop path in xfrm_input()
  (net/xfrm/xfrm_input.c) run xfrm_rcv_cb() before the async…

--

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

This patch reorders the drop label in xfrm_input() so that xfrm_rcv_cb()
runs before the async dev_put(dev). It also makes the early drop for an
invalid state let the drop label release the reference. The goal is to
keep callbacks such as xfrmi_rcv_cb() from touching skb->dev after its
reference has been dropped.

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 5ed87d51392a5..94de4f698c331 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 drop path now drops the reference "like the
success path does". Is that true for the tunnel-mode (decaps) success
path in xfrm_input()? That branch still drops the async reference before
handing the skb to GRO:

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() reads skb->dev right away:

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

For a plain tunnel-mode SA where no xfrmi or vti callback rewrote
skb->dev, skb->dev is still the local dev. The async reference is the
only thing pinning that device while the crypto request is pending.

During unregister, netdev_wait_allrefs_any() returns as soon as
netdev_refcnt_read(dev) == 1. netdev_run_todo() then calls
netdev_do_free_pcpu_stats(), priv_destructor and free_netdev() without
waiting for another RCU grace period. The rcu_read_lock() held in
xfrm_input() therefore does not protect the device after the dev_put().

Can this sequence free the netdev while gro_cells_receive() is still
using it?

esp_input() returns -EINPROGRESS with dev_hold(dev) taken
  underlay device is deleted, unregister waits for refs
crypto completes
  xfrm_input_resume()->xfrm_input(skb, nexthdr, 0, -1), async = 1
    resume_decapped sets decaps = 1
    xfrm_rcv_cb() succeeds, skb->dev == dev
    dev_put(dev)                 <- last outside reference dropped
      waiter sees refcnt == 1, frees dev
    gro_cells_receive()          <- dereferences skb->dev

On the IFF_UP-cleared branch, dev_core_stats_rx_dropped_inc() also
writes through the freed dev->core_stats pointer.

The decaps ordering comes from the same commit named in the Fixes tag,
1c428b038400. The rest of this series does not change it either: in the
final tree the async dev_put(dev) still comes before
xfrm_input_take_tunnel_dev() and gro_cells_receive(). The transport and
xfrm_gro branch already drops dev after transport_finish() and
gro_cells_receive().

Should this patch also move the decaps dev_put(dev) after
gro_cells_receive(), and should the commit message be updated so it
doesn't say the drop path matches all of the success paths?

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