Re: [PATCH net v4 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 3 potential
issue(s) to consider.

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

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 puts a dev_hold() on the vti/vti6 net_device into
  every outer skb (vti_input(), vti6_input_proto()).
- [High] __input_process_payload() can leak the outer skb and, after this
  patch, the vti/vti6 device reference it carries, so the reference is…
- [Low] The new comments describing the tunnel-device reference do not
  match the code.

--

xfrm: hold the tunnel device reference across the input path

vti_input() and vti6_input_proto() now take a reference on the tunnel
net_device when they cache the tunnel pointer in the skb control buffer.
xfrm_input() drops that reference on its delivery and drop paths. IPTFS
drops it through the new xfrm_input_drop() and xfrm_input_consume()
helpers when it frees an outer skb.

> diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
> index 3b80929994a00..cbe7906a4a1ba 100644
> --- a/net/ipv4/ip_vti.c
> +++ b/net/ipv4/ip_vti.c
> @@ -63,6 +63,9 @@ static int vti_input(struct sk_buff *skb, int nexthdr, __be32 spi,
>  
>  		XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4 = tunnel;
>  
> +		/* Kept until xfrm_input() is done with the skb. */
> +		dev_hold(tunnel->dev);
> +

[Severity: High]
Can IPTFS keep the vti device pinned for a very long time, or with no
time limit, now that every outer skb holds this reference?

IPTFS can keep the outer skb after xfrm_input() has returned. It keeps it
as xtfs->ra_newskb when __input_process_payload() reuses the outer skb as
first_skb and that skb is partial. It also keeps it in the reorder window
through __reorder_future_fits():

	xtfs->w_saved[index].skb = inskb;

Only iptfs_drop_timer(), later traffic or iptfs_destroy_state() release
these skbs. The drop time comes from XFRMA_IPTFS_DROP_TIME, which has no
upper bound (u32 microseconds, about 71 minutes).

The drop timer can also be lost. __iptfs_reassem_done() calls
hrtimer_try_to_cancel(&xtfs->drop_timer), but the reorder window uses the
same timer. iptfs_set_window_drop_times() only re-arms it here:

	if (index == -1 && !hrtimer_is_queued(&xtfs->drop_timer))
		hrtimer_start(&xtfs->drop_timer, xtfs->drop_time_ns,
			      IPTFS_HRTIMER_MODE);

With the default window of 3, this sequence seems to leave an outer skb
parked with no timer:

  wantseq = N
  N arrives, starts a reassembly, drop_timer is armed
  N+3 arrives, goes into w_saved[1]; the timer is already queued, so it
    is not re-armed
  N+1 arrives, __reorder_this() shifts N+3 into w_saved[0]
  iptfs_input_ordered(N+1) completes the reassembly
    iptfs_reassem_cont()->__iptfs_reassem_done()
      hrtimer_try_to_cancel(&xtfs->drop_timer)

N+3, and its vti device reference, then waits for more traffic or for the
SA to be destroyed.

Nothing in xfrm or IPTFS handles NETDEV_UNREGISTER for the vti device.
Would "ip link del vti0" then block in netdev_wait_allrefs_any()?

Netns teardown looks worse. vti_net_ops and vti6_net_ops are pernet device
ops, so their exit runs before the xfrm_net_ops subsys exit.
xfrm_net_pre_exit() only flushes policies. States are only flushed in
xfrm_net_exit()->xfrm_state_fini()->xfrm_state_flush().

So iptfs_destroy_state() cannot run until the device unregister finishes.
The unregister is waiting for a reference that only iptfs_destroy_state()
would drop. Could this stall cleanup_net with no end?

Before this patch the parked skbs held no device reference. The dev_hold()
added in vti6_input_proto() has the same issue.

[ ... ]

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 517369ba9efea..209fd7ffb5c82 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -120,6 +120,60 @@ static int xfrm_rcv_cb(struct sk_buff *skb, unsigned int family, u8 protocol,
>  	return ret;
>  }
>  
> +/*
> + * vti/vti6 cache the tunnel in the skb control buffer and keep a
> + * reference on its net_device.  The tunnel is the device's private
> + * area, so the reference keeps both alive.  xfrm_input() releases it
> + * once the packet has been delivered; the drop paths release it when
> + * no receive callback ran.  The caller passes the address family of the
> + * cached tunnel.
> + */

[Severity: Low]
This isn't a bug, but this comment doesn't seem to match the code.

The comment says "xfrm_input() releases it once the packet has been
delivered". In the decaps branch the reference is dropped right after
gro_cells_receive(), and gro_cells_receive() only queues the skb for the
gro_cells NAPI:

		tdev = xfrm_input_take_tunnel_dev(skb, family);
		gro_cells_receive(&gro_cells, skb);
		if (tdev)
			dev_put(tdev);

It also says "the drop paths release it when no receive callback ran".
The drop: label calls the receive callback first, and vti_rcv_cb() reads
tunnel->dev there:

	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
	tdev = xfrm_input_take_tunnel_dev(skb, family);

The comments in vti_input() ("Kept until xfrm_input() is done with the
skb.") and vti6_input_proto() have a similar problem with IPTFS.
xfrm_input() returns on -EINPROGRESS while the outer skb and its reference
are still in xtfs->w_saved[] or xtfs->ra_newskb.

Could these comments describe when the reference is actually released?

[ ... ]

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 6920940a35b49..657dce0c9fdf8 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c

[ ... ]

> @@ -1204,14 +1205,15 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
>  	skb_abort_seq_read(skbseq);
>  
>  	if (defer) {
> -		consume_skb(defer);
> +		xfrm_input_consume(defer, x->props.family);
>  	} else if (!first_skb) {
>  		/* skb is the original passed in skb, but we didn't get far
>  		 * enough to process it as the first_skb, if we had it would
>  		 * either be save in ra_newskb, trimmed and sent on as an skb or
>  		 * placed in defer to be freed.
>  		 */
> -		kfree_skb(skb);
> +		xfrm_input_drop(skb, x->props.family,
> +				SKB_DROP_REASON_NOT_SPECIFIED);
>  	}
>  	return true;
>  }

[Severity: High]
Does this path leak the outer skb, and with it the vti device reference?

On the first loop iteration, __input_process_payload() can reuse the
outer skb as first_skb while XFRM_TUNNEL_SKB_CB(skb)->tunnel is still
set. If the first inner packet is complete, the skb is queued:

		iptfs_complete_inner_skb(x, skb);
		list_add_tail(&skb->list, sublist);

If a later inner block in the same payload is malformed, the loop jumps
to done. For example:

		} else {
			XFRM_INC_STATS(net, LINUX_MIB_XFRMINBUFFERERROR);
			goto done;
		}

The IPv4 iplen < iphlen check and a skb_copy_seq_read() failure take the
same jump.

The done: label comes after the "Send the packets!" loop, so first_skb
never reaches xfrm_input(skb, 0, 0, -2). At done: defer is NULL and
first_skb is non-NULL, so neither xfrm_input_consume() nor
xfrm_input_drop() runs, and the function returns true.
iptfs_input_ordered() then sees consumed == true, and its on-stack
sublist goes out of scope.

The skb leak was already there. With this patch, each such packet also
leaks a reference on the vti/vti6 net_device, so a later unregister of
the tunnel would wait forever.

A completed reassembly skb that iptfs_reassem_cont() put on the same
sublist seems to leak the same way when it is an earlier reused outer
skb.

The commit message says the outer-skb free paths in
__input_process_payload() are covered. Should the goto done path also
free the skbs on sublist, through xfrm_input_drop()?

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