[PATCH] xfrm: iptfs: propagate SKBFL_SHARED_FRAG in iptfs_skb_add_frags()

[PATCH] xfrm: iptfs: propagate SKBFL_SHARED_FRAG in iptfs_skb_add_frags()

[Chen YanJun]

From: Chen YanJun <moomichen@tencent.com>

When iptfs_skb_add_frags() copies frag references from the source
frag walk into a new SKB, it increments the page reference count via
__skb_frag_ref() but does not propagate SKBFL_SHARED_FRAG to the
destination SKB's skb_shinfo->flags.

If the source SKB carries shared frags (e.g. from a page-pool backed
receive path), the new inner SKB will appear to ESP as having privately
owned frags.  A subsequent esp_input() call for a nested transport-mode
SA then takes the no-COW fast path and decrypts in place, writing over
pages that are still referenced by the outer IPTFS SKB.  This causes
kernel-visible memory corruption and can trigger a panic.

All other frag-transfer helpers in the kernel (skb_try_coalesce,
skb_gro_receive, __pskb_copy_fclone, skb_shift, skb_segment) correctly
propagate SKBFL_SHARED_FRAG; align iptfs_skb_add_frags() with this
convention.

Fixes: 5f2b6a909574 ("xfrm: iptfs: add skb-fragment sharing code")
Signed-off-by: Chen YanJun <moomichen@tencent.com>
---
 net/xfrm/xfrm_iptfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index ad810d1f97c0..0e0dcf47a470 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -496,6 +496,10 @@ static int iptfs_skb_add_frags(struct sk_buff *skb,
 		walk->past += frag->len;	/* careful, use src bv_len */
 		walk->fragi++;
 	}
+
+	if (skb_shinfo(skb)->nr_frags)
+		skb_shinfo(skb)->flags |= SKBFL_SHARED_FRAG;
+
 	return len;
 }
 
-- 
2.47.0

Re: [PATCH] xfrm: iptfs: propagate SKBFL_SHARED_FRAG in iptfs_skb_add_frags()

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
xfrm: iptfs: propagate SKBFL_SHARED_FRAG in iptfs_skb_add_frags()

This patch sets SKBFL_SHARED_FRAG on the destination SKB after
iptfs_skb_add_frags() transfers frag references from a frag walk, to
prevent ESP's no-COW fast path from decrypting in place over pages still
referenced by the outer IPTFS SKB.

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index ad810d1f97c0..0e0dcf47a470 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -496,6 +496,10 @@ static int iptfs_skb_add_frags(struct sk_buff *skb,
>  		walk->past += frag->len;	/* careful, use src bv_len */
>  		walk->fragi++;
>  	}
> +
> +	if (skb_shinfo(skb)->nr_frags)
> +		skb_shinfo(skb)->flags |= SKBFL_SHARED_FRAG;
> +
>  	return len;
>  }

[High]
Does this placement cover every path that adds frags to the destination
SKB?  The new assignment sits after the while loop, but the loop has an
earlier exit that returns once the requested length is satisfied
mid-frag:

    while (len && walk->fragi < walk->nr_frags) {
        skb_frag_t *frag = &walk->frags[walk->fragi];
        skb_frag_t *tofrag = &shinfo->frags[shinfo->nr_frags];

        *tofrag = *frag;
        if (offset) {
            tofrag->offset += offset;
            tofrag->len -= offset;
            offset = 0;
        }
        __skb_frag_ref(tofrag);
        shinfo->nr_frags++;

        /* see if we are done */
        fraglen = tofrag->len;
        if (len < fraglen) {
            tofrag->len = len;
            skb->len += len;
            skb->data_len += len;
            return 0;
        }
        ...
    }

By this point __skb_frag_ref(tofrag) has run and shinfo->nr_frags has
been incremented, so frags have been attached, but the early return 0
bypasses the new SKBFL_SHARED_FRAG assignment.

The caller iptfs_pskb_add_frags() typically requests fewer bytes than
the current source frag holds, so this early-return path is the common
termination of the loop.  In that case the destination SKB still appears
to ESP as having privately owned frags, and the nested transport-mode
esp_input() no-COW path described in the commit message can still
decrypt in place over pages referenced by the outer IPTFS SKB.

Would it be more robust to set the flag immediately after
__skb_frag_ref(tofrag) / shinfo->nr_frags++, or duplicate the assignment
before the inner return 0, so every path that attaches a frag also
propagates SKBFL_SHARED_FRAG?