[RFC] Wild and crazy ideas involving struct sk_buff

[RFC] Wild and crazy ideas involving struct sk_buff

[Paul Moore]

Over in LSM/SELinux land there has been a lot of talk recently about how to 
deal with loopback and forwarded traffic, specifically, how to preserve the 
sender's security label on those two types of traffic.  Yes, there is the 
existing sk_buff.secmark field but that is already being used for something 
else and utilizing it for this purpose has it's pros/cons.

We're currently talking about several different ideas to solve the problem, 
including leveraging the sk_buff.secmark field, and one of the ideas was to 
add an additional field to the sk_buff structure.  Knowing how well that idea 
would go over (lead balloon is probably an understatement at best) I started 
looking at what I might be able to remove from the sk_buff struct to make 
room for a new field (the new field would be a u32).  Looking at the sk_buff 
structure it appears that the sk_buff.dev and sk_buff.iif fields are a bit 
redundant and removing the sk_buff.dev field could free 32/64 bits depending 
on the platform.  Is there any reason (performance?) for keeping the 
sk_buff.dev field around?  Would the community be open to patches which 
removed it and transition users over to the sk_buff.iif field?  Finally, 
assuming the sk_buff.dev field was removed, would the community be open to 
adding a new LSM/SELinux related u32 field to the sk_buff struct?

Thanks.
 
-- 
paul moore
linux security @ hp

Re: [RFC] Wild and crazy ideas involving struct sk_buff

[David Miller]

From: Paul Moore <paul.moore@hp.com>
Date: Wed, 22 Aug 2007 16:31:34 -0400

> We're currently talking about several different ideas to solve the problem, 
> including leveraging the sk_buff.secmark field, and one of the ideas was to 
> add an additional field to the sk_buff structure.  Knowing how well that idea 
> would go over (lead balloon is probably an understatement at best) I started 
> looking at what I might be able to remove from the sk_buff struct to make 
> room for a new field (the new field would be a u32).  Looking at the sk_buff 
> structure it appears that the sk_buff.dev and sk_buff.iif fields are a bit 
> redundant and removing the sk_buff.dev field could free 32/64 bits depending 
> on the platform.  Is there any reason (performance?) for keeping the 
> sk_buff.dev field around?  Would the community be open to patches which 
> removed it and transition users over to the sk_buff.iif field?  Finally, 
> assuming the sk_buff.dev field was removed, would the community be open to 
> adding a new LSM/SELinux related u32 field to the sk_buff struct?

It's there for performance, and I bet there might be some semantic
issues involved.

And ironically James Morris still owes me a struct sk_buff removal
from when I let him put the "secmark" thing in there!

Stop spending money you guys haven't earned yet :-)

Re: [RFC] Wild and crazy ideas involving struct sk_buff

[Thomas Graf]

* Paul Moore <paul.moore@hp.com> 2007-08-22 16:31
> We're currently talking about several different ideas to solve the problem, 
> including leveraging the sk_buff.secmark field, and one of the ideas was to 
> add an additional field to the sk_buff structure.  Knowing how well that idea 
> would go over (lead balloon is probably an understatement at best) I started 
> looking at what I might be able to remove from the sk_buff struct to make 
> room for a new field (the new field would be a u32).  Looking at the sk_buff 
> structure it appears that the sk_buff.dev and sk_buff.iif fields are a bit 
> redundant and removing the sk_buff.dev field could free 32/64 bits depending 
> on the platform.  Is there any reason (performance?) for keeping the 
> sk_buff.dev field around?  Would the community be open to patches which 
> removed it and transition users over to the sk_buff.iif field?  Finally, 
> assuming the sk_buff.dev field was removed, would the community be open to 
> adding a new LSM/SELinux related u32 field to the sk_buff struct?

This reminds of an idea someone brought up a while ago, it involved
having a way to attach additional space to an sk_buff for all the
different marks and other non-essential fields.

I think skb->dev is required because we need to have a reference on the
device while a packet being processing is put on a queue somewhere.

Re: [RFC] Wild and crazy ideas involving struct sk_buff

[Paul Moore]

On Wednesday, August 22 2007 5:08:05 pm David Miller wrote:
> From: Paul Moore <paul.moore@hp.com>
> Date: Wed, 22 Aug 2007 16:31:34 -0400
>
> > We're currently talking about several different ideas to solve the
> > problem, including leveraging the sk_buff.secmark field, and one of the
> > ideas was to add an additional field to the sk_buff structure.  Knowing
> > how well that idea would go over (lead balloon is probably an
> > understatement at best) I started looking at what I might be able to
> > remove from the sk_buff struct to make room for a new field (the new
> > field would be a u32).  Looking at the sk_buff structure it appears that
> > the sk_buff.dev and sk_buff.iif fields are a bit redundant and removing
> > the sk_buff.dev field could free 32/64 bits depending on the platform. 
> > Is there any reason (performance?) for keeping the sk_buff.dev field
> > around?  Would the community be open to patches which removed it and
> > transition users over to the sk_buff.iif field?  Finally, assuming the
> > sk_buff.dev field was removed, would the community be open to adding a
> > new LSM/SELinux related u32 field to the sk_buff struct?
>
> It's there for performance, and I bet there might be some semantic
> issues involved.

Okay, thought that was probably the case considering the efforts to shrink the 
sk_buff as much as possible.

> And ironically James Morris still owes me a struct sk_buff removal
> from when I let him put the "secmark" thing in there!
>
> Stop spending money you guys haven't earned yet :-)

Hey, I was just asking how much it cost ... but then again, you know the old 
adage, "if you have to ask, you can't afford it" ;)

Thanks.

-- 
paul moore
linux security @ hp

Re: [RFC] Wild and crazy ideas involving struct sk_buff

[Paul Moore]

On Wednesday, August 22 2007 5:20:05 pm Thomas Graf wrote:
> * Paul Moore <paul.moore@hp.com> 2007-08-22 16:31
>
> > We're currently talking about several different ideas to solve the
> > problem, including leveraging the sk_buff.secmark field, and one of the
> > ideas was to add an additional field to the sk_buff structure.  Knowing
> > how well that idea would go over (lead balloon is probably an
> > understatement at best) I started looking at what I might be able to
> > remove from the sk_buff struct to make room for a new field (the new
> > field would be a u32).  Looking at the sk_buff structure it appears that
> > the sk_buff.dev and sk_buff.iif fields are a bit redundant and removing
> > the sk_buff.dev field could free 32/64 bits depending on the platform. 
> > Is there any reason (performance?) for keeping the sk_buff.dev field
> > around?  Would the community be open to patches which removed it and
> > transition users over to the sk_buff.iif field?  Finally, assuming the
> > sk_buff.dev field was removed, would the community be open to adding a
> > new LSM/SELinux related u32 field to the sk_buff struct?
>
> This reminds of an idea someone brought up a while ago, it involved
> having a way to attach additional space to an sk_buff for all the
> different marks and other non-essential fields.

Interesting.  Was it just a thought, or was there some actual 
design/code/patchset to go along with it that described the idea?

-- 
paul moore
linux security @ hp

Re: [RFC] Wild and crazy ideas involving struct sk_buff

[David Miller]

From: Paul Moore <paul.moore@hp.com>
Date: Wed, 22 Aug 2007 17:26:36 -0400

> Was it just a thought, or was there some actual design/code/patchset
> to go along with it that described the idea?

It was a thought mentioned at the first two netconfs, but it
went nowhere because the more we discussed the implementation
the more horrific it began to sound :-)

Re: [RFC] Wild and crazy ideas involving struct sk_buff

[Arnaldo Carvalho de Melo]

Em Wed, Aug 22, 2007 at 02:36:27PM -0700, David Miller escreveu:
> From: Paul Moore <paul.moore@hp.com>
> Date: Wed, 22 Aug 2007 17:26:36 -0400
> 
> > Was it just a thought, or was there some actual design/code/patchset
> > to go along with it that described the idea?
> 
> It was a thought mentioned at the first two netconfs, but it
> went nowhere because the more we discussed the implementation
> the more horrific it began to sound :-)

Which makes it more interesting to make it work :-)

Anyway, there is space there to be used, in 32 and 64 bits we have two
bytes after queue_mapping (this kernel doesn't have CONFIG_NET_SCHED nor
CONFIG_NET_CLS_ACT enabled, if the first was we wouldn't have the hole,
but if both, as its common in distro kernels, were we would have the 2
bytes hole, as we can see on the pahole output for the 64bits kernel,
that uses the kernel config used in the Red Hat kernels.

Bonus points would go to continue the skb offset fight and reduce the
size of mac_header to two bytes, and then moving it to just after
queue_mapping. Do that and you'll get some credits :-P

- Arnaldo

[acme@mica net-2.6.24]$ pahole -C sk_buff ../32bits/net/core/skbuff.o
struct sk_buff {
	struct sk_buff *           next;                 /*     0     4 */
	struct sk_buff *           prev;                 /*     4     4 */
	struct sock *              sk;                   /*     8     4 */
	ktime_t                    tstamp;               /*    12     8 */
	struct net_device *        dev;                  /*    20     4 */
	struct dst_entry *         dst;                  /*    24     4 */
	struct sec_path *          sp;                   /*    28     4 */
	char                       cb[48];               /*    32    48 */
	/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
	unsigned int               len;                  /*    80     4 */
	unsigned int               data_len;             /*    84     4 */
	__u16                      mac_len;              /*    88     2 */
	__u16                      hdr_len;              /*    90     2 */
	union {
		__wsum             csum;                 /*           4 */
		struct {
			__u16      csum_start;           /*    92     2 */
			__u16      csum_offset;          /*    94     2 */
		}                                        /*           4 */
	};                                               /*    92     4 */
	__u32                      priority;             /*    96     4 */
	__u8                       local_df:1;           /*   100     1 */
	__u8                       cloned:1;             /*   100     1 */
	__u8                       ip_summed:2;          /*   100     1 */
	__u8                       nohdr:1;              /*   100     1 */
	__u8                       nfctinfo:3;           /*   100     1 */
	__u8                       pkt_type:3;           /*   101     1 */
	__u8                       fclone:2;             /*   101     1 */
	__u8                       ipvs_property:1;      /*   101     1 */
	__u8                       nf_trace:1;           /*   101     1 */

	/* XXX 1 bit hole, try to pack */

	__be16                     protocol;             /*   102     2 */
	void            (*destructor)(struct sk_buff *); /*   104     4 */
	int                        iif;                  /*   108     4 */
	__u16                      queue_mapping;        /*   112     2 */

	/* XXX 2 bytes hole, try to pack */

	__u32                      mark;                 /*   116     4 */
	sk_buff_data_t             transport_header;     /*   120     4 */
	sk_buff_data_t             network_header;       /*   124     4 */
	/* --- cacheline 2 boundary (128 bytes) --- */
	sk_buff_data_t             mac_header;           /*   128     4 */
	sk_buff_data_t             tail;                 /*   132     4 */
	sk_buff_data_t             end;                  /*   136     4 */
	unsigned char *            head;                 /*   140     4 */
	unsigned char *            data;                 /*   144     4 */
	unsigned int               truesize;             /*   148     4 */
	atomic_t                   users;                /*   152     4 */

	/* size: 156, cachelines: 3 */
	/* sum members: 154, holes: 1, sum holes: 2 */
	/* bit holes: 1, sum bit holes: 1 bits */
	/* last cacheline: 28 bytes */
};

[acme@mica net-2.6.24]$ pahole -C sk_buff ../64bits/net/core/skbuff.o
struct sk_buff {
	struct sk_buff *           next;                 /*     0     8 */
	struct sk_buff *           prev;                 /*     8     8 */
	struct sock *              sk;                   /*    16     8 */
	ktime_t                    tstamp;               /*    24     8 */
	struct net_device *        dev;                  /*    32     8 */
	struct dst_entry *         dst;                  /*    40     8 */
	struct sec_path *          sp;                   /*    48     8 */
	char                       cb[48];               /*    56    48 */
	/* --- cacheline 1 boundary (64 bytes) was 40 bytes ago --- */
	unsigned int               len;                  /*   104     4 */
	unsigned int               data_len;             /*   108     4 */
	__u16                      mac_len;              /*   112     2 */
	__u16                      hdr_len;              /*   114     2 */
	union {
		__wsum             csum;                 /*           4 */
		struct {
			__u16      csum_start;           /*   116     2 */
			__u16      csum_offset;          /*   118     2 */
		}                                        /*           4 */
	};                                               /*   116     4 */
	__u32                      priority;             /*   120     4 */
	__u8                       local_df:1;           /*   124     1 */
	__u8                       cloned:1;             /*   124     1 */
	__u8                       ip_summed:2;          /*   124     1 */
	__u8                       nohdr:1;              /*   124     1 */
	__u8                       nfctinfo:3;           /*   124     1 */
	__u8                       pkt_type:3;           /*   125     1 */
	__u8                       fclone:2;             /*   125     1 */
	__u8                       ipvs_property:1;      /*   125     1 */
	__u8                       nf_trace:1;           /*   125     1 */

	/* XXX 1 bit hole, try to pack */

	__be16                     protocol;             /*   126     2 */
	/* --- cacheline 2 boundary (128 bytes) --- */
	void            (*destructor)(struct sk_buff *); /*   128     8 */
	struct nf_conntrack *      nfct;                 /*   136     8 */
	struct sk_buff *           nfct_reasm;           /*   144     8 */
	struct nf_bridge_info *    nf_bridge;            /*   152     8 */
	int                        iif;                  /*   160     4 */
	__u16                      queue_mapping;        /*   164     2 */
	__u16                      tc_index;             /*   166     2 */
	__u16                      tc_verd;              /*   168     2 */

	/* XXX 2 bytes hole, try to pack */

	dma_cookie_t               dma_cookie;           /*   172     4 */
	__u32                      secmark;              /*   176     4 */
	__u32                      mark;                 /*   180     4 */
	sk_buff_data_t             transport_header;     /*   184     4 */
	sk_buff_data_t             network_header;       /*   188     4 */
	/* --- cacheline 3 boundary (192 bytes) --- */
	sk_buff_data_t             mac_header;           /*   192     4 */
	sk_buff_data_t             tail;                 /*   196     4 */
	sk_buff_data_t             end;                  /*   200     4 */

	/* XXX 4 bytes hole, try to pack */

	unsigned char *            head;                 /*   208     8 */
	unsigned char *            data;                 /*   216     8 */
	unsigned int               truesize;             /*   224     4 */
	atomic_t                   users;                /*   228     4 */

	/* size: 232, cachelines: 4 */
	/* sum members: 226, holes: 2, sum holes: 6 */
	/* bit holes: 1, sum bit holes: 1 bits */
	/* last cacheline: 40 bytes */
};

Re: [RFC] Wild and crazy ideas involving struct sk_buff

[James Morris]

On Wed, 22 Aug 2007, David Miller wrote:

> From: Paul Moore <paul.moore@hp.com>
> Date: Wed, 22 Aug 2007 17:26:36 -0400
> 
> > Was it just a thought, or was there some actual design/code/patchset
> > to go along with it that described the idea?
> 
> It was a thought mentioned at the first two netconfs, but it
> went nowhere because the more we discussed the implementation
> the more horrific it began to sound :-)

Don't forget Rusty's skb reservation patches from 1999...


-- 
James Morris
<jmorris@namei.org>

Re: [RFC] Wild and crazy ideas involving struct sk_buff

[Paul Moore]

On Wednesday, August 22 2007 6:09:36 pm James Morris wrote:
> On Wed, 22 Aug 2007, David Miller wrote:
> > From: Paul Moore <paul.moore@hp.com>
> > Date: Wed, 22 Aug 2007 17:26:36 -0400
> >
> > > Was it just a thought, or was there some actual design/code/patchset
> > > to go along with it that described the idea?
> >
> > It was a thought mentioned at the first two netconfs, but it
> > went nowhere because the more we discussed the implementation
> > the more horrific it began to sound :-)
>
> Don't forget Rusty's skb reservation patches from 1999...

I'm guessing those would probably need to be forward ported just a teensy 
weensy bit ;)

-- 
paul moore
linux security @ hp

Re: [RFC] Wild and crazy ideas involving struct sk_buff

[James Morris]

On Wed, 22 Aug 2007, Paul Moore wrote:

> > Don't forget Rusty's skb reservation patches from 1999...
> 
> I'm guessing those would probably need to be forward ported just a teensy 
> weensy bit ;)

The point being that the idea has been around since last century and has 
never gone anywhere :-)


-- 
James Morris
<jmorris@namei.org>