Re: [PATCH 07/12] xfrm: add missing rcu_read_lock(), skb_dst_force() and dev_hold() for xfrm_trans_reinject()

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xfrm: add missing rcu_read_lock(), skb_dst_force() and dev_hold() for xfrm_trans_reinject()

This adds rcu_read_lock() around the finish() callback loop in
xfrm_trans_reinject(), forces the dst to be refcounted in
xfrm_trans_queue_net(), and takes a reference on skb->dev across the
workqueue deferral.

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index eecab337bd0a7..8f6109eada7ea 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -800,12 +800,17 @@ static void xfrm_trans_reinject(struct work_struct *work)
>  	spin_unlock_bh(&trans->queue_lock);
>  
>  	local_bh_disable();
> +	rcu_read_lock();
>  	while ((skb = __skb_dequeue(&queue))) {
>  		struct net *net = XFRM_TRANS_SKB_CB(skb)->net;
> +		struct net_device *dev = skb->dev;
>  
>  		XFRM_TRANS_SKB_CB(skb)->finish(net, NULL, skb);
> +		if (dev)
> +			dev_put(dev);
>  		put_net(net);
>  	}
> +	rcu_read_unlock();
>  	local_bh_enable();
>  }
>  
> @@ -821,12 +826,18 @@ int xfrm_trans_queue_net(struct net *net, struct sk_buff *skb,
>  	if (skb_queue_len(&trans->queue) >= READ_ONCE(net_hotdata.max_backlog))
>  		return -ENOBUFS;
>  
> +	if (skb_dst(skb) && !skb_dst_force(skb))
> +		return -EHOSTUNREACH;
> +
>  	BUILD_BUG_ON(sizeof(struct xfrm_trans_cb) > sizeof(skb->cb));
>  
>  	hold_net = maybe_get_net(net);
>  	if (!hold_net)
>  		return -ENODEV;
>  
> +	if (skb->dev)
> +		dev_hold(skb->dev);

[Severity: High]
Is skb->dev guaranteed to be alive here for every caller of
xfrm_trans_queue_net()?

The three transport-input callers look fine: xfrm_input() holds
rcu_read_lock() around afinfo->transport_finish(), and espintcp_rcv()
sets skb->dev via dev_get_by_index_rcu() inside the same RCU section.

The ESPinTCP output caller looks different. esp_output_tail_tcp() in
net/ipv4/esp4.c (and its net/ipv6/esp6.c twin) only does:

	local_bh_disable();
	err = xfrm_trans_queue_net(xs_net(x), skb, esp_output_tcp_encap_cb);
	local_bh_enable();

There is no rcu_read_lock() here, and per this patch's own commit message
local_bh_disable() is not an RCU read-side critical section under
CONFIG_PREEMPT_RCU.

That path is also reachable asynchronously. esp_output_tail() registers
the completion with aead_request_set_callback(req, 0, esp_output_done,
skb), and when crypto_aead_encrypt() returns -EINPROGRESS,
esp_output_done() later calls esp_output_tail_tcp() from a cryptd worker
or driver completion context, long after the receive-side RCU section
ended.

For a forwarded packet, what is skb->dev at that point? ip_forward() ->
NF_HOOK(..., ip_forward_finish) -> dst_output() enters the xfrm bundle
output with skb->dev still pointing at the ingress netdevice:

net/ipv4/ip_forward.c:ip_forward_finish() {
	...
	skb_clear_tstamp(skb);
	return dst_output(net, sk, skb);
}

Nothing on that path resets skb->dev (net/xfrm/xfrm_output.c only assigns
skb->dev on the HW-offload xmit path and in the IPv6 pmtu error path), and
nothing holds a reference on the ingress device - the skb only records
skb_iif, and the bundle dst references the egress device.

So if the ingress device is unregistered and freed while the request sits
in the crypto queue, does dev_hold(skb->dev) here increment a refcount in
freed memory, with the matching dev_put() in xfrm_trans_reinject() writing
to it a second time? With CONFIG_PCPU_DEV_REFCNT=n this would also hit
refcount_warn_saturate() on increment-from-zero.

There is a second aspect: esp_output_tcp_encap_cb() only uses
skb_dst(skb)->xfrm and never touches skb->dev, so the hold seems to be
unnecessary for that caller. Would it make sense to take the reference
only in the callers where skb->dev is provably live and the finish()
callback needs it - for example recording the held device in struct
xfrm_trans_cb - rather than holding unconditionally in the shared helper?