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

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

[Petr Wozniak]

validate_xmit_xfrm() returns NULL both when a packet is dropped and
when it is stolen by async crypto (-EINPROGRESS from ->xmit()).
Callers cannot distinguish the two cases.

f53c723902d1 ("net: Add asynchronous callbacks for xfrm on layer 2.")
changed the semantics of a NULL return from "dropped" to "stolen or
dropped", but __dev_queue_xmit() was not updated.  On virtual/bridge
interfaces (noqueue qdisc) __dev_queue_xmit() initialises rc=-ENOMEM
and jumps to out: when skb is NULL, returning -ENOMEM to the caller
even though the packet will be delivered correctly via xfrm_dev_resume().

Return ERR_PTR(-EINPROGRESS) for the async case so callers can tell
it apart from a real drop.  Update validate_xmit_skb_list() to track
stolen skbs and return ERR_PTR(-EINPROGRESS) when all skbs in the
list were taken by async crypto.  Update __dev_queue_xmit() to return
NET_XMIT_SUCCESS in that case.

Fixes: f53c723902d1 ("net: Add asynchronous callbacks for xfrm on layer 2.")
Suggested-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Petr Wozniak <petr.wozniak@gmail.com>
---
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)

 net/core/dev.c         | 15 +++++++++++++--
 net/xfrm/xfrm_device.c |  2 +-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 804e8ad25..618e6299f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4079,6 +4079,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;
@@ -4088,8 +4089,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))
+				stolen = true;
 			continue;
+		}
 
 		if (!head)
 			head = skb;
@@ -4100,6 +4104,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);
 	return head;
 }
 EXPORT_SYMBOL_GPL(validate_xmit_skb_list);
@@ -4859,8 +4865,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(). */
+			if (PTR_ERR(skb) == -EINPROGRESS)
+				rc = NET_XMIT_SUCCESS;
 			goto out;
+		}
 
 		HARD_TX_LOCK(dev, txq, cpu);
 
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 5454be0b2..7702cca2b 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -182,7 +182,7 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur
 		err = x->type_offload->xmit(x, skb, esp_features);
 		if (err) {
 			if (err == -EINPROGRESS)
-				return NULL;
+				return ERR_PTR(-EINPROGRESS);
 
 			XFRM_INC_STATS(xs_net(x), LINUX_MIB_XFRMOUTSTATEPROTOERROR);
 			kfree_skb(skb);
-- 
2.51.0

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

[Sabrina Dubroca]

2026-05-30, 08:16:19 +0200, Petr Wozniak wrote:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 804e8ad25..618e6299f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4079,6 +4079,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;
> @@ -4088,8 +4089,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))
> +				stolen = true;
>  			continue;
> +		}
>  
>  		if (!head)
>  			head = skb;
> @@ -4100,6 +4104,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);

Why? You said previously that this path isn't affected (sure, since
validate_xmit_skb_list ignores intermediate returns).

Also, sashiko complains that if you're going to do this, the callers
should be updated to handle EINPROGRESS. And from my side, if you want
to keep this "bool stolen" handling here, stolen should only be true
only for EINPROGRESS, not all errors that validate_xmit_skb could
return.

>  	return head;
>  }
>  EXPORT_SYMBOL_GPL(validate_xmit_skb_list);
> @@ -4859,8 +4865,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(). */
> +			if (PTR_ERR(skb) == -EINPROGRESS)
> +				rc = NET_XMIT_SUCCESS;
>  			goto out;
> +		}
>  
>  		HARD_TX_LOCK(dev, txq, cpu);
>  
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index 5454be0b2..7702cca2b 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -182,7 +182,7 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur
>  		err = x->type_offload->xmit(x, skb, esp_features);
>  		if (err) {
>  			if (err == -EINPROGRESS)
> -				return NULL;
> +				return ERR_PTR(-EINPROGRESS);
>  
>  			XFRM_INC_STATS(xs_net(x), LINUX_MIB_XFRMOUTSTATEPROTOERROR);
>  			kfree_skb(skb);

c/p'ing my comment from v1 since you didn't address it (either with a
change or a reply to explain why it's not a valid concern):

What about the skb_list_walk_safe loop? If ->xmit() returns
-EINPROGRESS for all the skbs in the chain, we'll end up returning
NULL even though all the packets were stolen by the async path.

-- 
Sabrina