Re: [PATCH net] inet: frags: fix a possible frag_mem_limit drift

[Jakub Kicinski <kuba@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.
---
inet: frags: fix a possible frag_mem_limit drift

This patch attempts to compensate for a frag_mem_limit accounting drift
in inet_frag_reasm_prepare() when an incoming fragment that completes
reassembly is not the first fragment, by adding the difference between
skb->truesize and head->truesize to fqdir->mem before consuming head.

> diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
> index 393770920abd..3222cc2ac235 100644
> --- a/net/ipv4/inet_fragment.c
> +++ b/net/ipv4/inet_fragment.c
> @@ -539,6 +539,9 @@ void *inet_frag_reasm_prepare(struct inet_frag_queue *q, struct sk_buff *skb,
>  		FRAG_CB(skb)->next_frag = FRAG_CB(head)->next_frag;
>  		rb_replace_node(&head->rbnode, &skb->rbnode,
>  				&q->rb_fragments);
> +		delta = skb->truesize - head->truesize;
> +		if (delta)
> +			add_frag_mem_limit(q->fqdir, delta);

[High]
Can the new delta computation ever evaluate to a non-zero value at this
point?

Looking a few lines above in the same function, skb_morph(skb, head) is
invoked before this hunk:

	skb_morph(skb, head);
	FRAG_CB(skb)->next_frag = FRAG_CB(head)->next_frag;
	rb_replace_node(&head->rbnode, &skb->rbnode,
			&q->rb_fragments);
	delta = skb->truesize - head->truesize;

In net/core/skbuff.c, skb_morph() is implemented as:

	struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src)
	{
		skb_release_all(dst, SKB_CONSUMED);
		return __skb_clone(dst, src);
	}

and __skb_clone() copies truesize from src to dst via the C(x) macro:

	#define C(x) n->x = skb->x
	...
	C(truesize);

So by the time the new code runs, skb->truesize has already been
overwritten with head->truesize, and the subtraction is identically
zero. The if (delta) branch never fires and add_frag_mem_limit() is
never called.

If the drift described in the changelog is real (the original
skb->truesize accounted via ip_frag_queue() differs from the clone fp's
truesize that ends up being subtracted later), would the compensation
need to be computed before skb_morph() overwrites skb->truesize, or
expressed against fp->truesize rather than the post-morph skb->truesize?

>  		consume_skb(head);
>  		head = skb;
>  	}
-- 
pw-bot: cr