On Fri, 2017-01-27 at 10:49 +0000, David Laight wrote:
> From: Eric Dumazet
> > Sent: 27 January 2017 00:21
> > Slava Shwartsman reported a warning in skb_try_coalesce(), when we
> > detect skb->truesize is completely wrong.
> >
> > In his case, issue came from IPv6 reassembly coping with malicious
> > datagrams, that forced various pskb_may_pull() to reallocate a bigger
> > skb->head than the one allocated by NIC driver before entering GRO
> > layer.
> >
> > Current code does not change skb->truesize, leaving this burden to
> > callers if they care enough.
> >
> > Blindly changing skb->truesize in pskb_expand_head() is not
> > easy, as some producers might track skb->truesize, for example
> > in xmit path for back pressure feedback (sk->sk_wmem_alloc)
> >
> > We can detect the cases where it should be safe to change
> > skb->truesize :
> >
> > 1) skb is not attached to a socket.
> > 2) If it is attached to a socket, destructor is sock_edemux()
> ...
> > int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
> > gfp_t gfp_mask)
> > {
> > + int i, osize = skb_end_offset(skb);
> > + int size = osize + nhead + ntail;
> > long off;
> > + u8 *data;
> >
> > BUG_ON(nhead < 0);
> >
> > @@ -1257,6 +1257,14 @@ int pskb_expand_head(struct sk_buff *skb, int nhead,
> > int ntail,
> > skb->hdr_len = 0;
> > skb->nohdr = 0;
> > atomic_set(&skb_shinfo(skb)->dataref, 1);
> > +
> > + /* It is not generally safe to change skb->truesize.
> > + * For the moment, we really care of rx path, or
> > + * when skb is orphaned (not attached to a socket)
> > + */
> > + if (!skb->sk || skb->destructor == sock_edemux)
> > + skb->truesize += size - osize;
>
> That calculation doesn't look right to me at all.
> Isn't 'truesize' supposed to reflect the amount of memory allocated to the
> skb.
> So you need the difference between the size of the new and old memory blocks.
>
Well, please take a look at the code, because I believe I did exactly
that.
> I'm also guessing that extra headroom can be generated by stealing unused
> tailroom.
This is already done.
Quoting
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=87fb4b7b533073eeeaed0b6bf7c2328995f6c075
At skb alloc phase, we put skb_shared_info struct at the exact end of
skb head, to allow a better use of memory (lowering number of
reallocations), since kmalloc() gives us power-of-two memory blocks.
