netdev - Re: [PATCH net-next] net: adjust skb->truesize in pskb_expand

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1485528261.6360.38.camel@edumazet-glaptop3.roam.corp.google.com>
Date:   Fri, 27 Jan 2017 06:44:21 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Slava Shwartsman <slavash@...lanox.com>
Subject: Re: [PATCH net-next] net: adjust skb->truesize in pskb_expand_head()

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.