Re: [PATCH net v2 2/3] xfrm: iptfs: run the receive callbacks before freeing the skb

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 1 · 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 converts only some of the IPTFS paths that free the
  outer ESP skb, so the bug its commit message describes is still…
- [High] xfrm_input_drop() now runs the per-protocol receive callbacks on
  outer IPTFS skbs that may have been parked in the reorder window.
- [Medium] xfrm_input_drop() always calls xfrm_rcv_cb() with err = -1.
- [Low] The commit message says the receive callbacks must run because
  "whatever they cached in the skb control buffer is never cleaned up".

--

xfrm: iptfs: run the receive callbacks before freeing the skb

This patch adds xfrm_input_drop(). It runs the protocol receive callback
with err = -1 and then frees the skb. The patch uses it in three places
where IPTFS frees the outer skb: __input_process_payload(),
iptfs_input_ordered() and the reorder-window freelist in iptfs_input().

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index ddf8fd75fd207..f293d5e124d47 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -120,6 +120,25 @@ static int xfrm_rcv_cb(struct sk_buff *skb, unsigned int family, u8 protocol,
>  	return ret;
>  }
>  
> +/* Finish an skb that is freed outside of xfrm_input()'s own drop path, e.g.
> + * from IPTFS.  The receive callbacks must still run before the skb goes away.
> + */

[Severity: Low]
The commit message says the callbacks need to run because "whatever they
cached in the skb control buffer is never cleaned up". Is that accurate at
this commit?

None of the cb_handlers release anything when err is nonzero.
vti_rcv_cb(), vti6_rcv_cb() and xfrmi_rcv_cb() only bump rx_errors and
rx_dropped. esp4_rcv_cb() and esp6_rcv_cb() just return 0:

net/ipv4/ip_vti.c:vti_rcv_cb() {
    ...
	if (err) {
		DEV_STATS_INC(dev, rx_errors);
		DEV_STATS_INC(dev, rx_dropped);

		return 0;
	}
    ...
}

A later patch in the series, "xfrm: transfer the skb->dev reference to the
input path", adds dev_hold() in vti_input()/vti6_input_proto() and
dev_put() in these err branches. Once that patch is applied, the
description is accurate.

This patch carries Fixes: and Cc: stable. If it were backported without
that follow-up, its only effect would be on device drop accounting. Could
the commit message mention the dependency, or be reworded?

> +void xfrm_input_drop(struct sk_buff *skb)
> +{
> +	struct sec_path *sp = skb_sec_path(skb);
> +	struct xfrm_state *x;
> +
> +	if (!sp || !sp->len) {
> +		kfree_skb(skb);
> +		return;
> +	}
> +
> +	x = sp->xvec[sp->len - 1];
> +	xfrm_rcv_cb(skb, x->props.family, x->type ? x->type->proto : 0, -1);

[Severity: Medium]
err is always -1 here, whatever the reason the skb is being freed.

In __input_process_payload(), the else if (!first_skb) branch is reached
after real errors. It is also reached after valid outer packets that were
fully consumed without starting a new inner packet. One example is a
pad-only payload:

net/xfrm/xfrm_iptfs.c:__input_process_payload() {
    ...
		} else if (iph->version == 0x0) {
			/* pad */
			data = tail;
			break;
    ...
}

Two other valid cases also end with first_skb == NULL:

- A continuation-only payload, such as the middle fragment of a large
  inner packet. Here iptfs_reassem_cont() returns data + fraglen == tail.
- A payload whose only new data is a runt saved by
  iptfs_input_save_runt().

For these valid packets, will vti_rcv_cb(), vti6_rcv_cb() and
xfrmi_rcv_cb() take their err branch and bump rx_errors and rx_dropped on
the vti/vti6/xfrm interface? Before this patch, these paths ran no
callback.

The later patch in the series still passes -1 here, so the counts would
still be wrong at the end of the series.

> +	kfree_skb(skb);
> +}
> +EXPORT_SYMBOL(xfrm_input_drop);

[ ... ]

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 6920940a35b49..4b3d04e531a51 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -1211,7 +1211,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
>  		 * 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);

[Severity: High]
This converts the !first_skb branch. Does the branch next to it still free
the outer skb without running the callbacks?

	if (defer) {
		consume_skb(defer);
	} else if (!first_skb) {

defer is the outer root skb whenever the first inner packet can't reuse
it. That covers the skb_has_frag_list() case, the head_frag/fragwalk case
and the allocate-new fallback.

defer is also the reused outer skb when pskb_trim() fails:

		if (pskb_trim(first_skb, first_iplen)) {
			/* error trimming */
			list_del(&first_skb->list);
			defer = first_skb;
		}

In both cases the skb is freed by consume_skb(defer) and no callback
runs. The inner skbs come from iptfs_alloc_skb(), which zeroes cb, so no
callback ever sees the outer skb's cb.

Other outer-skb free paths also skip xfrm_rcv_cb():

- When the reused outer skb becomes xtfs->ra_newskb (first_skb_partial),
  it can be freed by iptfs_drop_timer() via kfree_skb_reason(skb,
  SKB_DROP_REASON_FRAG_REASM_TIMEOUT).
- It can also be freed by __iptfs_reassem_done() with free == true, via
  iptfs_reassem_abort().
- It can also be freed by iptfs_destroy_state().
- iptfs_destroy_state() frees the outer skbs still in xtfs->w_saved[] with
  plain kfree_skb().

The commit message says IPTFS "frees it directly on several paths, so the
receive callbacks ... never run". The new comment says "The receive
callbacks must still run before the skb goes away."

The last patch in the series adds dev_hold(tunnel->dev) in
vti_input()/vti6_input_proto(). It relies on vti_rcv_cb()/vti6_rcv_cb()
calling dev_put() on the error path, and it doesn't touch xfrm_iptfs.c.

At the end of the series, would each outer skb freed on these paths leak
a reference on the vti/vti6 net_device? Deleting the device or its netns
would then hang in netdev_wait_allrefs().

>  	}
>  	return true;
>  }
> @@ -1298,7 +1298,7 @@ static void iptfs_input_ordered(struct xfrm_state *x, struct sk_buff *skb)
>  	if (!consumed) {
>  		skb = skbseq.root_skb;
>  		skb_abort_seq_read(&skbseq);
> -		kfree_skb(skb);
> +		xfrm_input_drop(skb);

[Severity: High]
iptfs_input_ordered() also processes outer skbs that
__reorder_future_fits() and __reorder_future_shifts() parked in the
reorder window. They are drained later, either from another softirq or
from the iptfs_drop_timer() soft hrtimer:

iptfs_drop_timer()
    iptfs_input_ordered()
        __input_process_payload() / !consumed
            xfrm_input_drop()
                xfrm_rcv_cb()
                    xfrmi_rcv_cb()

Nothing holds a reference on skb->dev or on the cached
XFRM_TUNNEL_SKB_CB()->tunnel while the skb is parked. With synchronous
crypto, xfrm_input() calls dev_put(dev) right after x->type->input()
returns -EINPROGRESS.

xfrmi_rcv_cb() dereferences skb->dev before it checks err:

net/xfrm/xfrm_interface_core.c:xfrmi_rcv_cb() {
    ...
	link = skb->dev->ifindex;
	dev = xi->dev;
	skb->dev = dev;

	if (err) {
    ...
}

If the ingress netdev is unregistered while the skb sits in w_saved, can
this read a freed net_device? IPTFS has no netdev notifier, and
unregistering the device doesn't flush w_saved. Before this patch, these
paths only called kfree_skb(), which doesn't dereference skb->dev.

At this commit, vti_rcv_cb()/vti6_rcv_cb() also call DEV_STATS_INC()
through tunnel->dev without holding a reference. If the vti device was
deleted in the meantime, that would write to freed memory.

The later patch "xfrm: transfer the skb->dev reference to the input path"
fixes the vti part with dev_hold() in vti_input()/vti6_input_proto(). The
xfrmi_rcv_cb() case still looks open at the end of the series.

The success path, xfrm_input(skb, 0, 0, -2) -> dev_net(skb->dev), already
has the same dangling skb->dev problem. That part is pre-existing, but
this patch extends it to the drop paths.

The freelist in iptfs_input() only ever holds the current inskb, so that
hunk isn't affected.

>  	}
>  }

[ ... ]

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