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

[PATCH net v4] 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) from validate_xmit_xfrm() for the async
case so callers can tell it apart from a real drop.  Update
__dev_queue_xmit() to handle ERR_PTR(-EINPROGRESS) from
validate_xmit_skb() correctly.  Update validate_xmit_skb_list() to
use IS_ERR_OR_NULL() so that ERR_PTR(-EINPROGRESS) is not mistakenly
added to the transmitted list.

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 v4:
 - Drop bool stolen tracking in validate_xmit_skb_list() and ERR_PTR
   return from that function; use IS_ERR_OR_NULL() in the loop so
   stolen skbs are silently skipped (same as the pre-patch NULL behaviour)
   (Sabrina Dubroca)
 - Drop ERR_PTR(-EINPROGRESS) handling in __dev_direct_xmit(); callers
   cannot reach the -EINPROGRESS path of validate_xmit_xfrm()
   (Sabrina Dubroca)
 - Move validate_xmit_skb() return-value comment above the function
   instead of inline in __dev_queue_xmit() (Sabrina Dubroca)

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

diff --git a/net/core/dev.c b/net/core/dev.c
index 0c6c270d9..18e92bef7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4016,6 +4016,9 @@ static struct sk_buff *validate_xmit_unreadable_skb(struct sk_buff *skb,
 	return NULL;
 }
 
+/* Returns the skb on success, NULL if dropped, or ERR_PTR(-EINPROGRESS)
+ * if stolen by async xfrm crypto (delivered via xfrm_dev_resume()).
+ */
 static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev, bool *again)
 {
 	netdev_features_t features;
@@ -4078,7 +4081,6 @@ 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;
-
 	for (; skb != NULL; skb = next) {
 		next = skb->next;
 		skb_mark_not_on_list(skb);
@@ -4087,7 +4089,7 @@ 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))
 			continue;
 
 		if (!head)
@@ -4858,8 +4860,11 @@ 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)) {
+			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 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 v4] xfrm: propagate -EINPROGRESS from validate_xmit_xfrm()

[Sabrina Dubroca]

2026-06-03, 08:46:59 +0200, Petr Wozniak wrote:
> +/* Returns the skb on success, NULL if dropped, or ERR_PTR(-EINPROGRESS)
> + * if stolen by async xfrm crypto (delivered via xfrm_dev_resume()).
> + */
>  static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev, bool *again)
>  {
>  	netdev_features_t features;
> @@ -4078,7 +4081,6 @@ 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;
> -

nit: no, that blank line needs to stay

>  	for (; skb != NULL; skb = next) {
>  		next = skb->next;
>  		skb_mark_not_on_list(skb);
> @@ -4087,7 +4089,7 @@ 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))
>  			continue;
>  
>  		if (!head)

[...]
> 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);

Could you please explain why you're ignoring my comment about the
2nd x->type_offload->xmit() call in this function?

Thanks,

-- 
Sabrina

[PATCH net v5 0/2] xfrm: fix async crypto (-EINPROGRESS) handling in validate_xmit_xfrm()

[Petr Wozniak]

This series fixes how the async crypto path (-EINPROGRESS from ->xmit())
is handled in validate_xmit_xfrm() and its callers.

Patch 1 (previously sent on its own, v1-v4) makes validate_xmit_xfrm()
return ERR_PTR(-EINPROGRESS) instead of NULL when a packet is stolen by
async crypto, so __dev_queue_xmit() can tell it apart from a real drop
and stop reporting -ENOMEM on noqueue/bridge interfaces.  v5 also covers
the GSO segment loop, as Sabrina pointed out.

Patch 2 fixes a use-after-free found while looking at that GSO loop:
validate_xmit_xfrm() unlinks async-stolen segments but never updates the
list head ->prev, which validate_xmit_skb_list() later dereferences.

Changes in v5:
 - 1/2: also propagate ERR_PTR(-EINPROGRESS) from the GSO segment loop
   (the 2nd ->xmit() call); v4 only handled the single-skb path.  Restore
   the blank line in validate_xmit_skb_list().  Add the missing
   maintainers to Cc. (Sabrina Dubroca)
 - 2/2: new patch -- fix the stale skb->prev use-after-free (also flagged
   by Sashiko)

Changes in v4:
 - Drop bool stolen tracking and the ERR_PTR return in
   validate_xmit_skb_list(); use IS_ERR_OR_NULL() so stolen skbs are
   silently skipped (Sabrina Dubroca)
 - Drop ERR_PTR(-EINPROGRESS) handling in __dev_direct_xmit() (Sabrina Dubroca)
 - Move validate_xmit_skb() return-value comment above the function
   (Sabrina Dubroca)

Changes in v3:
 - validate_xmit_skb_list(): set stolen=true only for -EINPROGRESS
   (Sabrina Dubroca)

Changes in v2:
 - Reset rc to NET_XMIT_SUCCESS only when PTR_ERR(skb) == -EINPROGRESS
   (Sabrina Dubroca)

Petr Wozniak (2):
  xfrm: propagate -EINPROGRESS from validate_xmit_xfrm()
  xfrm: fix stale skb->prev after async crypto steals a GSO segment

 net/core/dev.c         | 10 ++++++++--
 net/xfrm/xfrm_device.c | 12 ++++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.51.0

[PATCH net v5 1/2] 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) from validate_xmit_xfrm() for the async
case so callers can tell it apart from a real drop.  Update
__dev_queue_xmit() to handle ERR_PTR(-EINPROGRESS) from
validate_xmit_skb() correctly.  Update validate_xmit_skb_list() to
use IS_ERR_OR_NULL() so that ERR_PTR(-EINPROGRESS) is not mistakenly
added to the transmitted list.

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

diff --git a/net/core/dev.c b/net/core/dev.c
index 5c01dfaa6c44..f7ffc4d29597 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4018,6 +4018,9 @@ static struct sk_buff *validate_xmit_unreadable_skb(struct sk_buff *skb,
 	return NULL;
 }
 
+/* Returns the skb on success, NULL if dropped, or ERR_PTR(-EINPROGRESS)
+ * if stolen by async xfrm crypto (delivered via xfrm_dev_resume()).
+ */
 static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev, bool *again)
 {
 	netdev_features_t features;
@@ -4089,7 +4092,7 @@ 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))
 			continue;
 
 		if (!head)
@@ -4860,8 +4863,11 @@ 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)) {
+			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 630f3dd31cc5..19c77f09acc9 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);
@@ -224,7 +224,7 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur
 		pskb = skb2;
 	}
 
-	return skb;
+	return skb ? skb : ERR_PTR(-EINPROGRESS);
 }
 EXPORT_SYMBOL_GPL(validate_xmit_xfrm);
 
-- 
2.51.0

[PATCH net v5 2/2] xfrm: fix stale skb->prev after async crypto steals a GSO segment

[Petr Wozniak]

skb_gso_segment() leaves the segment list head with ->prev pointing at
the last segment, an invariant validate_xmit_skb_list() relies on when
it sets its tail pointer (tail = skb->prev).

When validate_xmit_xfrm() walks a GSO list and some segments are stolen
by async crypto (->xmit() returns -EINPROGRESS), those segments are
unlinked from the list but the head ->prev is never updated.  If the
last segment is the one stolen, the returned head still has ->prev
pointing at it, even though it is now owned by the crypto engine and may
be freed.  validate_xmit_skb_list() later does tail->next = skb, writing
through that stale pointer -- a use-after-free.

Repoint skb->prev at the last retained segment before returning.

Fixes: f53c723902d1 ("net: Add asynchronous callbacks for xfrm on layer 2.")
Signed-off-by: Petr Wozniak <petr.wozniak@gmail.com>
---
 net/xfrm/xfrm_device.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 19c77f09acc9..aec1e1184a71 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -224,6 +224,14 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur
 		pskb = skb2;
 	}
 
+	/* skb_gso_segment() set skb->prev to the last segment, but async
+	 * crypto may have stolen it above without updating ->prev.  Repoint
+	 * it at the last retained segment so validate_xmit_skb_list() does
+	 * not chain onto a segment now owned by the crypto engine.
+	 */
+	if (skb)
+		skb->prev = pskb;
+
 	return skb ? skb : ERR_PTR(-EINPROGRESS);
 }
 EXPORT_SYMBOL_GPL(validate_xmit_xfrm);
-- 
2.51.0

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

[Petr Wozniak]

Reposting on the list, as you asked.

Apologies for missing your comment about the 2nd x->type_offload->xmit()
call across several versions -- entirely my fault.

You're right: in the skb_list_walk_safe() loop, if all GSO segments
return -EINPROGRESS, skb is advanced to NULL and the function returns
NULL instead of ERR_PTR(-EINPROGRESS).  v5 1/2 fixes it:

	-	return skb;
	+	return skb ? skb : ERR_PTR(-EINPROGRESS);

At that point NULL can only mean all segments were stolen -- the error
path (err != -EINPROGRESS) returns NULL directly from inside the loop.

v5 1/2 also restores the blank line in validate_xmit_skb_list() and adds
the missing maintainers to Cc.

For the use-after-free I mentioned: I confirmed it.  validate_xmit_xfrm()
unlinks async-stolen segments but never updates the list head ->prev, so
when the last segment is stolen, validate_xmit_skb_list() chains onto it
via tail->next.  v5 2/2 fixes it by repointing skb->prev at the last
retained segment.  As you suggested, the two fixes go as a series.

I could not confirm the head-list leak on a closer look, so I left it
out; I'll send a separate patch if I find it.

The v5 series has been sent.

Thanks,
Petr