Re: [PATCH net v3] xfrm: propagate -EINPROGRESS from validate_xmit_xfrm()

[Sabrina Dubroca <sd@queasysnail.net>]

2026-06-01, 17:58:22 +0200, Petr Wozniak wrote:
> Changes in v2:
>  - Only reset rc to NET_XMIT_SUCCESS when PTR_ERR(skb) == -EINPROGRESS,
>    not for any IS_ERR() result (Sabrina Dubroca)
>  - Add comment explaining why rc is reset and the async delivery path
>  - Fix validate_xmit_skb_list(): track stolen skbs and return
>    ERR_PTR(-EINPROGRESS) when all skbs in the list were stolen by
>    async crypto, not NULL (Sabrina Dubroca)

I'm confused by this changelog. Was the "bool stolen" stuff in
validate_xmit_skb_list added to address my comment on v1 about the
skb_list_walk_safe() loop?

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0c6c270d9..b77364f83 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4078,6 +4078,7 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
>  struct sk_buff *validate_xmit_skb_list(struct sk_buff *skb, struct net_device *dev, bool *again)
>  {
>  	struct sk_buff *next, *head = NULL, *tail;
> +	bool stolen = false;
> 
>  	for (; skb != NULL; skb = next) {
>  		next = skb->next;
> @@ -4087,8 +4088,11 @@ struct sk_buff *validate_xmit_skb_list(struct sk_buff *skb, struct net_device *d
>  		skb->prev = skb;
>  
>  		skb = validate_xmit_skb(skb, dev, again);
> -		if (!skb)
> +		if (IS_ERR_OR_NULL(skb)) {
> +			if (IS_ERR(skb) && PTR_ERR(skb) == -EINPROGRESS)
> +				stolen = true;
>  			continue;
> +		}
>  
>  		if (!head)
>  			head = skb;
> @@ -4099,6 +4103,8 @@ struct sk_buff *validate_xmit_skb_list(struct sk_buff *skb, struct net_device *d
>  		 */
>  		tail = skb->prev;
>  	}
> +	if (!head && stolen)
> +		return ERR_PTR(-EINPROGRESS);

If you're keeping this code, sch_direct_xmit also needs to be updated
to handle ERR_PTR, not just __dev_direct_xmit.

(considering that sch_direct_xmit doesn't care, and I don't think
__dev_direct_xmit callers can reach the -EINPROGRESS path of
validate_xmit_xfrm, I'd skip all this "bool stolen" logic)

>  	return head;
>  }
>  EXPORT_SYMBOL_GPL(validate_xmit_skb_list);
> @@ -4858,8 +4864,13 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev)
>  			goto recursion_alert;
>  
>  		skb = validate_xmit_skb(skb, dev, &again);
> -		if (!skb)
> +		if (IS_ERR_OR_NULL(skb)) {
> +			/* -EINPROGRESS: packet stolen by async xfrm crypto,
> +			 * delivered via xfrm_dev_resume(). */

FWIW, I don't think this comment is that beneficial. A comment above
validate_xmit_skb describing the possible returns (NULL = dropped,
EINPROGRESS = stolen) would make sense, but something here and very
specific to xfrm (there may never be other sources of EINPROGRESS for
this codepath, but refering to implementation details of xfrm in the
core feels wrong).

-- 
Sabrina