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

[Simon Horman <horms@kernel.org>]

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?