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: <063D6719AE5E284EB5DD2968C1650D6DB0270D8B@AcuExch.aculab.com>
Date:   Fri, 27 Jan 2017 15:46:04 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Eric Dumazet' <eric.dumazet@...il.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()

From: Eric Dumazet [mailto:eric.dumazet@...il.com]
> Sent: 27 January 2017 14:44
> 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.

Reads code ...
My confusion is that the call is specifying the number of EXTRA bytes of head/tail
room rather than the number of bytes needed.

> > 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=87fb4b7b533073eeeaed0b6bf7c
> 2328995f6c075
> 
>     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.

It looks as though that just makes all the 'spare' space tailroom.
My guess is that headroom is needed more often than tailroom.
It doesn't look as though the amount of tailroom that has actually been requested
is saved either (nor headroom for that matter).

I was thinking that pskb_expand_head(skb, 16, -16, ...) could be implemented
(mostly) with memmove().

Hmmm.... Also if a caller asks for 3 extra bytes of headroom you really want to
allocate 8 extra bytes so that the memcpy() is aligned.

	David