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

[PATCH net v3] 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
skbs stolen by async crypto and return ERR_PTR(-EINPROGRESS) when all
skbs in the list were taken by async crypto.  Update both
__dev_queue_xmit() and __dev_direct_xmit() to handle ERR_PTR correctly.

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 v3:
 - Fix validate_xmit_skb_list(): set stolen=true only for -EINPROGRESS,
   not for all IS_ERR() cases (Sabrina Dubroca)
 - Update __dev_direct_xmit() to handle ERR_PTR(-EINPROGRESS) from
   validate_xmit_skb_list() before kfree_skb_list() which would crash
   on an ERR_PTR (Sabrina Dubroca / sashiko)

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         | 19 ++++++++++++++++---
 net/xfrm/xfrm_device.c |  2 +-
 2 files changed, 17 insertions(+), 4 deletions(-)

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);
 	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(). */
+			if (PTR_ERR(skb) == -EINPROGRESS)
+				rc = NET_XMIT_SUCCESS;
 			goto out;
+		}
 
 		HARD_TX_LOCK(dev, txq, cpu);
 
@@ -4920,7 +4931,9 @@ int __dev_direct_xmit(struct sk_buff *skb, u16 queue_id)
 		goto drop;
 
 	skb = validate_xmit_skb_list(skb, dev, &again);
-	if (skb != orig_skb)
+	if (IS_ERR(skb))
+		return PTR_ERR(skb) == -EINPROGRESS ? NET_XMIT_SUCCESS : NET_XMIT_DROP;
+	if (!skb || skb != orig_skb)
 		goto drop;
 
 	skb_set_queue_mapping(skb, queue_id);
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 550457e4c..bb0c0fafa 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 v3] xfrm: propagate -EINPROGRESS from validate_xmit_xfrm()

[Sabrina Dubroca]

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