Re: [PATCH net v8 3/3] xfrm: hold the tunnel device reference across the input path

[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 adds a reference that leaks, and the leak fires a
  WARN on the error path that the async-teardown fix is meant to cover.

--

xfrm: hold the tunnel device reference across the input path

This patch makes vti_input(), vti6_input_proto() and xfrm6_rcv_spi() take
a reference on the tunnel net_device when they cache the tunnel pointer in
the skb control buffer. xfrm_input() releases the reference on delivery
and on drop, and IPTFS releases it through new helpers,
xfrm_input_drop() and xfrm_input_consume().

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index cfd5cea033127..e3b6c486f0e9c 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c

[ ... ]

> @@ -781,9 +838,12 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  	spin_unlock(&x->lock);
>  drop:
>  	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
> +	tdev = xfrm_input_take_tunnel_dev(skb, family);
> +	kfree_skb(skb);
> +	if (tdev)
> +		dev_put(tdev);

[Severity: High]
Can this leak the tunnel device reference taken by dev_hold() in
vti_input() or vti6_input_proto()?

At the top of xfrm_input(), in the resume/GRO/offload branch, the
invalid-state check jumps to drop before family is assigned:

    if (unlikely(x->km.state != XFRM_STATE_VALID)) {
        ...
        if (encap_type == -1)
            async = 1;
        goto drop;
    }

    family = x->props.family;

At that point family still has its initial value, AF_UNSPEC. The slot is
non-NULL, so xfrm_input_take_tunnel_dev() falls into its default case:

    default:
        WARN_ON_ONCE(1);
        return NULL;

The slot is not cleared and tdev stays NULL. The skb is freed and the
dev_put() is skipped.

One way to reach this is async crypto that completes after the SA has
been deleted or has expired, for example during an IKE rekey:

vti_input()
  dev_hold(tunnel->dev)
  xfrm_input()
    esp_input() returns -EINPROGRESS
<SA deleted, km.state no longer XFRM_STATE_VALID>
xfrm_input_resume()
  xfrm_input(skb, nexthdr, 0, -1)
    goto drop with family == AF_UNSPEC

Two other paths look the same. One is an offload skb that enters through
vti_input() with encap_type 0. The other is the IPTFS outer skb that is
passed to xfrm_input(skb, 0, 0, -2) after the SA has become invalid.

If the reference leaks, a later unregister of the vti/vti6 device or a
netns teardown would wait forever in netdev_wait_allrefs_any(). The
WARN_ON_ONCE() would also panic the machine with panic_on_warn set.

The commit message says:

  "The early drop for a state that is no longer valid releases
  the reference too, and needs the address family for that, so set the
  family before the validity check."

The diff doesn't move the family = x->props.family assignment ahead of
the km.state check, though. Was that hunk left out of the patch?

>  	if (async)
>  		dev_put(dev);
> -	kfree_skb(skb);
>  	return 0;
>  }
>  EXPORT_SYMBOL(xfrm_input);

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