[ofa-general] Re: [PATCH] connected mode leaks pages on receive & reuses old skb

[Michael S. Tsirkin]

The following claim seems wrong, too:

> NOTE that [frags..IPOIB_CM_RX_SG-1] are *not* copied to newskb and
> thus leaked. Also, newskb's nr_frags is set based on the message size
> rather than the full size.

Not really. nr_frags *starts* at the message size.
skb_fill_page_desc will fill in the fargment in toskb, and
increment nr_frags until it matches the full size.

> @@ -378,23 +382,26 @@ static void skb_put_frags(struct sk_buff *skb, unsigned int hdr_space,
>  	skb->len += size;
>  	length -= size;
>  
> -	num_frags = skb_shinfo(skb)->nr_frags;
> +	num_frags = skb_shinfo(toskb)->nr_frags;
>  	for (i = 0; i < num_frags; i++) {
>  		skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
>  
> -		if (length == 0) {
> -			/* don't need this page */
> -			skb_fill_page_desc(toskb, i, frag->page, 0, PAGE_SIZE);
> -			--skb_shinfo(skb)->nr_frags;
> -		} else {
> -			size = min(length, (unsigned) PAGE_SIZE);
> -
> -			frag->size = size;
> -			skb->data_len += size;
> -			skb->truesize += size;
> -			skb->len += size;
> -			length -= size;
> -		}
> +		size = min(length, (unsigned) PAGE_SIZE);
> +		WARN_ON(size == 0);
> +		frag->size = size;
> +		skb->data_len += size;
> +		skb->truesize += size;
> +		skb->len += size;
> +		length -= size;
> +	}
> +	WARN_ON(length != 0);
> +	skb_shinfo(skb)->nr_frags = num_frags;
> +
> +	/* move unused pages from skb to toskb */
> +	for (; i < IPOIB_CM_RX_SG - 1; i++) {
> +		skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> +
> +		skb_fill_page_desc(toskb, i, frag->page, 0, PAGE_SIZE);
>  	}
>  }
>  

I don't see how this chunk changes anything.
It seems you have basically replaced a single loop with
a condition by two loops.

-- 
MST