netdev - Re: [PATCH 05/10] net: move destructor_arg to the front of sk

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]

Date:	Tue, 10 Apr 2012 16:19:19 +0100
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	"Wei Liu (Intern)" <wei.liu2@...rix.com>,
	"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>
Subject: Re: [PATCH 05/10] net: move destructor_arg to the front of sk_buff.

On Tue, 2012-04-10 at 16:05 +0100, Eric Dumazet wrote:
> On Tue, 2012-04-10 at 15:26 +0100, Ian Campbell wrote:
> > As of the previous patch we align the end (rather than the start) of the struct
> > to a cache line and so, with 32 and 64 byte cache lines and the shinfo size
> > increase from the next patch, the first 8 bytes of the struct end up on a
> > different cache line to the rest of it so make sure it is something relatively
> > unimportant to avoid hitting an extra cache line on hot operations such as
> > kfree_skb.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@...rix.com>
> > Cc: "David S. Miller" <davem@...emloft.net>
> > Cc: Eric Dumazet <eric.dumazet@...il.com>
> > ---
> >  include/linux/skbuff.h |   15 ++++++++++-----
> >  net/core/skbuff.c      |    5 ++++-
> >  2 files changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 0ad6a46..f0ae39c 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -265,6 +265,15 @@ struct ubuf_info {
> >   * the end of the header data, ie. at skb->end.
> >   */
> >  struct skb_shared_info {
> > +	/* Intermediate layers must ensure that destructor_arg
> > +	 * remains valid until skb destructor */
> > +	void		*destructor_arg;
> > +
> > +	/*
> > +	 * Warning: all fields from here until dataref are cleared in
> > +	 * __alloc_skb()
> > +	 *
> > +	 */
> >  	unsigned char	nr_frags;
> >  	__u8		tx_flags;
> >  	unsigned short	gso_size;
> > @@ -276,14 +285,10 @@ struct skb_shared_info {
> >  	__be32          ip6_frag_id;
> >  
> >  	/*
> > -	 * Warning : all fields before dataref are cleared in __alloc_skb()
> > +	 * Warning: all fields before dataref are cleared in __alloc_skb()
> >  	 */
> >  	atomic_t	dataref;
> >  
> > -	/* Intermediate layers must ensure that destructor_arg
> > -	 * remains valid until skb destructor */
> > -	void *		destructor_arg;
> > -
> >  	/* must be last field, see pskb_expand_head() */
> >  	skb_frag_t	frags[MAX_SKB_FRAGS];
> >  };
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index d4e139e..b8a41d6 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -214,7 +214,10 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
> >  
> >  	/* make sure we initialize shinfo sequentially */
> >  	shinfo = skb_shinfo(skb);
> > -	memset(shinfo, 0, offsetof(struct skb_shared_info, dataref));
> > +
> > +	memset(&shinfo->nr_frags, 0,
> > +	       offsetof(struct skb_shared_info, dataref)
> > +	       - offsetof(struct skb_shared_info, nr_frags));
> >  	atomic_set(&shinfo->dataref, 1);
> >  	kmemcheck_annotate_variable(shinfo->destructor_arg);
> >  
> 
> Not sure if we can do the same in build_skb()

I don't think there's any chance of there being a destructor_arg to
preserve in that case?

Regardless of that though I think for consistency it would be worth
pulling the common shinfo init out into a helper and using it in both
places.

I'll make that change.

Ian.

> 
> 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html