[PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

Commit 90ba9b1986b5ac4b2d184575847147ea7c4280a2 converted
tcp_make_synack() to use alloc_skb() directly instead of calling
sock_wmalloc(), the goal being the elimination of two atomic
operations.  Unfortunately, in doing so the change broke certain
SELinux/NetLabel configurations by no longer correctly assigning
the sock to the outgoing packet.

This patch fixes this regression by doing the skb->sk assignment
directly inside tcp_make_synack().

Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
Signed-off-by: Paul Moore <pmoore@redhat.com>
---
 net/ipv4/tcp_output.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5d0b438..23cc295 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2705,6 +2705,8 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 		dst_release(dst);
 		return NULL;
 	}
+	skb->sk = sk;
+
 	/* Reserve space for headers. */
 	skb_reserve(skb, MAX_TCP_HEADER);
 

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Mon, 08 Apr 2013 11:45:19 -0400

> Commit 90ba9b1986b5ac4b2d184575847147ea7c4280a2 converted
> tcp_make_synack() to use alloc_skb() directly instead of calling
> sock_wmalloc(), the goal being the elimination of two atomic
> operations.  Unfortunately, in doing so the change broke certain
> SELinux/NetLabel configurations by no longer correctly assigning
> the sock to the outgoing packet.
> 
> This patch fixes this regression by doing the skb->sk assignment
> directly inside tcp_make_synack().
> 
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Signed-off-by: Paul Moore <pmoore@redhat.com>

Setting skb->sk without the destructor results in an SKB that can live
potentially forever with a stale reference to a destroyed socket.

You cannot fix the problem in this way.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 12:14:34 PM David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Mon, 08 Apr 2013 11:45:19 -0400
> 
> > Commit 90ba9b1986b5ac4b2d184575847147ea7c4280a2 converted
> > tcp_make_synack() to use alloc_skb() directly instead of calling
> > sock_wmalloc(), the goal being the elimination of two atomic
> > operations.  Unfortunately, in doing so the change broke certain
> > SELinux/NetLabel configurations by no longer correctly assigning
> > the sock to the outgoing packet.
> > 
> > This patch fixes this regression by doing the skb->sk assignment
> > directly inside tcp_make_synack().
> > 
> > Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> > Signed-off-by: Paul Moore <pmoore@redhat.com>
> 
> Setting skb->sk without the destructor results in an SKB that can live
> potentially forever with a stale reference to a destroyed socket.
> 
> You cannot fix the problem in this way.

Okay, no worries, I'll work on v2.  For some reason I missed the destructor 
assignment in skb_set_owner_w(); I guess I was spending so much time hunting 
around looking for the missing skb->sk assignment that once I found it I 
declared victory ... a bit too soon.

Looking at the code again, I think the right solution is to call 
skb_set_owner_w() instead of doing the assignment directly but that is 
starting to bring us back to sock_wmalloc(force == 1) which gets back to 
Eric's comments ... (below) ...

On Monday, April 08, 2013 09:19:23 AM Eric Dumazet wrote:
> Keeping a pointer on a socket without taking a refcount is not going to
> work.
> 
> We are trying to make the stack scale, so you need to add a selinux call
> to take a ref count only if needed.
> 
> That is : If selinux is not used, we don't need to slow down the stack.

Contrary to popular belief, my goal is to not destroy the scalability and/or 
performance of our network stack, I just want to make sure we have a quality 
network stack that is not only fast and scalable, but also preserves the 
security functionality that makes Linux attractive to a number of users.  To 
that end, we could put a #ifdef in the middle of tcp_make_synack(), but that 
seems very ugly to me and I think sets a bad precedence for the network stack 
and kernel as a whole.

So a question for Dave, et al. - would you prefer that I fix this by:

1. Restore the original sock_wmalloc() call?
2. Keep things as-is with skb_alloc() but add skb_set_owner_w()?
3. Add a #ifdef depending on SELinux (probably the LSM in general to be safe) 
and use sock_wmalloc() if enabled, skb_alloc() if not?

I guess I'm leaning towards #1 for the sake of simplicity, but I'd be happy 
with either #1 or #2.  The #3 option seems like a hack and makes me a bit 
afraid of the future.  I am also open to suggestions; to me, the most 
important thing is that we fix this regression, I'm less concerned about how 
we do it.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Mon, 2013-04-08 at 13:22 -0400, Paul Moore wrote:
> On Monday, April 08, 2013 12:14:34 PM David Miller wrote:
> > From: Paul Moore <pmoore@redhat.com>
> > Date: Mon, 08 Apr 2013 11:45:19 -0400
> > 
> > > Commit 90ba9b1986b5ac4b2d184575847147ea7c4280a2 converted
> > > tcp_make_synack() to use alloc_skb() directly instead of calling
> > > sock_wmalloc(), the goal being the elimination of two atomic
> > > operations.  Unfortunately, in doing so the change broke certain
> > > SELinux/NetLabel configurations by no longer correctly assigning
> > > the sock to the outgoing packet.
> > > 
> > > This patch fixes this regression by doing the skb->sk assignment
> > > directly inside tcp_make_synack().
> > > 
> > > Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> > > Signed-off-by: Paul Moore <pmoore@redhat.com>
> > 
> > Setting skb->sk without the destructor results in an SKB that can live
> > potentially forever with a stale reference to a destroyed socket.
> > 
> > You cannot fix the problem in this way.
> 
> Okay, no worries, I'll work on v2.  For some reason I missed the destructor 
> assignment in skb_set_owner_w(); I guess I was spending so much time hunting 
> around looking for the missing skb->sk assignment that once I found it I 
> declared victory ... a bit too soon.
> 
> Looking at the code again, I think the right solution is to call 
> skb_set_owner_w() instead of doing the assignment directly but that is 
> starting to bring us back to sock_wmalloc(force == 1) which gets back to 
> Eric's comments ... (below) ...
> 
> On Monday, April 08, 2013 09:19:23 AM Eric Dumazet wrote:
> > Keeping a pointer on a socket without taking a refcount is not going to
> > work.
> > 
> > We are trying to make the stack scale, so you need to add a selinux call
> > to take a ref count only if needed.
> > 
> > That is : If selinux is not used, we don't need to slow down the stack.
> 
> Contrary to popular belief, my goal is to not destroy the scalability and/or 
> performance of our network stack, I just want to make sure we have a quality 
> network stack that is not only fast and scalable, but also preserves the 
> security functionality that makes Linux attractive to a number of users.  To 
> that end, we could put a #ifdef in the middle of tcp_make_synack(), but that 
> seems very ugly to me and I think sets a bad precedence for the network stack 
> and kernel as a whole.
> 
> So a question for Dave, et al. - would you prefer that I fix this by:
> 
> 1. Restore the original sock_wmalloc() call?
> 2. Keep things as-is with skb_alloc() but add skb_set_owner_w()?
> 3. Add a #ifdef depending on SELinux (probably the LSM in general to be safe) 
> and use sock_wmalloc() if enabled, skb_alloc() if not?

Didnt we had the same issue with RST packets ?

Please take a look at commit 3a7c384ffd57ef5fbd95f48edaa2ca4eb3d9f2ee

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 10:36:26 AM Eric Dumazet wrote:
> On Mon, 2013-04-08 at 13:22 -0400, Paul Moore wrote:
> > On Monday, April 08, 2013 12:14:34 PM David Miller wrote:
> > > From: Paul Moore <pmoore@redhat.com>
> > > Date: Mon, 08 Apr 2013 11:45:19 -0400
> > > 
> > > > Commit 90ba9b1986b5ac4b2d184575847147ea7c4280a2 converted
> > > > tcp_make_synack() to use alloc_skb() directly instead of calling
> > > > sock_wmalloc(), the goal being the elimination of two atomic
> > > > operations.  Unfortunately, in doing so the change broke certain
> > > > SELinux/NetLabel configurations by no longer correctly assigning
> > > > the sock to the outgoing packet.
> > > > 
> > > > This patch fixes this regression by doing the skb->sk assignment
> > > > directly inside tcp_make_synack().
> > > > 
> > > > Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> > > > Signed-off-by: Paul Moore <pmoore@redhat.com>
> > > 
> > > Setting skb->sk without the destructor results in an SKB that can live
> > > potentially forever with a stale reference to a destroyed socket.
> > > 
> > > You cannot fix the problem in this way.
> > 
> > Okay, no worries, I'll work on v2.  For some reason I missed the
> > destructor assignment in skb_set_owner_w(); I guess I was spending so much
> > time hunting around looking for the missing skb->sk assignment that once I
> > found it I declared victory ... a bit too soon.
> > 
> > Looking at the code again, I think the right solution is to call
> > skb_set_owner_w() instead of doing the assignment directly but that is
> > starting to bring us back to sock_wmalloc(force == 1) which gets back to
> > Eric's comments ... (below) ...
> > 
> > On Monday, April 08, 2013 09:19:23 AM Eric Dumazet wrote:
> > > Keeping a pointer on a socket without taking a refcount is not going to
> > > work.
> > > 
> > > We are trying to make the stack scale, so you need to add a selinux call
> > > to take a ref count only if needed.
> > > 
> > > That is : If selinux is not used, we don't need to slow down the stack.
> > 
> > Contrary to popular belief, my goal is to not destroy the scalability
> > and/or performance of our network stack, I just want to make sure we have
> > a quality network stack that is not only fast and scalable, but also
> > preserves the security functionality that makes Linux attractive to a
> > number of users.  To that end, we could put a #ifdef in the middle of
> > tcp_make_synack(), but that seems very ugly to me and I think sets a bad
> > precedence for the network stack and kernel as a whole.
> > 
> > So a question for Dave, et al. - would you prefer that I fix this by:
> > 
> > 1. Restore the original sock_wmalloc() call?
> > 2. Keep things as-is with skb_alloc() but add skb_set_owner_w()?
> > 3. Add a #ifdef depending on SELinux (probably the LSM in general to be
> > safe) and use sock_wmalloc() if enabled, skb_alloc() if not?
> 
> Didnt we had the same issue with RST packets ?
> 
> Please take a look at commit 3a7c384ffd57ef5fbd95f48edaa2ca4eb3d9f2ee

Sort of a similar problem, but not really the same.  Also, arguably, there is 
no real associated sock/socket for a RST so orphaning the packet makes sense.  
In the case of a SYNACK we can, and should, associate the packet with a 
sock/socket.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Mon, 2013-04-08 at 13:40 -0400, Paul Moore wrote:

> Sort of a similar problem, but not really the same.  Also, arguably, there is 
> no real associated sock/socket for a RST so orphaning the packet makes sense.  
> In the case of a SYNACK we can, and should, associate the packet with a 
> sock/socket.

What is the intent ?

This kind of requirement has a huge cost, and thats why I want a hook
instead of a 'generic thing'

Please read : 

http://workshop.netfilter.org/2013/wiki/images/3/3a/NFWS-TCP.pdf

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Mon, 2013-04-08 at 10:47 -0700, Eric Dumazet wrote:

> This kind of requirement has a huge cost, and thats why I want a hook
> instead of a 'generic thing'

I meant " I would like ... "

We for example have security_inet_csk_clone()

We could have security_skb_owned_by(skb, sk)

I probably can send a patch, it seems quite easy.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 10:47:47 AM Eric Dumazet wrote:
> On Mon, 2013-04-08 at 13:40 -0400, Paul Moore wrote:
> > Sort of a similar problem, but not really the same.  Also, arguably, there
> > is no real associated sock/socket for a RST so orphaning the packet makes
> > sense. In the case of a SYNACK we can, and should, associate the packet
> > with a sock/socket.
> 
> What is the intent ?

We have to do a number of painful things in SELinux because we aren't allowed 
a proper security blob (void *security) in a sk_buff.  One of those things is 
using the security blob in the originating sock as a stand-in for the packet's 
own security blob; as a result, when skb->sk is not set we have to make some 
guesses about the security attributes of packet.  We do have methods to handle 
packets without a valid sock pointer, but those are used primarily for non-
local packets (e.g. forwarded traffic) and some limited local use cases (e.g. 
TCP RST packets).

Also, don't mention skb->secmark; unfortunately that was used for something 
else and doesn't apply to this conversation.
 
> On Mon, 2013-04-08 at 10:47 -0700, Eric Dumazet wrote:
> > This kind of requirement has a huge cost, and thats why I want a hook
> > instead of a 'generic thing'
> 
> I meant " I would like ... "
> 
> We for example have security_inet_csk_clone()
> 
> We could have security_skb_owned_by(skb, sk)
>
> I probably can send a patch, it seems quite easy.

It seems a bit fragile to me, perhaps even hacky, but in some ways I guess it 
isn't anymore fragile than relying on skb->sk - as this problem demonstrates.  
My other concern is that adding this hook *correctly* is likely to touch a lot 
of files and may be a bit much so late in the 3.9 cycle, Dave, what say you?

Assuming that this is the preferred option, Eric, would you be open to 
reverting your patch for 3.9 with the assumption that I promise to add the 
hook for 3.10?  You've got my word that I'll have it done ASAP.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Mon, 2013-04-08 at 14:12 -0400, Paul Moore wrote:

> 
> It seems a bit fragile to me, perhaps even hacky, but in some ways I guess it 
> isn't anymore fragile than relying on skb->sk - as this problem demonstrates.  
> My other concern is that adding this hook *correctly* is likely to touch a lot 
> of files and may be a bit much so late in the 3.9 cycle, Dave, what say you?

I don't get it, 90ba9b1986b5ac4b2d18 was in 3.6, why do you care of
3.9 ?

I am preparing a fix right now. Not a revert, thank you.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 11:21:43 AM Eric Dumazet wrote:
> On Mon, 2013-04-08 at 14:12 -0400, Paul Moore wrote:
> > It seems a bit fragile to me, perhaps even hacky, but in some ways I guess
> > it isn't anymore fragile than relying on skb->sk - as this problem
> > demonstrates. My other concern is that adding this hook *correctly* is
> > likely to touch a lot of files and may be a bit much so late in the 3.9
> > cycle, Dave, what say you?
>
> I don't get it, 90ba9b1986b5ac4b2d18 was in 3.6, why do you care of
> 3.9 ?

I wasn't made aware of the particular bug until just recently.  When I see a 
regression I generally try to get it fixed as soon as possible.
 
> I am preparing a fix right now. Not a revert, thank you.

I guess we'll have to wait and see then; the more I think about the new hook 
you proposed the less enthused I am about it.

I'm still curious to hear what Dave has to say on this.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Mon, 2013-04-08 at 14:26 -0400, Paul Moore wrote:

> I guess we'll have to wait and see then; the more I think about the new hook 
> you proposed the less enthused I am about it.
> 
> I'm still curious to hear what Dave has to say on this.
> 

90ba9b1986b5ac4b2 is 10 months old, and nobody complained until today ?

This sounds like a very small issue to me, a revert is simply overkill. 

Lets go forward instead of applying blind fixes.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Mon, 2013-04-08 at 11:21 -0700, Eric Dumazet wrote:
> On Mon, 2013-04-08 at 14:12 -0400, Paul Moore wrote:
> 
> > 
> > It seems a bit fragile to me, perhaps even hacky, but in some ways I guess it 
> > isn't anymore fragile than relying on skb->sk - as this problem demonstrates.  
> > My other concern is that adding this hook *correctly* is likely to touch a lot 
> > of files and may be a bit much so late in the 3.9 cycle, Dave, what say you?
> 
> I don't get it, 90ba9b1986b5ac4b2d18 was in 3.6, why do you care of
> 3.9 ?
> 
> I am preparing a fix right now. Not a revert, thank you.
> 

Is the following patch not good enough ?

 include/linux/security.h |    7 +++++++
 net/ipv4/tcp_output.c    |    1 +
 security/security.c      |    6 ++++++
 3 files changed, 14 insertions(+)

diff --git a/include/linux/security.h b/include/linux/security.h
index eee7478..1bde8bb 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -2588,6 +2588,8 @@ int security_tun_dev_attach_queue(void *security);
 int security_tun_dev_attach(struct sock *sk, void *security);
 int security_tun_dev_open(void *security);
 
+void security_skb_owned_by(struct sk_buff *skb, struct sock *sk);
+
 #else	/* CONFIG_SECURITY_NETWORK */
 static inline int security_unix_stream_connect(struct sock *sock,
 					       struct sock *other,
@@ -2779,6 +2781,11 @@ static inline int security_tun_dev_open(void *security)
 {
 	return 0;
 }
+
+static inline void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
+{
+}
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5d0b438..b44cf81 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2709,6 +2709,7 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 	skb_reserve(skb, MAX_TCP_HEADER);
 
 	skb_dst_set(skb, dst);
+	security_skb_owned_by(skb, sk);
 
 	mss = dst_metric_advmss(dst);
 	if (tp->rx_opt.user_mss && tp->rx_opt.user_mss < mss)
diff --git a/security/security.c b/security/security.c
index 7b88c6a..cdabd4d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -25,6 +25,7 @@
 #include <linux/personality.h>
 #include <linux/backing-dev.h>
 #include <net/flow.h>
+#include <net/sock.h>
 
 #define MAX_LSM_EVM_XATTR	2
 
@@ -1290,6 +1291,11 @@ int security_tun_dev_open(void *security)
 }
 EXPORT_SYMBOL(security_tun_dev_open);
 
+void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
+{
+	skb_set_owner_w(skb, sk);
+}
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 11:30:25 AM Eric Dumazet wrote:
> On Mon, 2013-04-08 at 11:21 -0700, Eric Dumazet wrote:
> > On Mon, 2013-04-08 at 14:12 -0400, Paul Moore wrote:
> > > It seems a bit fragile to me, perhaps even hacky, but in some ways I
> > > guess it isn't anymore fragile than relying on skb->sk - as this
> > > problem demonstrates. My other concern is that adding this hook
> > > *correctly* is likely to touch a lot of files and may be a bit much so
> > > late in the 3.9 cycle, Dave, what say you?> 
> > I don't get it, 90ba9b1986b5ac4b2d18 was in 3.6, why do you care of
> > 3.9 ?
> > 
> > I am preparing a fix right now. Not a revert, thank you.
> 
> Is the following patch not good enough ?

I think it is somewhat telling that the hook you're proposing doesn't ever 
make any calls into any of the individual LSMs, it only calls back into the 
networking stack.  In my mind, this makes it an abuse of the LSM mechanism.

On Monday, April 08, 2013 11:34:11 AM Eric Dumazet wrote:
> On Mon, 2013-04-08 at 14:26 -0400, Paul Moore wrote:
> > I guess we'll have to wait and see then; the more I think about the new
> > hook you proposed the less enthused I am about it.
> > 
> > I'm still curious to hear what Dave has to say on this.
> 
> 90ba9b1986b5ac4b2 is 10 months old, and nobody complained until today ?

The people who use this functionality almost never use upstream kernels, they 
need to protection/certification/warm-fuzzies/etc. that come from a 
distribution kernel and a support infrastructure.  I didn't catch it because I 
use a slightly different configuration that didn't expose this bug; while I 
would like to run a full regression test every release I simply don't have the 
time to do that myself.

> This sounds like a very small issue to me, a revert is simply overkill.

It all depends on your use case.  To you, whom I assume doesn't use SELinux, 
it is indeed a trivial issue.  To someone who relies on SELinux for its 
network access controls this is a pretty significant issue.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Mon, 08 Apr 2013 16:37:22 -0400

> On Monday, April 08, 2013 11:30:25 AM Eric Dumazet wrote:
>> On Mon, 2013-04-08 at 11:21 -0700, Eric Dumazet wrote:
>> > On Mon, 2013-04-08 at 14:12 -0400, Paul Moore wrote:
>> > > It seems a bit fragile to me, perhaps even hacky, but in some ways I
>> > > guess it isn't anymore fragile than relying on skb->sk - as this
>> > > problem demonstrates. My other concern is that adding this hook
>> > > *correctly* is likely to touch a lot of files and may be a bit much so
>> > > late in the 3.9 cycle, Dave, what say you?> 
>> > I don't get it, 90ba9b1986b5ac4b2d18 was in 3.6, why do you care of
>> > 3.9 ?
>> > 
>> > I am preparing a fix right now. Not a revert, thank you.
>> 
>> Is the following patch not good enough ?
> 
> I think it is somewhat telling that the hook you're proposing doesn't ever 
> make any calls into any of the individual LSMs, it only calls back into the 
> networking stack.  In my mind, this makes it an abuse of the LSM mechanism.

Without LSMs the socket reference is spurious and pointless overhead,
therefore the only acceptable fix one which only takes the socket
reference when there are LSMs with a need.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 04:44:39 PM David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Mon, 08 Apr 2013 16:37:22 -0400
> 
> > On Monday, April 08, 2013 11:30:25 AM Eric Dumazet wrote:
> >> On Mon, 2013-04-08 at 11:21 -0700, Eric Dumazet wrote:
> >> > On Mon, 2013-04-08 at 14:12 -0400, Paul Moore wrote:
> >> > > It seems a bit fragile to me, perhaps even hacky, but in some ways I
> >> > > guess it isn't anymore fragile than relying on skb->sk - as this
> >> > > problem demonstrates. My other concern is that adding this hook
> >> > > *correctly* is likely to touch a lot of files and may be a bit much
> >> > > so late in the 3.9 cycle, Dave, what say you?>
> >> > 
> >> > I don't get it, 90ba9b1986b5ac4b2d18 was in 3.6, why do you care of
> >> > 3.9 ?
> >> > 
> >> > I am preparing a fix right now. Not a revert, thank you.
> >> 
> >> Is the following patch not good enough ?
> > 
> > I think it is somewhat telling that the hook you're proposing doesn't ever
> > make any calls into any of the individual LSMs, it only calls back into
> > the networking stack.  In my mind, this makes it an abuse of the LSM
> > mechanism.
> 
> Without LSMs the socket reference is spurious and pointless overhead,
> therefore the only acceptable fix one which only takes the socket
> reference when there are LSMs with a need.

Well that's frustrating, but if that is the game so be it.

Eric, your patch didn't cross-post to the LSM list, do you want to post it 
there for review/merging?  I would recommend also adding a pointer to this 
thread for reference.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Mon, 2013-04-08 at 16:37 -0400, Paul Moore wrote:

> The people who use this functionality almost never use upstream kernels, they 
> need to protection/certification/warm-fuzzies/etc. that come from a 
> distribution kernel and a support infrastructure.  I didn't catch it because I 
> use a slightly different configuration that didn't expose this bug; while I 
> would like to run a full regression test every release I simply don't have the 
> time to do that myself.
> 
> > This sounds like a very small issue to me, a revert is simply overkill.
> 
> It all depends on your use case.  To you, whom I assume doesn't use SELinux, 
> it is indeed a trivial issue.  To someone who relies on SELinux for its 
> network access controls this is a pretty significant issue.
> 

Is the patch I sent addressing the problem or not ?

Note that I do have : CONFIG_SECURITY=y

So this patch basically adds the overhead back, and I'll have to use
real hook later in net-next.

At least my patch clearly _shows_ the security requirement, instead of
relying on a side effect of a previous sock_wmalloc()

Again, it would be nice you understand the plan.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 01:55:01 PM Eric Dumazet wrote:
> On Mon, 2013-04-08 at 16:37 -0400, Paul Moore wrote:
> > The people who use this functionality almost never use upstream kernels,
> > they need to protection/certification/warm-fuzzies/etc. that come from a
> > distribution kernel and a support infrastructure.  I didn't catch it
> > because I use a slightly different configuration that didn't expose this
> > bug; while I would like to run a full regression test every release I
> > simply don't have the time to do that myself.
> > 
> > > This sounds like a very small issue to me, a revert is simply overkill.
> > 
> > It all depends on your use case.  To you, whom I assume doesn't use
> > SELinux, it is indeed a trivial issue.  To someone who relies on SELinux
> > for its network access controls this is a pretty significant issue.
> 
> Is the patch I sent addressing the problem or not ?
> 
> Note that I do have : CONFIG_SECURITY=y
> 
> So this patch basically adds the overhead back, and I'll have to use
> real hook later in net-next.

Please repost the patch to the LSM list, it needs to be discussed there.

> At least my patch clearly _shows_ the security requirement, instead of
> relying on a side effect of a previous sock_wmalloc()

I don't see it as a side effect, and as far as demonstration, I think the 
SELinux network access controls in their entirety shows the security 
requirement.  If we want to make the security requirements even more explicit 
in the networking stack, let's add a security blob to the sk_buff and allow 
some proper LSM hooks.

> Again, it would be nice you understand the plan.

I have no idea what the above sentence is trying to say.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Mon, 08 Apr 2013 17:09:32 -0400

> On Monday, April 08, 2013 01:55:01 PM Eric Dumazet wrote:
>> At least my patch clearly _shows_ the security requirement, instead of
>> relying on a side effect of a previous sock_wmalloc()
> 
> I don't see it as a side effect, and as far as demonstration, I think the 
> SELinux network access controls in their entirety shows the security 
> requirement.  If we want to make the security requirements even more explicit 
> in the networking stack, let's add a security blob to the sk_buff and allow 
> some proper LSM hooks.

You don't get it.

Without LSM there is no need to use sock_wmalloc() or to have a socket
context attacked to the SYN/ACK packet at all.

Therefore, the need should be explicit (Eric's approach), rather than
implicit (what you seem to be after).

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Mon, 2013-04-08 at 17:09 -0400, Paul Moore wrote:

> I have no idea what the above sentence is trying to say.

I guess the amount of mails exchanged today on this subject is kind of
astonishing. I am truly sorry I had more urgent issues to deal with
today.

If the patch is not officially sent by you today, I'll take care of it
after my day work.

Thank you.

[PATCH] selinux: add a skb_owned_by() hook

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Commit 90ba9b1986b5ac (tcp: tcp_make_synack() can use alloc_skb())
broke certain SELinux/NetLabel configurations by no longer correctly
assigning the sock to the outgoing SYNACK packet.

Cost of atomic operations on the LISTEN socket is quite big,
and we would like it to happen only if really needed.

This patch introduces a new security_ops->skb_owned_by() method,
that is a void operation unless selinux is active.

Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
Diagnosed-by: Paul Moore <pmoore@redhat.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-security-module@vger.kernel.org
---
 include/linux/security.h |    8 ++++++++
 net/ipv4/tcp_output.c    |    1 +
 security/capability.c    |    6 ++++++
 security/security.c      |    5 +++++
 security/selinux/hooks.c |    7 +++++++
 5 files changed, 27 insertions(+)

diff --git a/include/linux/security.h b/include/linux/security.h
index eee7478..6c3a78a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1638,6 +1638,7 @@ struct security_operations {
 	int (*tun_dev_attach_queue) (void *security);
 	int (*tun_dev_attach) (struct sock *sk, void *security);
 	int (*tun_dev_open) (void *security);
+	void (*skb_owned_by) (struct sk_buff *skb, struct sock *sk);
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
@@ -2588,6 +2589,8 @@ int security_tun_dev_attach_queue(void *security);
 int security_tun_dev_attach(struct sock *sk, void *security);
 int security_tun_dev_open(void *security);
 
+void security_skb_owned_by(struct sk_buff *skb, struct sock *sk);
+
 #else	/* CONFIG_SECURITY_NETWORK */
 static inline int security_unix_stream_connect(struct sock *sock,
 					       struct sock *other,
@@ -2779,6 +2782,11 @@ static inline int security_tun_dev_open(void *security)
 {
 	return 0;
 }
+
+static inline void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
+{
+}
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5d0b438..b44cf81 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2709,6 +2709,7 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 	skb_reserve(skb, MAX_TCP_HEADER);
 
 	skb_dst_set(skb, dst);
+	security_skb_owned_by(skb, sk);
 
 	mss = dst_metric_advmss(dst);
 	if (tp->rx_opt.user_mss && tp->rx_opt.user_mss < mss)
diff --git a/security/capability.c b/security/capability.c
index 5797750..c36cca6 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -737,6 +737,11 @@ static int cap_tun_dev_open(void *security)
 {
 	return 0;
 }
+
+static void cap_skb_owned_by(struct sk_buff *skb, struct sock *sk) 
+{
+}
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
@@ -1071,6 +1076,7 @@ void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, tun_dev_open);
 	set_to_cap_if_null(ops, tun_dev_attach_queue);
 	set_to_cap_if_null(ops, tun_dev_attach);
+	set_to_cap_if_null(ops, skb_owned_by);
 #endif	/* CONFIG_SECURITY_NETWORK */
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 	set_to_cap_if_null(ops, xfrm_policy_alloc_security);
diff --git a/security/security.c b/security/security.c
index 7b88c6a..03f248b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1290,6 +1290,11 @@ int security_tun_dev_open(void *security)
 }
 EXPORT_SYMBOL(security_tun_dev_open);
 
+void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
+{
+	security_ops->skb_owned_by(skb, sk);
+}
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2fa28c8..7171a95 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -51,6 +51,7 @@
 #include <linux/tty.h>
 #include <net/icmp.h>
 #include <net/ip.h>		/* for local_port_range[] */
+#include <net/sock.h>
 #include <net/tcp.h>		/* struct or_callable used in sock_rcv_skb */
 #include <net/net_namespace.h>
 #include <net/netlabel.h>
@@ -4363,6 +4364,11 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb)
 	selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid);
 }
 
+static void selinux_skb_owned_by(struct sk_buff *skb, struct sock *sk)
+{
+	skb_set_owner_w(skb, sk);
+}
+
 static int selinux_secmark_relabel_packet(u32 sid)
 {
 	const struct task_security_struct *__tsec;
@@ -5664,6 +5670,7 @@ static struct security_operations selinux_ops = {
 	.tun_dev_attach_queue =		selinux_tun_dev_attach_queue,
 	.tun_dev_attach =		selinux_tun_dev_attach,
 	.tun_dev_open =			selinux_tun_dev_open,
+	.skb_owned_by =			selinux_skb_owned_by,
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 	.xfrm_policy_alloc_security =	selinux_xfrm_policy_alloc,

Re: [PATCH] selinux: add a skb_owned_by() hook

[Casey Schaufler]

On 4/8/2013 8:58 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Commit 90ba9b1986b5ac (tcp: tcp_make_synack() can use alloc_skb())
> broke certain SELinux/NetLabel configurations by no longer correctly
> assigning the sock to the outgoing SYNACK packet.
>
> Cost of atomic operations on the LISTEN socket is quite big,
> and we would like it to happen only if really needed.
>
> This patch introduces a new security_ops->skb_owned_by() method,
> that is a void operation unless selinux is active.

I don't understand what this hook does.
Does it affect Smack (which uses NetLabel) as well?
How can I find out?

>
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Diagnosed-by: Paul Moore <pmoore@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-security-module@vger.kernel.org
> ---
>  include/linux/security.h |    8 ++++++++
>  net/ipv4/tcp_output.c    |    1 +
>  security/capability.c    |    6 ++++++
>  security/security.c      |    5 +++++
>  security/selinux/hooks.c |    7 +++++++
>  5 files changed, 27 insertions(+)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index eee7478..6c3a78a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1638,6 +1638,7 @@ struct security_operations {
>  	int (*tun_dev_attach_queue) (void *security);
>  	int (*tun_dev_attach) (struct sock *sk, void *security);
>  	int (*tun_dev_open) (void *security);
> +	void (*skb_owned_by) (struct sk_buff *skb, struct sock *sk);
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> @@ -2588,6 +2589,8 @@ int security_tun_dev_attach_queue(void *security);
>  int security_tun_dev_attach(struct sock *sk, void *security);
>  int security_tun_dev_open(void *security);
>  
> +void security_skb_owned_by(struct sk_buff *skb, struct sock *sk);
> +
>  #else	/* CONFIG_SECURITY_NETWORK */
>  static inline int security_unix_stream_connect(struct sock *sock,
>  					       struct sock *other,
> @@ -2779,6 +2782,11 @@ static inline int security_tun_dev_open(void *security)
>  {
>  	return 0;
>  }
> +
> +static inline void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
> +{
> +}
> +
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 5d0b438..b44cf81 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2709,6 +2709,7 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
>  	skb_reserve(skb, MAX_TCP_HEADER);
>  
>  	skb_dst_set(skb, dst);
> +	security_skb_owned_by(skb, sk);
>  
>  	mss = dst_metric_advmss(dst);
>  	if (tp->rx_opt.user_mss && tp->rx_opt.user_mss < mss)
> diff --git a/security/capability.c b/security/capability.c
> index 5797750..c36cca6 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -737,6 +737,11 @@ static int cap_tun_dev_open(void *security)
>  {
>  	return 0;
>  }
> +
> +static void cap_skb_owned_by(struct sk_buff *skb, struct sock *sk) 
> +{
> +}
> +
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> @@ -1071,6 +1076,7 @@ void __init security_fixup_ops(struct security_operations *ops)
>  	set_to_cap_if_null(ops, tun_dev_open);
>  	set_to_cap_if_null(ops, tun_dev_attach_queue);
>  	set_to_cap_if_null(ops, tun_dev_attach);
> +	set_to_cap_if_null(ops, skb_owned_by);
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
>  	set_to_cap_if_null(ops, xfrm_policy_alloc_security);
> diff --git a/security/security.c b/security/security.c
> index 7b88c6a..03f248b 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1290,6 +1290,11 @@ int security_tun_dev_open(void *security)
>  }
>  EXPORT_SYMBOL(security_tun_dev_open);
>  
> +void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
> +{
> +	security_ops->skb_owned_by(skb, sk);
> +}
> +
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 2fa28c8..7171a95 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -51,6 +51,7 @@
>  #include <linux/tty.h>
>  #include <net/icmp.h>
>  #include <net/ip.h>		/* for local_port_range[] */
> +#include <net/sock.h>
>  #include <net/tcp.h>		/* struct or_callable used in sock_rcv_skb */
>  #include <net/net_namespace.h>
>  #include <net/netlabel.h>
> @@ -4363,6 +4364,11 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb)
>  	selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid);
>  }
>  
> +static void selinux_skb_owned_by(struct sk_buff *skb, struct sock *sk)
> +{
> +	skb_set_owner_w(skb, sk);
> +}
> +
>  static int selinux_secmark_relabel_packet(u32 sid)
>  {
>  	const struct task_security_struct *__tsec;
> @@ -5664,6 +5670,7 @@ static struct security_operations selinux_ops = {
>  	.tun_dev_attach_queue =		selinux_tun_dev_attach_queue,
>  	.tun_dev_attach =		selinux_tun_dev_attach,
>  	.tun_dev_open =			selinux_tun_dev_open,
> +	.skb_owned_by =			selinux_skb_owned_by,
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
>  	.xfrm_policy_alloc_security =	selinux_xfrm_policy_alloc,
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] selinux: add a skb_owned_by() hook

[David Miller]

It makes sure SYN/ACKs have a socket context attached to the
packet, which only LSMs actually need.

You participated in the thread where this stuff was discussed and the
initial version of this patch was posted, so this patch, or any aspect
of it, should not be a mystery.

Re: [PATCH] selinux: add a skb_owned_by() hook

[Casey Schaufler]

On 4/8/2013 9:41 PM, David Miller wrote:
> It makes sure SYN/ACKs have a socket context attached to the
> packet, which only LSMs actually need.
>
> You participated in the thread where this stuff was discussed and the
> initial version of this patch was posted, so this patch, or any aspect
> of it, should not be a mystery.

I am flattered by your assumption that I can recall
on a moment's notice every thread I've been involved in since
1978. A reference to that thread would be mighty helpful.

The reality is that someone new to the thread is going to
have a bunch of trouble figuring out what you're talking
about. Every patch posting should have sufficient context
that someone who did not happen to see the background
threads will understand why it is being proposed.

I believe you when you say that I participated in the discussion.
I'll be dipped in caramel and rolled in nuts if I can find the
threads. Help me out by providing enough context that I can find
what I said, what you said, and if I still agree with myself.

Re: [PATCH] selinux: add a skb_owned_by() hook

[Paul Moore]

On Tuesday, April 09, 2013 12:41:44 AM David Miller wrote:
> It makes sure SYN/ACKs have a socket context attached to the
> packet, which only LSMs actually need.
> 
> You participated in the thread where this stuff was discussed and the
> initial version of this patch was posted, so this patch, or any aspect
> of it, should not be a mystery.

Casey, and the LSM list as a whole, was not included in the entire thread as 
when I first posted my original patch I believed the "mergeable" fix was going 
to be self contained within the network stack.

For Casey, and others on the LSM list, here is a pointer to the original 
thread which started on netdev.  Enjoy.

 * http://marc.info/?t=136543607500006&r=1&w=2

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] selinux: add a skb_owned_by() hook

[Eric Dumazet]

On Mon, 2013-04-08 at 21:29 -0700, Casey Schaufler wrote:

> I don't understand what this hook does.

It documents that security modules might need to get an sk pointer from
an skb, especially for TCP SYNACK messages.

> Does it affect Smack (which uses NetLabel) as well?
> How can I find out?

If you ask the question, thats is probably because Smack is not
affected.

selinux uses netfilter hooks, not Smack. selinux could probably refine
the need to set skb->sk based on CONFIG_NETFILTER, but I leave that
for a future change.

Just try the patch, and add your 'Tested-by', that will be fine.

If you believe Smack has an issue, tell us why, and we'll add the
follow-up patch.

Re: [PATCH] selinux: add a skb_owned_by() hook

[Paul Moore]

On Monday, April 08, 2013 09:29:35 PM Casey Schaufler wrote:
> Does it affect Smack (which uses NetLabel) as well?

The short answer is "no".  Smack approaches the network access controls 
differently and as a result has hooks in different places that do slightly 
different things; the regression in the network stack only affects SELinux.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] selinux: add a skb_owned_by() hook

[James Morris]

On Mon, 8 Apr 2013, Eric Dumazet wrote:

> From: Eric Dumazet <edumazet@google.com>
> 
> Commit 90ba9b1986b5ac (tcp: tcp_make_synack() can use alloc_skb())
> broke certain SELinux/NetLabel configurations by no longer correctly
> assigning the sock to the outgoing SYNACK packet.
> 
> Cost of atomic operations on the LISTEN socket is quite big,
> and we would like it to happen only if really needed.
> 
> This patch introduces a new security_ops->skb_owned_by() method,
> that is a void operation unless selinux is active.
> 
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Diagnosed-by: Paul Moore <pmoore@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-security-module@vger.kernel.org

Acked-by: James Morris <james.l.morris@oracle.com>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] selinux: add a skb_owned_by() hook

[Paul Moore]

On Monday, April 08, 2013 08:58:11 PM Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Commit 90ba9b1986b5ac (tcp: tcp_make_synack() can use alloc_skb())
> broke certain SELinux/NetLabel configurations by no longer correctly
> assigning the sock to the outgoing SYNACK packet.
> 
> Cost of atomic operations on the LISTEN socket is quite big,
> and we would like it to happen only if really needed.
> 
> This patch introduces a new security_ops->skb_owned_by() method,
> that is a void operation unless selinux is active.
> 
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Diagnosed-by: Paul Moore <pmoore@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-security-module@vger.kernel.org
> ---
>  include/linux/security.h |    8 ++++++++
>  net/ipv4/tcp_output.c    |    1 +
>  security/capability.c    |    6 ++++++
>  security/security.c      |    5 +++++
>  security/selinux/hooks.c |    7 +++++++
>  5 files changed, 27 insertions(+)

I've already voiced my objections to this approach, but I've just tested it 
and it does resolve the regression in the network stack.

Tested-by: Paul Moore <pmoore@redhat.com>
Acked-by: Paul Moore <pmoore@redhat.com>

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] selinux: add a skb_owned_by() hook

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 08 Apr 2013 20:58:11 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> Commit 90ba9b1986b5ac (tcp: tcp_make_synack() can use alloc_skb())
> broke certain SELinux/NetLabel configurations by no longer correctly
> assigning the sock to the outgoing SYNACK packet.
> 
> Cost of atomic operations on the LISTEN socket is quite big,
> and we would like it to happen only if really needed.
> 
> This patch introduces a new security_ops->skb_owned_by() method,
> that is a void operation unless selinux is active.
> 
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Diagnosed-by: Paul Moore <pmoore@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Since this fixes a regression that got added by the networking
tree, I'll push this to Linus, applied, thanks.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 02:12:01 PM Paul Moore wrote:
> On Monday, April 08, 2013 10:47:47 AM Eric Dumazet wrote:
> > On Mon, 2013-04-08 at 13:40 -0400, Paul Moore wrote:
> > > Sort of a similar problem, but not really the same.  Also, arguably,
> > > there is no real associated sock/socket for a RST so orphaning the
> > > packet makes sense. In the case of a SYNACK we can, and should,
> > > associate the packet with a sock/socket.
> > 
> > What is the intent ?
> 
> We have to do a number of painful things in SELinux because we aren't
> allowed a proper security blob (void *security) in a sk_buff.  One of those
> things ...

Actually, I wonder if this problem means it is a good time to revisit the no-
security-blob-in-sk_buff decision?  The management of the blob would be hidden 
behind the LSM hooks like everything else and it would have a number of 
advantages including making problems like we are seeing here easier to fix or 
avoid entirely.  It would also make life much easier for those of working on 
LSM stuff and it would pave the way for including network access controls in 
the stacked-LSM stuff Casey is kicking around.

I'm aware of all the arguments against, but thought it would be worth bringing 
it up again, if for no other reason than I haven't heard enough shouting yet 
today :)

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 02:32:00 PM Paul Moore wrote:
> On Monday, April 08, 2013 02:12:01 PM Paul Moore wrote:
> > On Monday, April 08, 2013 10:47:47 AM Eric Dumazet wrote:
> > > On Mon, 2013-04-08 at 13:40 -0400, Paul Moore wrote:
> > > > Sort of a similar problem, but not really the same.  Also, arguably,
> > > > there is no real associated sock/socket for a RST so orphaning the
> > > > packet makes sense. In the case of a SYNACK we can, and should,
> > > > associate the packet with a sock/socket.
> > > 
> > > What is the intent ?
> > 
> > We have to do a number of painful things in SELinux because we aren't
> > allowed a proper security blob (void *security) in a sk_buff.  One of
> > those things ...
> 
> Actually, I wonder if this problem means it is a good time to revisit the
> no- security-blob-in-sk_buff decision?  The management of the blob would be
> hidden behind the LSM hooks like everything else and it would have a number
> of advantages including making problems like we are seeing here easier to
> fix or avoid entirely.  It would also make life much easier for those of
> working on LSM stuff and it would pave the way for including network access
> controls in the stacked-LSM stuff Casey is kicking around.

No comment, or am I just too anxious?

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Mon, 08 Apr 2013 17:10:43 -0400

> On Monday, April 08, 2013 02:32:00 PM Paul Moore wrote:
>> On Monday, April 08, 2013 02:12:01 PM Paul Moore wrote:
>> > On Monday, April 08, 2013 10:47:47 AM Eric Dumazet wrote:
>> > > On Mon, 2013-04-08 at 13:40 -0400, Paul Moore wrote:
>> > > > Sort of a similar problem, but not really the same.  Also, arguably,
>> > > > there is no real associated sock/socket for a RST so orphaning the
>> > > > packet makes sense. In the case of a SYNACK we can, and should,
>> > > > associate the packet with a sock/socket.
>> > > 
>> > > What is the intent ?
>> > 
>> > We have to do a number of painful things in SELinux because we aren't
>> > allowed a proper security blob (void *security) in a sk_buff.  One of
>> > those things ...
>> 
>> Actually, I wonder if this problem means it is a good time to revisit the
>> no- security-blob-in-sk_buff decision?  The management of the blob would be
>> hidden behind the LSM hooks like everything else and it would have a number
>> of advantages including making problems like we are seeing here easier to
>> fix or avoid entirely.  It would also make life much easier for those of
>> working on LSM stuff and it would pave the way for including network access
>> controls in the stacked-LSM stuff Casey is kicking around.
> 
> No comment, or am I just too anxious?

There is no way I'm putting LSM overhead into sk_buff, it's already
too big.

I didn't comment because it wasn't worth a comment, but since you're
pushing me on the issue, I'll make the no explicit.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 05:15:12 PM David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Mon, 08 Apr 2013 17:10:43 -0400
> 
> > On Monday, April 08, 2013 02:32:00 PM Paul Moore wrote:
> >> On Monday, April 08, 2013 02:12:01 PM Paul Moore wrote:
> >> > On Monday, April 08, 2013 10:47:47 AM Eric Dumazet wrote:
> >> > > On Mon, 2013-04-08 at 13:40 -0400, Paul Moore wrote:
> >> > > > Sort of a similar problem, but not really the same.  Also,
> >> > > > arguably,
> >> > > > there is no real associated sock/socket for a RST so orphaning the
> >> > > > packet makes sense. In the case of a SYNACK we can, and should,
> >> > > > associate the packet with a sock/socket.
> >> > > 
> >> > > What is the intent ?
> >> > 
> >> > We have to do a number of painful things in SELinux because we aren't
> >> > allowed a proper security blob (void *security) in a sk_buff.  One of
> >> > those things ...
> >> 
> >> Actually, I wonder if this problem means it is a good time to revisit the
> >> no- security-blob-in-sk_buff decision?  The management of the blob would
> >> be hidden behind the LSM hooks like everything else and it would have a
> >> number of advantages including making problems like we are seeing here
> >> easier to fix or avoid entirely.  It would also make life much easier for
> >> those of working on LSM stuff and it would pave the way for including
> >> network access controls in the stacked-LSM stuff Casey is kicking around.
> > 
> > No comment, or am I just too anxious?
> 
> There is no way I'm putting LSM overhead into sk_buff, it's already
> too big.

If the void pointer is wrapped by a #ifdef (plenty of precedence for that) and 
the management of that pointer is handled by LSM hooks why is it a concern?  I 
apologize for pushing on the issue, but I'm having a hard time reconciling the 
reason for the "no" with the comments/decisions about the regression fix; at 
present there seems to be a level of contradiction between the two.

> I didn't comment because it wasn't worth a comment, but since you're
> pushing me on the issue, I'll make the no explicit.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Mon, 08 Apr 2013 17:24:50 -0400

> If the void pointer is wrapped by a #ifdef (plenty of precedence for that) and 
> the management of that pointer is handled by LSM hooks why is it a concern?  I 
> apologize for pushing on the issue, but I'm having a hard time reconciling the 
> reason for the "no" with the comments/decisions about the regression fix; at 
> present there seems to be a level of contradiction between the two.

8 bytes times however many millions of packets per second we can process
on a big machine, you do the math.

It's memory, less cache locality, etc. etc. etc.

It's the most important data structure in the entire networking stack,
and every single byte matters.

I want the overhead to be your problem, so that only users of your
stuff eat the overhead, rather than everyone.

And don't even mention ifdefs, that's bogus, because every
distribution turns every option on, %99.9999999 of users will
therefore not see the savings.

Really, this is a dead topic, let's move on.

Thanks.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 05:33:25 PM David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Mon, 08 Apr 2013 17:24:50 -0400
> 
> > If the void pointer is wrapped by a #ifdef (plenty of precedence for that)
> > and the management of that pointer is handled by LSM hooks why is it a
> > concern?  I apologize for pushing on the issue, but I'm having a hard
> > time reconciling the reason for the "no" with the comments/decisions
> > about the regression fix; at present there seems to be a level of
> > contradiction between the two.
>
> 8 bytes times however many millions of packets per second we can process
> on a big machine, you do the math.
> 
> It's memory, less cache locality, etc. etc. etc.
> 
> It's the most important data structure in the entire networking stack,
> and every single byte matters.
> 
> I want the overhead to be your problem, so that only users of your
> stuff eat the overhead, rather than everyone.

Okay, if the objection is really just one of structure size and not the hooks, 
what if I did the work to consolidate the skb->secmark and skb->sp fields into 
a new structure/pointer?  Assuming it wasn't too painful, it would be a net 
reduction of four bytes.  If that worked would you have an objection to us 
adding a LSM security blob to this new structure?

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Mon, 08 Apr 2013 18:01:56 -0400

> Okay, if the objection is really just one of structure size and not the hooks, 
> what if I did the work to consolidate the skb->secmark and skb->sp fields into 
> a new structure/pointer?  Assuming it wasn't too painful, it would be a net 
> reduction of four bytes.  If that worked would you have an objection to us 
> adding a LSM security blob to this new structure?

'sp' is sepreate from LSM and making it indirect would hurt IPSEC
performance.

Please, really, just drop this.

Thanks.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Casey Schaufler]

On 4/8/2013 2:33 PM, David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Mon, 08 Apr 2013 17:24:50 -0400
>
>> If the void pointer is wrapped by a #ifdef (plenty of precedence for that) and 
>> the management of that pointer is handled by LSM hooks why is it a concern?  I 
>> apologize for pushing on the issue, but I'm having a hard time reconciling the 
>> reason for the "no" with the comments/decisions about the regression fix; at 
>> present there seems to be a level of contradiction between the two.
> 8 bytes times however many millions of packets per second we can process
> on a big machine, you do the math.

OK, let's do the math.

First off, it's 4 bytes, not 8. It replaces the secmark.
Your increased memory usage is going to be

	4 bytes/packet *  M packets/second * N seconds

Where M is the rate at which you're processing packets and
N is the length of time it takes to process a packet.

Let's pretend we have an embedded system that does nothing but send
128 byte packets on a 10Gb port. That's 10M packets/second. If it
takes a full second to process a packet the overhead is 40MB for that
second. I have it on good authority that packets can be processed
in considerably less time than that. The real number is more like
0.05 seconds. That means your actual overhead is more like 1MB.

These are dumbed down calculations. I am not a memory usage expert.
I am convinced that "real" calculations are going to get similar
numbers. I am, of course, willing to be swayed by evidence that I
am wrong.

Compare that to the overhead associated with using CIPSO on packets
that never leave the box.



>
> It's memory, less cache locality, etc. etc. etc.
>
> It's the most important data structure in the entire networking stack,
> and every single byte matters.
>
> I want the overhead to be your problem, so that only users of your
> stuff eat the overhead, rather than everyone.
>
> And don't even mention ifdefs, that's bogus, because every
> distribution turns every option on, %99.9999999 of users will
> therefore not see the savings.
>
> Really, this is a dead topic, let's move on.
>
> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Mon, 2013-04-08 at 16:40 -0700, Casey Schaufler wrote:

> OK, let's do the math.
> 
> First off, it's 4 bytes, not 8. It replaces the secmark.
> Your increased memory usage is going to be
> 
> 	4 bytes/packet *  M packets/second * N seconds
> 
> Where M is the rate at which you're processing packets and
> N is the length of time it takes to process a packet.
> 
> Let's pretend we have an embedded system that does nothing but send
> 128 byte packets on a 10Gb port. That's 10M packets/second. If it
> takes a full second to process a packet the overhead is 40MB for that
> second. I have it on good authority that packets can be processed
> in considerably less time than that. The real number is more like
> 0.05 seconds. That means your actual overhead is more like 1MB.
> 
> These are dumbed down calculations. I am not a memory usage expert.
> I am convinced that "real" calculations are going to get similar
> numbers. I am, of course, willing to be swayed by evidence that I
> am wrong.
> 
> Compare that to the overhead associated with using CIPSO on packets
> that never leave the box.

Maths are not that simple, and its not about size of sk_buff, since the
number of in-flight skb should be quite small.

Its the time to init this memory for _every_ packet.

sizeof(sk_buff) is 0xf8, very close to cross the 256 bytes limit.

Add a single _byte_ and it becomes a matter of adding a _cache_ line,
and thats 25 % cost, assuming 64bytes cache lines.

So instead of processing 10M packets per second, we would process 9M
packets per second, or maybe less.

Yes, 256 bytes per sk_buff, this is the current insane situation.
(Not counting the struct skb_shared_info, adding at least one additional
cache line)

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Casey Schaufler]

On 4/8/2013 5:33 PM, Eric Dumazet wrote:
> On Mon, 2013-04-08 at 16:40 -0700, Casey Schaufler wrote:
>
>> OK, let's do the math.
>>
>> First off, it's 4 bytes, not 8. It replaces the secmark.
>> Your increased memory usage is going to be
>>
>> 	4 bytes/packet *  M packets/second * N seconds
>>
>> Where M is the rate at which you're processing packets and
>> N is the length of time it takes to process a packet.
>>
>> Let's pretend we have an embedded system that does nothing but send
>> 128 byte packets on a 10Gb port. That's 10M packets/second. If it
>> takes a full second to process a packet the overhead is 40MB for that
>> second. I have it on good authority that packets can be processed
>> in considerably less time than that. The real number is more like
>> 0.05 seconds. That means your actual overhead is more like 1MB.
>>
>> These are dumbed down calculations. I am not a memory usage expert.
>> I am convinced that "real" calculations are going to get similar
>> numbers. I am, of course, willing to be swayed by evidence that I
>> am wrong.
>>
>> Compare that to the overhead associated with using CIPSO on packets
>> that never leave the box.
> Maths are not that simple,

I am willing to accept that. I am willing to be
educated. I would be interested to see what the
"real" maths are, and how far off my dumb version
might actually be.

> and its not about size of sk_buff, since the
> number of in-flight skb should be quite small.

The reason we're told that there can't be a blob
pointer in the sk_buff is that it increases the size
of the sk_buff. Yes, it *is* about the size.

> Its the time to init this memory for _every_ packet.

Which is a function of the size, no?

> sizeof(sk_buff) is 0xf8, very close to cross the 256 bytes limit.

0xf8 + 0x4 = 0xfc
248 + 4 = 252

> Add a single _byte_ and it becomes a matter of adding a _cache_ line,
> and thats 25 % cost, assuming 64bytes cache lines.

I don't see that with adding 4 bytes. Again, I'm willing to be
educated if I'm wrong.

> So instead of processing 10M packets per second, we would process 9M
> packets per second, or maybe less.
>
> Yes, 256 bytes per sk_buff, this is the current insane situation.
> (Not counting the struct skb_shared_info, adding at least one additional
> cache line)

Sorry, but I don't see what's insane with either the
current 248 byte sk_buff or a 252 byte sk_buff.

As always, I am willing to be educated.

Thank you.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Mon, 2013-04-08 at 17:59 -0700, Casey Schaufler wrote:

> I don't see that with adding 4 bytes. Again, I'm willing to be
> educated if I'm wrong.

Feel free to add 4 bytes without having the 'align to 8 bytes' problem
on 64 bit arches. Show us your patch.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Casey Schaufler]

On 4/8/2013 6:09 PM, Eric Dumazet wrote:
> On Mon, 2013-04-08 at 17:59 -0700, Casey Schaufler wrote:
>
>> I don't see that with adding 4 bytes. Again, I'm willing to be
>> educated if I'm wrong.
> Feel free to add 4 bytes without having the 'align to 8 bytes' problem
> on 64 bit arches. Show us your patch.

Recall that it's replacing an existing 4 byte value with an 8 byte value.
My compiler days were quite short and long ago, but it would seem that
an 8 byte value ought not have an 'align to 8 bytes' problem.

Again, I'm willing to be educated.

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

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 06:24:59 PM Casey Schaufler wrote:
> On 4/8/2013 6:09 PM, Eric Dumazet wrote:
> > On Mon, 2013-04-08 at 17:59 -0700, Casey Schaufler wrote:
> >> I don't see that with adding 4 bytes. Again, I'm willing to be
> >> educated if I'm wrong.
> > 
> > Feel free to add 4 bytes without having the 'align to 8 bytes' problem
> > on 64 bit arches. Show us your patch.
> 
> Recall that it's replacing an existing 4 byte value with an 8 byte value.
> My compiler days were quite short and long ago, but it would seem that
> an 8 byte value ought not have an 'align to 8 bytes' problem.
> 
> Again, I'm willing to be educated.

Armed with a cup of coffee I took a look at the sk_buff structure this morning 
with the pahole tool and using the current sk_buff if we turn on all the 
#ifdefs here is what I see on x86_64:

struct sk_buff {
        struct sk_buff *           next;                 /*     0     8 */
        struct sk_buff *           prev;                 /*     8     8 */
        ktime_t                    tstamp;               /*    16     8 */
        struct sock *              sk;                   /*    24     8 */
        struct net_device *        dev;                  /*    32     8 */
        char                       cb[48];               /*    40    48 */
        /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
        long unsigned int          _skb_refdst;          /*    88     8 */
        struct sec_path *          sp;                   /*    96     8 */
        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 */
        int                        flags1_begin[0];      /*   124     0 */
        __u8                       local_df:1;           /*   124: 7  1 */
        __u8                       cloned:1;             /*   124: 6  1 */
        __u8                       ip_summed:2;          /*   124: 4  1 */
        __u8                       nohdr:1;              /*   124: 3  1 */
        __u8                       nfctinfo:3;           /*   124: 0  1 */
        __u8                       pkt_type:3;           /*   125: 5  1 */
        __u8                       fclone:2;             /*   125: 3  1 */
        __u8                       ipvs_property:1;      /*   125: 2  1 */
        __u8                       peeked:1;             /*   125: 1  1 */
        __u8                       nf_trace:1;           /*   125: 0  1 */

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

        /* --- cacheline 2 boundary (128 bytes) --- */
        int                        flags1_end[0];        /*   128     0 */
        __be16                     protocol;             /*   128     2 */

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

        void                       (*destructor)(struct sk_buff *); /*   136     
8 */
        struct nf_conntrack *      nfct;                 /*   144     8 */
        struct sk_buff *           nfct_reasm;           /*   152     8 */
        struct nf_bridge_info *    nf_bridge;            /*   160     8 */
        int                        skb_iif;              /*   168     4 */
        __u32                      rxhash;               /*   172     4 */
        __u16                      vlan_tci;             /*   176     2 */
        __u16                      tc_index;             /*   178     2 */
        __u16                      tc_verd;              /*   180     2 */
        __u16                      queue_mapping;        /*   182     2 */
        int                        flags2_begin[0];      /*   184     0 */
        __u8                       ndisc_nodetype:2;     /*   184: 6  1 */
        __u8                       pfmemalloc:1;         /*   184: 5  1 */
        __u8                       ooo_okay:1;           /*   184: 4  1 */
        __u8                       l4_rxhash:1;          /*   184: 3  1 */
        __u8                       wifi_acked_valid:1;   /*   184: 2  1 */
        __u8                       wifi_acked:1;         /*   184: 1  1 */
        __u8                       no_fcs:1;             /*   184: 0  1 */
        __u8                       head_frag:1;          /*   185: 7  1 */
        __u8                       encapsulation:1;      /*   185: 6  1 */

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

        int                        flags2_end[0];        /*   188     0 */
        dma_cookie_t               dma_cookie;           /*   188     4 */
        /* --- cacheline 3 boundary (192 bytes) --- */
        __u32                      secmark;              /*   192     4 */
        union {
                __u32              mark;                 /*           4 */
                __u32              dropcount;            /*           4 */
                __u32              reserved_tailroom;    /*           4 */
        };                                               /*   196     4 */
        sk_buff_data_t             inner_transport_header; /*   200     8 */
        sk_buff_data_t             inner_network_header; /*   208     8 */
        sk_buff_data_t             transport_header;     /*   216     8 */
        sk_buff_data_t             network_header;       /*   224     8 */
        sk_buff_data_t             mac_header;           /*   232     8 */
        sk_buff_data_t             tail;                 /*   240     8 */
        sk_buff_data_t             end;                  /*   248     8 */
        /* --- cacheline 4 boundary (256 bytes) --- */
        unsigned char *            head;                 /*   256     8 */
        unsigned char *            data;                 /*   264     8 */
        unsigned int               truesize;             /*   272     4 */
        atomic_t                   users;                /*   276     4 */

        /* size: 280, cachelines: 5, members: 62 */
        /* sum members: 270, holes: 3, sum holes: 10 */
        /* bit holes: 1, sum bit holes: 6 bits */
        /* last cacheline: 24 bytes */
};

It looks like there some holes we might be able to capitalize on.  If we 
remove "secmark" (we can handle it inside a security blob) and move "protocol" 
to after the flags2 bit field we can make an aligned 8 byte hold for a 
security blob before "destructor".  According to pahole the structure size 
stays the same and the only field which moves to a different cacheline is 
"dma_cookie" which moves from cacheline 2 to 3.  Here is the pahole output:

struct sk_buff_test {
        struct sk_buff *           next;                 /*     0     8 */
        struct sk_buff *           prev;                 /*     8     8 */
        ktime_t                    tstamp;               /*    16     8 */
        struct sock *              sk;                   /*    24     8 */
        struct net_device *        dev;                  /*    32     8 */
        char                       cb[48];               /*    40    48 */
        /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
        long unsigned int          _skb_refdst;          /*    88     8 */
        struct sec_path *          sp;                   /*    96     8 */
        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 */
        int                        flags1_begin[0];      /*   124     0 */
        __u8                       local_df:1;           /*   124: 7  1 */
        __u8                       cloned:1;             /*   124: 6  1 */
        __u8                       ip_summed:2;          /*   124: 4  1 */
        __u8                       nohdr:1;              /*   124: 3  1 */
        __u8                       nfctinfo:3;           /*   124: 0  1 */
        __u8                       pkt_type:3;           /*   125: 5  1 */
        __u8                       fclone:2;             /*   125: 3  1 */
        __u8                       ipvs_property:1;      /*   125: 2  1 */
        __u8                       peeked:1;             /*   125: 1  1 */
        __u8                       nf_trace:1;           /*   125: 0  1 */

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

        /* --- cacheline 2 boundary (128 bytes) --- */
        int                        flags1_end[0];        /*   128     0 */
        void *                     security;             /*   128     8 */
        void                       (*destructor)(struct sk_buff *); /*   136     
8 */
        struct nf_conntrack *      nfct;                 /*   144     8 */
        struct sk_buff *           nfct_reasm;           /*   152     8 */
        struct nf_bridge_info *    nf_bridge;            /*   160     8 */
        int                        skb_iif;              /*   168     4 */
        __u32                      rxhash;               /*   172     4 */
        __u16                      vlan_tci;             /*   176     2 */
        __u16                      tc_index;             /*   178     2 */
        __u16                      tc_verd;              /*   180     2 */
        __u16                      queue_mapping;        /*   182     2 */
        int                        flags2_begin[0];      /*   184     0 */
        __u8                       ndisc_nodetype:2;     /*   184: 6  1 */
        __u8                       pfmemalloc:1;         /*   184: 5  1 */
        __u8                       ooo_okay:1;           /*   184: 4  1 */
        __u8                       l4_rxhash:1;          /*   184: 3  1 */
        __u8                       wifi_acked_valid:1;   /*   184: 2  1 */
        __u8                       wifi_acked:1;         /*   184: 1  1 */
        __u8                       no_fcs:1;             /*   184: 0  1 */
        __u8                       head_frag:1;          /*   185: 7  1 */
        __u8                       encapsulation:1;      /*   185: 6  1 */

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

        int                        flags2_end[0];        /*   188     0 */
        __be16                     protocol;             /*   188     2 */

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

        /* --- cacheline 3 boundary (192 bytes) --- */
        dma_cookie_t               dma_cookie;           /*   192     4 */
        union {
                __u32              mark;                 /*           4 */
                __u32              dropcount;            /*           4 */
                __u32              reserved_tailroom;    /*           4 */
        };                                               /*   196     4 */
        sk_buff_data_t             inner_transport_header; /*   200     8 */
        sk_buff_data_t             inner_network_header; /*   208     8 */
        sk_buff_data_t             transport_header;     /*   216     8 */
        sk_buff_data_t             network_header;       /*   224     8 */
        sk_buff_data_t             mac_header;           /*   232     8 */
        sk_buff_data_t             tail;                 /*   240     8 */
        sk_buff_data_t             end;                  /*   248     8 */
        /* --- cacheline 4 boundary (256 bytes) --- */
        unsigned char *            head;                 /*   256     8 */
        unsigned char *            data;                 /*   264     8 */
        unsigned int               truesize;             /*   272     4 */
        atomic_t                   users;                /*   276     4 */

        /* size: 280, cachelines: 5, members: 62 */
        /* sum members: 274, holes: 3, sum holes: 6 */
        /* bit holes: 1, sum bit holes: 6 bits */
        /* last cacheline: 24 bytes */
};

As Casey already mentioned, if this isn't acceptable please help me understand 
why.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Tuesday, April 09, 2013 09:19:30 AM Paul Moore wrote:
> On Monday, April 08, 2013 06:24:59 PM Casey Schaufler wrote:
> > On 4/8/2013 6:09 PM, Eric Dumazet wrote:
> > > On Mon, 2013-04-08 at 17:59 -0700, Casey Schaufler wrote:
> > >> I don't see that with adding 4 bytes. Again, I'm willing to be
> > >> educated if I'm wrong.
> > > 
> > > Feel free to add 4 bytes without having the 'align to 8 bytes' problem
> > > on 64 bit arches. Show us your patch.
> > 
> > Recall that it's replacing an existing 4 byte value with an 8 byte value.
> > My compiler days were quite short and long ago, but it would seem that
> > an 8 byte value ought not have an 'align to 8 bytes' problem.
> > 
> > Again, I'm willing to be educated.
> 
> Armed with a cup of coffee I took a look at the sk_buff structure this
> morning with the pahole tool and using the current sk_buff if we turn on
> all the #ifdefs here is what I see on x86_64:
> 
> struct sk_buff {

...

>         /* size: 280, cachelines: 5, members: 62 */
>         /* sum members: 270, holes: 3, sum holes: 10 */
>         /* bit holes: 1, sum bit holes: 6 bits */
>         /* last cacheline: 24 bytes */
> };
> 
> It looks like there some holes we might be able to capitalize on.  If we
> remove "secmark" (we can handle it inside a security blob) and move
> "protocol" to after the flags2 bit field we can make an aligned 8 byte hold
> for a security blob before "destructor".  According to pahole the structure
> size stays the same and the only field which moves to a different cacheline
> is "dma_cookie" which moves from cacheline 2 to 3.  Here is the pahole
> output:
> 
> struct sk_buff_test {

...

>         /* size: 280, cachelines: 5, members: 62 */
>         /* sum members: 274, holes: 3, sum holes: 6 */
>         /* bit holes: 1, sum bit holes: 6 bits */
>         /* last cacheline: 24 bytes */
> };
> 
> As Casey already mentioned, if this isn't acceptable please help me
> understand why.

For the sake of completeness I also checked out the changes when compiled for 
32 bit and it was very much the same; same structure size and in the 32 bit 
case no field movement from one cacheline to another.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Tue, 2013-04-09 at 09:19 -0400, Paul Moore wrote:

> As Casey already mentioned, if this isn't acceptable please help me understand 
> why.
> 

You see something which is not the reality. If you do such analysis,
better do it properly, because any change you are going to submit will
be doubly checked by people who really care.

sizeof(sk_buff) is not 280. (aligned to 320 because of cache line being
64)

Tell me what you have when doing :

ls -l /sys/kernel/slab/skbuff_head_cache

Do you really see 

$ ls -l /sys/kernel/slab/skbuff_head_cache
lrwxrwxrwx 1 root root 0 Apr  9 06:54 /sys/kernel/slab/skbuff_head_cache -> :t-0000320

Here I get :

$ ls -l /sys/kernel/slab/skbuff_head_cache
lrwxrwxrwx 1 root root 0 Apr  9 06:54 /sys/kernel/slab/skbuff_head_cache -> :t-0000256

because sizeof(sk_buff) <= 256

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Tuesday, April 09, 2013 07:00:22 AM Eric Dumazet wrote:
> On Tue, 2013-04-09 at 09:19 -0400, Paul Moore wrote:
> > As Casey already mentioned, if this isn't acceptable please help me
> > understand why.
> 
> You see something which is not the reality. If you do such analysis,
> better do it properly, because any change you are going to submit will
> be doubly checked by people who really care.

I am attempting to do it properly, I simply made a mistake.  Ben also pointed 
it out.  As you wrote yesterday, "Lets go forward".

After fixing the BITS_PER_LONG problem I looked at it again and it appears 
that by simply replacing the "secmark" field with a blob we retain the size of 
the sk_buff as well as the cacheline positions of all the fields, e.g. 
dma_cookie no longer moves cachelines.  Thoughts?

struct sk_buff_test {
        struct sk_buff *           next;                 /*     0     8 */
        struct sk_buff *           prev;                 /*     8     8 */
        ktime_t                    tstamp;               /*    16     8 */
        struct sock *              sk;                   /*    24     8 */
        struct net_device *        dev;                  /*    32     8 */
        char                       cb[48];               /*    40    48 */
        /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
        long unsigned int          _skb_refdst;          /*    88     8 */
        struct sec_path *          sp;                   /*    96     8 */
        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 */
        int                        flags1_begin[0];      /*   124     0 */
        __u8                       local_df:1;           /*   124: 7  1 */
        __u8                       cloned:1;             /*   124: 6  1 */
        __u8                       ip_summed:2;          /*   124: 4  1 */
        __u8                       nohdr:1;              /*   124: 3  1 */
        __u8                       nfctinfo:3;           /*   124: 0  1 */
        __u8                       pkt_type:3;           /*   125: 5  1 */
        __u8                       fclone:2;             /*   125: 3  1 */
        __u8                       ipvs_property:1;      /*   125: 2  1 */
        __u8                       peeked:1;             /*   125: 1  1 */
        __u8                       nf_trace:1;           /*   125: 0  1 */

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

        /* --- cacheline 2 boundary (128 bytes) --- */
        int                        flags1_end[0];        /*   128     0 */
        __be16                     protocol;             /*   128     2 */

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

        void                       (*destructor)(struct sk_buff *); /*   136     
8 */
        struct nf_conntrack *      nfct;                 /*   144     8 */
        struct sk_buff *           nfct_reasm;           /*   152     8 */
        struct nf_bridge_info *    nf_bridge;            /*   160     8 */
        int                        skb_iif;              /*   168     4 */
        __u32                      rxhash;               /*   172     4 */
        __u16                      vlan_tci;             /*   176     2 */
        __u16                      tc_index;             /*   178     2 */
        __u16                      tc_verd;              /*   180     2 */
        __u16                      queue_mapping;        /*   182     2 */
        int                        flags2_begin[0];      /*   184     0 */
        __u8                       ndisc_nodetype:2;     /*   184: 6  1 */
        __u8                       pfmemalloc:1;         /*   184: 5  1 */
        __u8                       ooo_okay:1;           /*   184: 4  1 */
        __u8                       l4_rxhash:1;          /*   184: 3  1 */
        __u8                       wifi_acked_valid:1;   /*   184: 2  1 */
        __u8                       wifi_acked:1;         /*   184: 1  1 */
        __u8                       no_fcs:1;             /*   184: 0  1 */
        __u8                       head_frag:1;          /*   185: 7  1 */
        __u8                       encapsulation:1;      /*   185: 6  1 */

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

        int                        flags2_end[0];        /*   188     0 */
        dma_cookie_t               dma_cookie;           /*   188     4 */
        /* --- cacheline 3 boundary (192 bytes) --- */
        void *                     security;             /*   192     8 */
        union {
                __u32              mark;                 /*           4 */
                __u32              dropcount;            /*           4 */
                __u32              reserved_tailroom;    /*           4 */
        };                                               /*   200     4 */
        sk_buff_data_t             inner_transport_header; /*   204     4 */
        sk_buff_data_t             inner_network_header; /*   208     4 */
        sk_buff_data_t             transport_header;     /*   212     4 */
        sk_buff_data_t             network_header;       /*   216     4 */
        sk_buff_data_t             mac_header;           /*   220     4 */
        sk_buff_data_t             tail;                 /*   224     4 */
        sk_buff_data_t             end;                  /*   228     4 */
        unsigned char *            head;                 /*   232     8 */
        unsigned char *            data;                 /*   240     8 */
        unsigned int               truesize;             /*   248     4 */
        atomic_t                   users;                /*   252     4 */
        /* --- cacheline 4 boundary (256 bytes) --- */

        /* size: 256, cachelines: 4, members: 62 */
        /* sum members: 246, holes: 3, sum holes: 10 */
        /* bit holes: 1, sum bit holes: 6 bits */
};

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Tue, 2013-04-09 at 10:19 -0400, Paul Moore wrote:
> On Tuesday, April 09, 2013 07:00:22 AM Eric Dumazet wrote:
> > On Tue, 2013-04-09 at 09:19 -0400, Paul Moore wrote:
> > > As Casey already mentioned, if this isn't acceptable please help me
> > > understand why.
> > 
> > You see something which is not the reality. If you do such analysis,
> > better do it properly, because any change you are going to submit will
> > be doubly checked by people who really care.
> 
> I am attempting to do it properly, I simply made a mistake.  Ben also pointed 
> it out.  As you wrote yesterday, "Lets go forward".
> 
> After fixing the BITS_PER_LONG problem I looked at it again and it appears 
> that by simply replacing the "secmark" field with a blob we retain the size of 
> the sk_buff as well as the cacheline positions of all the fields, e.g. 
> dma_cookie no longer moves cachelines.  Thoughts?

If you take a look at recent history of changes on sk_buff, you can see
we added very recently fields for encapsulation support. These were
absolutely wanted for modern operations at datacenter level.

This effort might still need new room, so I prefer not filling sk_buff
right now.

Take a look at the cloned sk_buff. We need an extra atomic_t at the end,
so if make sk_buff bigger than 0xf8 bytes,  fclone_cache will use an
extra cache line as well. Not a big deal, but RPC workloads like netperf
-t TCP_RR will probably show a regression.

ls -l /sys/kernel/slab/skbuff_fclone_cache

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Tuesday, April 09, 2013 07:31:04 AM Eric Dumazet wrote:
> On Tue, 2013-04-09 at 10:19 -0400, Paul Moore wrote:
> > On Tuesday, April 09, 2013 07:00:22 AM Eric Dumazet wrote:
> > > On Tue, 2013-04-09 at 09:19 -0400, Paul Moore wrote:
> > > > As Casey already mentioned, if this isn't acceptable please help me
> > > > understand why.
> > > 
> > > You see something which is not the reality. If you do such analysis,
> > > better do it properly, because any change you are going to submit will
> > > be doubly checked by people who really care.
> > 
> > I am attempting to do it properly, I simply made a mistake.  Ben also
> > pointed it out.  As you wrote yesterday, "Lets go forward".
> > 
> > After fixing the BITS_PER_LONG problem I looked at it again and it appears
> > that by simply replacing the "secmark" field with a blob we retain the
> > size of the sk_buff as well as the cacheline positions of all the fields,
> > e.g. dma_cookie no longer moves cachelines.  Thoughts?
> 
> If you take a look at recent history of changes on sk_buff, you can see
> we added very recently fields for encapsulation support. These were
> absolutely wanted for modern operations at datacenter level.
> 
> This effort might still need new room, so I prefer not filling sk_buff
> right now.

Has anyone proposed any additional encapsulation patches which need additional 
fields in the sk_buff?  Are you aware of any additional encapsulation patches 
which are in progress?  When would you consider it "safe"?

> Take a look at the cloned sk_buff. We need an extra atomic_t at the end,
> so if make sk_buff bigger than 0xf8 bytes,  fclone_cache will use an
> extra cache line as well. Not a big deal, but RPC workloads like netperf
> -t TCP_RR will probably show a regression.
> 
> ls -l /sys/kernel/slab/skbuff_fclone_cache

Perhaps I'm misunderstanding, but these comments above only apply if we were 
to increase the size of the sk_buff struct, yes?  What I proposed, replacing 
"secmark" with a blob, does not currently change the size of the sk_buff 
struct so the performance and memory usage should remain unchanged as well.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Tuesday, April 09, 2013 10:52:17 AM Paul Moore wrote:
> On Tuesday, April 09, 2013 07:31:04 AM Eric Dumazet wrote:
> > On Tue, 2013-04-09 at 10:19 -0400, Paul Moore wrote:
> > > On Tuesday, April 09, 2013 07:00:22 AM Eric Dumazet wrote:
> > > > On Tue, 2013-04-09 at 09:19 -0400, Paul Moore wrote:
> > > > > As Casey already mentioned, if this isn't acceptable please help me
> > > > > understand why.
> > > > 
> > > > You see something which is not the reality. If you do such analysis,
> > > > better do it properly, because any change you are going to submit will
> > > > be doubly checked by people who really care.
> > > 
> > > I am attempting to do it properly, I simply made a mistake.  Ben also
> > > pointed it out.  As you wrote yesterday, "Lets go forward".
> > > 
> > > After fixing the BITS_PER_LONG problem I looked at it again and it
> > > appears
> > > that by simply replacing the "secmark" field with a blob we retain the
> > > size of the sk_buff as well as the cacheline positions of all the
> > > fields,
> > > e.g. dma_cookie no longer moves cachelines.  Thoughts?
> > 
> > If you take a look at recent history of changes on sk_buff, you can see
> > we added very recently fields for encapsulation support. These were
> > absolutely wanted for modern operations at datacenter level.
> > 
> > This effort might still need new room, so I prefer not filling sk_buff
> > right now.
> 
> Has anyone proposed any additional encapsulation patches which need
> additional fields in the sk_buff?  Are you aware of any additional
> encapsulation patches which are in progress?  When would you consider it
> "safe"?

Another thought.

If we move the "protocol" field after the flags2 bitfield, remove "secmark", 
and add the blob before/after "destructor" then we preserve the existing four 
byte hole in cacheline #3 for potential use by the encapsulation folks and 
move the dma_cookie into cacheline #3 as well which may actually be a good 
thing (corrections welcome on this comment).  The overall size remains the 
same at 256 bytes.

struct sk_buff_test {
        struct sk_buff *           next;                 /*     0     8 */
        struct sk_buff *           prev;                 /*     8     8 */
        ktime_t                    tstamp;               /*    16     8 */
        struct sock *              sk;                   /*    24     8 */
        struct net_device *        dev;                  /*    32     8 */
        char                       cb[48];               /*    40    48 */
        /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
        long unsigned int          _skb_refdst;          /*    88     8 */
        struct sec_path *          sp;                   /*    96     8 */
        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 */
        int                        flags1_begin[0];      /*   124     0 */
        __u8                       local_df:1;           /*   124: 7  1 */
        __u8                       cloned:1;             /*   124: 6  1 */
        __u8                       ip_summed:2;          /*   124: 4  1 */
        __u8                       nohdr:1;              /*   124: 3  1 */
        __u8                       nfctinfo:3;           /*   124: 0  1 */
        __u8                       pkt_type:3;           /*   125: 5  1 */
        __u8                       fclone:2;             /*   125: 3  1 */
        __u8                       ipvs_property:1;      /*   125: 2  1 */
        __u8                       peeked:1;             /*   125: 1  1 */
        __u8                       nf_trace:1;           /*   125: 0  1 */

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

        /* --- cacheline 2 boundary (128 bytes) --- */
        int                        flags1_end[0];        /*   128     0 */
        void *                     security;             /*   128     8 */
        void                       (*destructor)(struct sk_buff *); /*   136     
8 */
        struct nf_conntrack *      nfct;                 /*   144     8 */
        struct sk_buff *           nfct_reasm;           /*   152     8 */
        struct nf_bridge_info *    nf_bridge;            /*   160     8 */
        int                        skb_iif;              /*   168     4 */
        __u32                      rxhash;               /*   172     4 */
        __u16                      vlan_tci;             /*   176     2 */
        __u16                      tc_index;             /*   178     2 */
        __u16                      tc_verd;              /*   180     2 */
        __u16                      queue_mapping;        /*   182     2 */
        int                        flags2_begin[0];      /*   184     0 */
        __u8                       ndisc_nodetype:2;     /*   184: 6  1 */
        __u8                       pfmemalloc:1;         /*   184: 5  1 */
        __u8                       ooo_okay:1;           /*   184: 4  1 */
        __u8                       l4_rxhash:1;          /*   184: 3  1 */
        __u8                       wifi_acked_valid:1;   /*   184: 2  1 */
        __u8                       wifi_acked:1;         /*   184: 1  1 */
        __u8                       no_fcs:1;             /*   184: 0  1 */
        __u8                       head_frag:1;          /*   185: 7  1 */
        __u8                       encapsulation:1;      /*   185: 6  1 */

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

        int                        flags2_end[0];        /*   188     0 */
        __be16                     protocol;             /*   188     2 */

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

        /* --- cacheline 3 boundary (192 bytes) --- */
        dma_cookie_t               dma_cookie;           /*   192     4 */
        union {
                __u32              mark;                 /*           4 */
                __u32              dropcount;            /*           4 */
                __u32              reserved_tailroom;    /*           4 */
        };                                               /*   196     4 */
        sk_buff_data_t             inner_transport_header; /*   200     4 */
        sk_buff_data_t             inner_network_header; /*   204     4 */
        sk_buff_data_t             transport_header;     /*   208     4 */
        sk_buff_data_t             network_header;       /*   212     4 */
        sk_buff_data_t             mac_header;           /*   216     4 */
        sk_buff_data_t             tail;                 /*   220     4 */
        sk_buff_data_t             end;                  /*   224     4 */

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

        unsigned char *            head;                 /*   232     8 */
        unsigned char *            data;                 /*   240     8 */
        unsigned int               truesize;             /*   248     4 */
        atomic_t                   users;                /*   252     4 */
        /* --- cacheline 4 boundary (256 bytes) --- */

        /* size: 256, cachelines: 4, members: 62 */
        /* sum members: 246, holes: 4, sum holes: 10 */
        /* bit holes: 1, sum bit holes: 6 bits */
};


-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Tue, 2013-04-09 at 10:52 -0400, Paul Moore wrote:

> 
> Perhaps I'm misunderstanding, but these comments above only apply if we were 
> to increase the size of the sk_buff struct, yes?  What I proposed, replacing 
> "secmark" with a blob, does not currently change the size of the sk_buff 
> struct so the performance and memory usage should remain unchanged as well.
> 

If blob size is 4 bytes, thats fine.

If not, read again my mail.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Tuesday, April 09, 2013 08:07:06 AM Eric Dumazet wrote:
> On Tue, 2013-04-09 at 10:52 -0400, Paul Moore wrote:
> > Perhaps I'm misunderstanding, but these comments above only apply if we
> > were to increase the size of the sk_buff struct, yes?  What I proposed,
> > replacing "secmark" with a blob, does not currently change the size of
> > the sk_buff struct so the performance and memory usage should remain
> > unchanged as well.
>
> If blob size is 4 bytes, thats fine.
> 
> If not, read again my mail.

The "blob" is a void pointer, so 8 bytes.  We're talking about removing the 
"secmark" field (4 bytes) and adding a void pointer (8 bytes).  I've shown 
several different approaches that make this change without increasing the 
overall size of the sk_buff struct.

One of the proposals makes use of the existing holes in the third cacheline to 
make the change without any increase in size, misalignment, or cacheline 
changes.  You were concerned that at some point in the future, the hardware 
encapsulation developers *might* want to add another field.

Taking your comments into consideration I just made another proposal which 
preserves the overall size of the sk_buff struct, as well as the 4 byte hole 
in the third cacheline (for possible use by hw encapsulation folks at some 
later date).  It does shift the "dma_cookie" field from the second to the 
third cacheline, but considering the fields already in the third cacheline 
this may be a good thing.

To the best of my knowledge, all of the proposals I've posted this morning do 
not change the size of the sk_buff so the cloned sk_buff 
functionality/performance/etc. should not be affected.  If that is not the 
case, please let me know because I'm currently at a loss (even after re-
reading your email).

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Tue, 2013-04-09 at 11:17 -0400, Paul Moore wrote:

> The "blob" is a void pointer, so 8 bytes.  We're talking about removing the 
> "secmark" field (4 bytes) and adding a void pointer (8 bytes).  I've shown 
> several different approaches that make this change without increasing the 
> overall size of the sk_buff struct.

You want to use 4 extra bytes in sk_buff. You'll have to show us why we
close the way for other valid uses of the current holes.

I have no idea why its needed, and why it can't be solved in another
way.

It looks like _I_ have to do your work. Sorry, I have no more time to
spend on this topic. You'll have to convince David, not me.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Tuesday, April 09, 2013 08:32:56 AM Eric Dumazet wrote:
> On Tue, 2013-04-09 at 11:17 -0400, Paul Moore wrote:
> > The "blob" is a void pointer, so 8 bytes.  We're talking about removing
> > the "secmark" field (4 bytes) and adding a void pointer (8 bytes).  I've
> > shown several different approaches that make this change without
> > increasing the overall size of the sk_buff struct.
> 
> You want to use 4 extra bytes in sk_buff. You'll have to show us why we
> close the way for other valid uses of the current holes.
> 
> I have no idea why its needed, and why it can't be solved in another
> way.

FWIW, I was focusing on arriving at a basic design that addressed the initial 
reasons for not including a security blob in a sk_buff.  In the beginning I 
thought it was both the need for LSM hook in the skb management routines as 
well as the memory overhead in the skb itself.  During the course of our 
discussion it became clear that the hooks were acceptable, it was the memory 
overhead that was the concern, so that is what I (and Casey) focused on.

Based on your latest comment, it appears that we have some possible candidates 
for adding a security blob (void *) to the sk_buff that address your technical 
arguments, I wasn't aware we had reached that point, but it is indeed good 
news.  Now we just need to make our case that it is the "Right Thing to Do", 
that is perfectly reasonable.

> It looks like _I_ have to do your work.

I don't believe I ever asked you to do anything other than to repost a patch 
you posted to the LSM list so we could get it included upstream.  A patch that 
you created to counter my proposed fix for a SELinux regression.  Further, I 
tested your patch, and ACK'd it earlier this morning.

I suppose I also asked you to explain/clarify a few of your technical 
objections a bit further so I could address them, but that just seems like 
normal peer design review.

> Sorry, I have no more time to spend on this topic. You'll have to convince
> David, not me.

Well, thank you for your time; I'm sure we'll get to talk about this again in 
the future.  It looks like we've had enough of a conversation now that I can 
start working on some patches.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Casey Schaufler]

On 4/9/2013 8:32 AM, Eric Dumazet wrote:
> On Tue, 2013-04-09 at 11:17 -0400, Paul Moore wrote:
>
>> The "blob" is a void pointer, so 8 bytes.  We're talking about removing the 
>> "secmark" field (4 bytes) and adding a void pointer (8 bytes).  I've shown 
>> several different approaches that make this change without increasing the 
>> overall size of the sk_buff struct.
> You want to use 4 extra bytes in sk_buff.

So far, so good.

> You'll have to show us why we
> close the way for other valid uses of the current holes.

We have what looks to us like a very legitimate
use for a 4 byte hole, we have that use defined,
and we have it today. In fact, we've had it for the
last five years. The 4 byte secmark (instead of an
8 byte blob pointer) has impacted design choices
since the LSM broke out of the SELinux monopoly.
Not having a blob *will* negatively impact the
forthcoming multiple concurrent LSM support. That
is not hypothetical, that is concrete and very
much a problem.

> I have no idea why its needed, and why it can't be solved in another
> way.

Ah, let's see if I can't help there.

We have two LSMs today and a third on the way that
potentially want to use the secmark. SELinux uses the
secmark, Smack may pick it up for IPv6 support in the
not to distant future, and AppArmor appears to be
considering it as well.

Each of these LSMs uses (or will use) a 4 byte secid
to represent the security information about a process
or other relevant entity. This is what gets put into
the secmark. The secid maps to the real security information.

If I have two LSMs, say SELinux and AppArmor, I have a
real problem. I have 8 bytes of data and a 4 byte
secmark. If I have SELinux, Smack and AppArmor I have
12 bytes to stuff into a 4 byte bag. To top it off,
these are already indirect references to the security
data, not pointers to the data. What I have to try to
do is fit 3 pointers into 4 bytes. Not good.

If we replace secmark with secblob, I don't have to
use the secid indirection if there is only one LSM.
If there are multiple LSMs the secblob contains a
pointer to a master blob, and it's still faster than
the secmark indirection.

The alternative is to restrict the use of secmarks to
one LSM and let all the others figure out some other
method for communicating the security information.
I don't see that as a great choice.

> It looks like _I_ have to do your work. Sorry, I have no more time to
> spend on this topic. You'll have to convince David, not me.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 09 Apr 2013 08:32:56 -0700

> It looks like _I_ have to do your work. Sorry, I have no more time to
> spend on this topic. You'll have to convince David, not me.

I already have no interest in considering these changes seriously,
and I've already asked Paul multiple times to drop this idea.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Tuesday, April 09, 2013 12:56:35 PM David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 09 Apr 2013 08:32:56 -0700
> 
> > It looks like _I_ have to do your work. Sorry, I have no more time to
> > spend on this topic. You'll have to convince David, not me.
> 
> I already have no interest in considering these changes seriously,
> and I've already asked Paul multiple times to drop this idea.

If we address all of your technical concerns, why are you not interested in 
allowing a security blob in the sk_buff?

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Tue, 09 Apr 2013 13:00:06 -0400

> On Tuesday, April 09, 2013 12:56:35 PM David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Tue, 09 Apr 2013 08:32:56 -0700
>> 
>> > It looks like _I_ have to do your work. Sorry, I have no more time to
>> > spend on this topic. You'll have to convince David, not me.
>> 
>> I already have no interest in considering these changes seriously,
>> and I've already asked Paul multiple times to drop this idea.
> 
> If we address all of your technical concerns, why are you not interested in 
> allowing a security blob in the sk_buff?

If you have a patch that removes space, rather than adds space, to sk_buff,
then we can talk.

Otherwise, please don't waste our time, because I'm not adding anything to
sk_buff for LSM's sake.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[David Miller]

And please, until you have a real patch, stop wasting our time.

Like Eric, as you can imagine, I have hundreds of other people's patches
to review and reports to analyze.

You're issue is not so important compared to all of the other ones
that it should consume such an unbalanced amount of my time, sorry.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Ben Hutchings]

On Tue, 2013-04-09 at 09:19 -0400, Paul Moore wrote:
> On Monday, April 08, 2013 06:24:59 PM Casey Schaufler wrote:
> > On 4/8/2013 6:09 PM, Eric Dumazet wrote:
> > > On Mon, 2013-04-08 at 17:59 -0700, Casey Schaufler wrote:
> > >> I don't see that with adding 4 bytes. Again, I'm willing to be
> > >> educated if I'm wrong.
> > > 
> > > Feel free to add 4 bytes without having the 'align to 8 bytes' problem
> > > on 64 bit arches. Show us your patch.
> > 
> > Recall that it's replacing an existing 4 byte value with an 8 byte value.
> > My compiler days were quite short and long ago, but it would seem that
> > an 8 byte value ought not have an 'align to 8 bytes' problem.
> > 
> > Again, I'm willing to be educated.
> 
> Armed with a cup of coffee I took a look at the sk_buff structure this morning 
> with the pahole tool and using the current sk_buff if we turn on all the 
> #ifdefs here is what I see on x86_64:
> 
> struct sk_buff {
[...]
>         sk_buff_data_t             inner_transport_header; /*   200     8 */
>         sk_buff_data_t             inner_network_header; /*   208     8 */
>         sk_buff_data_t             transport_header;     /*   216     8 */
>         sk_buff_data_t             network_header;       /*   224     8 */
>         sk_buff_data_t             mac_header;           /*   232     8 */
>         sk_buff_data_t             tail;                 /*   240     8 */
>         sk_buff_data_t             end;                  /*   248     8 */
[...]

This is wrong; sk_buff_data_t is always 32-bit:

#if BITS_PER_LONG > 32
#define NET_SKBUFF_DATA_USES_OFFSET 1
#endif

#ifdef NET_SKBUFF_DATA_USES_OFFSET
typedef unsigned int sk_buff_data_t;
#else
typedef unsigned char *sk_buff_data_t;
#endif

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Tuesday, April 09, 2013 03:05:42 PM Ben Hutchings wrote:
> On Tue, 2013-04-09 at 09:19 -0400, Paul Moore wrote:
> > On Monday, April 08, 2013 06:24:59 PM Casey Schaufler wrote:
> > > On 4/8/2013 6:09 PM, Eric Dumazet wrote:
> > > > On Mon, 2013-04-08 at 17:59 -0700, Casey Schaufler wrote:
> > > >> I don't see that with adding 4 bytes. Again, I'm willing to be
> > > >> educated if I'm wrong.
> > > > 
> > > > Feel free to add 4 bytes without having the 'align to 8 bytes' problem
> > > > on 64 bit arches. Show us your patch.
> > > 
> > > Recall that it's replacing an existing 4 byte value with an 8 byte
> > > value.
> > > My compiler days were quite short and long ago, but it would seem that
> > > an 8 byte value ought not have an 'align to 8 bytes' problem.
> > > 
> > > Again, I'm willing to be educated.
> > 
> > Armed with a cup of coffee I took a look at the sk_buff structure this
> > morning with the pahole tool and using the current sk_buff if we turn on
> > all the #ifdefs here is what I see on x86_64:
> > 
> > struct sk_buff {
> 
> [...]
> 
> >         sk_buff_data_t             inner_transport_header; /*   200     8
> >         */
> >         sk_buff_data_t             inner_network_header; /*   208     8 */
> >         sk_buff_data_t             transport_header;     /*   216     8 */
> >         sk_buff_data_t             network_header;       /*   224     8 */
> >         sk_buff_data_t             mac_header;           /*   232     8 */
> >         sk_buff_data_t             tail;                 /*   240     8 */
> >         sk_buff_data_t             end;                  /*   248     8 */
> 
> [...]
> 
> This is wrong; sk_buff_data_t is always 32-bit:

Yep.  My mistake.

While looking at this a bit more after my original email I noticed the same 
thing.  Ultimately it doesn't change the size or cachelines as the 
sk_buff_data_t structures are at the bottom of sk_buff but here are the 
correct breakdowns:

ORIG:
struct sk_buff {
        struct sk_buff *           next;                 /*     0     8 */
        struct sk_buff *           prev;                 /*     8     8 */
        ktime_t                    tstamp;               /*    16     8 */
        struct sock *              sk;                   /*    24     8 */
        struct net_device *        dev;                  /*    32     8 */
        char                       cb[48];               /*    40    48 */
        /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
        long unsigned int          _skb_refdst;          /*    88     8 */
        struct sec_path *          sp;                   /*    96     8 */
        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 */
        int                        flags1_begin[0];      /*   124     0 */
        __u8                       local_df:1;           /*   124: 7  1 */
        __u8                       cloned:1;             /*   124: 6  1 */
        __u8                       ip_summed:2;          /*   124: 4  1 */
        __u8                       nohdr:1;              /*   124: 3  1 */
        __u8                       nfctinfo:3;           /*   124: 0  1 */
        __u8                       pkt_type:3;           /*   125: 5  1 */
        __u8                       fclone:2;             /*   125: 3  1 */
        __u8                       ipvs_property:1;      /*   125: 2  1 */
        __u8                       peeked:1;             /*   125: 1  1 */
        __u8                       nf_trace:1;           /*   125: 0  1 */

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

        /* --- cacheline 2 boundary (128 bytes) --- */
        int                        flags1_end[0];        /*   128     0 */
        __be16                     protocol;             /*   128     2 */

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

        void                       (*destructor)(struct sk_buff *); /*   136     
8 */
        struct nf_conntrack *      nfct;                 /*   144     8 */
        struct sk_buff *           nfct_reasm;           /*   152     8 */
        struct nf_bridge_info *    nf_bridge;            /*   160     8 */
        int                        skb_iif;              /*   168     4 */
        __u32                      rxhash;               /*   172     4 */
        __u16                      vlan_tci;             /*   176     2 */
        __u16                      tc_index;             /*   178     2 */
        __u16                      tc_verd;              /*   180     2 */
        __u16                      queue_mapping;        /*   182     2 */
        int                        flags2_begin[0];      /*   184     0 */
        __u8                       ndisc_nodetype:2;     /*   184: 6  1 */
        __u8                       pfmemalloc:1;         /*   184: 5  1 */
        __u8                       ooo_okay:1;           /*   184: 4  1 */
        __u8                       l4_rxhash:1;          /*   184: 3  1 */
        __u8                       wifi_acked_valid:1;   /*   184: 2  1 */
        __u8                       wifi_acked:1;         /*   184: 1  1 */
        __u8                       no_fcs:1;             /*   184: 0  1 */
        __u8                       head_frag:1;          /*   185: 7  1 */
        __u8                       encapsulation:1;      /*   185: 6  1 */

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

        int                        flags2_end[0];        /*   188     0 */
        dma_cookie_t               dma_cookie;           /*   188     4 */
        /* --- cacheline 3 boundary (192 bytes) --- */
        __u32                      secmark;              /*   192     4 */
        union {
                __u32              mark;                 /*           4 */
                __u32              dropcount;            /*           4 */
                __u32              reserved_tailroom;    /*           4 */
        };                                               /*   196     4 */
        sk_buff_data_t             inner_transport_header; /*   200     4 */
        sk_buff_data_t             inner_network_header; /*   204     4 */
        sk_buff_data_t             transport_header;     /*   208     4 */
        sk_buff_data_t             network_header;       /*   212     4 */
        sk_buff_data_t             mac_header;           /*   216     4 */
        sk_buff_data_t             tail;                 /*   220     4 */
        sk_buff_data_t             end;                  /*   224     4 */

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

        unsigned char *            head;                 /*   232     8 */
        unsigned char *            data;                 /*   240     8 */
        unsigned int               truesize;             /*   248     4 */
        atomic_t                   users;                /*   252     4 */
        /* --- cacheline 4 boundary (256 bytes) --- */

        /* size: 256, cachelines: 4, members: 62 */
        /* sum members: 242, holes: 4, sum holes: 14 */
        /* bit holes: 1, sum bit holes: 6 bits */
};

W/BLOB:
struct sk_buff_test {
        struct sk_buff *           next;                 /*     0     8 */
        struct sk_buff *           prev;                 /*     8     8 */
        ktime_t                    tstamp;               /*    16     8 */
        struct sock *              sk;                   /*    24     8 */
        struct net_device *        dev;                  /*    32     8 */
        char                       cb[48];               /*    40    48 */
        /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
        long unsigned int          _skb_refdst;          /*    88     8 */
        struct sec_path *          sp;                   /*    96     8 */
        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 */
        int                        flags1_begin[0];      /*   124     0 */
        __u8                       local_df:1;           /*   124: 7  1 */
        __u8                       cloned:1;             /*   124: 6  1 */
        __u8                       ip_summed:2;          /*   124: 4  1 */
        __u8                       nohdr:1;              /*   124: 3  1 */
        __u8                       nfctinfo:3;           /*   124: 0  1 */
        __u8                       pkt_type:3;           /*   125: 5  1 */
        __u8                       fclone:2;             /*   125: 3  1 */
        __u8                       ipvs_property:1;      /*   125: 2  1 */
        __u8                       peeked:1;             /*   125: 1  1 */
        __u8                       nf_trace:1;           /*   125: 0  1 */

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

        /* --- cacheline 2 boundary (128 bytes) --- */
        int                        flags1_end[0];        /*   128     0 */
        void *                     security;             /*   128     8 */
        void                       (*destructor)(struct sk_buff *); /*   136     
8 */
        struct nf_conntrack *      nfct;                 /*   144     8 */
        struct sk_buff *           nfct_reasm;           /*   152     8 */
        struct nf_bridge_info *    nf_bridge;            /*   160     8 */
        int                        skb_iif;              /*   168     4 */
        __u32                      rxhash;               /*   172     4 */
        __u16                      vlan_tci;             /*   176     2 */
        __u16                      tc_index;             /*   178     2 */
        __u16                      tc_verd;              /*   180     2 */
        __u16                      queue_mapping;        /*   182     2 */
        int                        flags2_begin[0];      /*   184     0 */
        __u8                       ndisc_nodetype:2;     /*   184: 6  1 */
        __u8                       pfmemalloc:1;         /*   184: 5  1 */
        __u8                       ooo_okay:1;           /*   184: 4  1 */
        __u8                       l4_rxhash:1;          /*   184: 3  1 */
        __u8                       wifi_acked_valid:1;   /*   184: 2  1 */
        __u8                       wifi_acked:1;         /*   184: 1  1 */
        __u8                       no_fcs:1;             /*   184: 0  1 */
        __u8                       head_frag:1;          /*   185: 7  1 */
        __u8                       encapsulation:1;      /*   185: 6  1 */

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

        int                        flags2_end[0];        /*   188     0 */
        __be16                     protocol;             /*   188     2 */

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

        /* --- cacheline 3 boundary (192 bytes) --- */
        dma_cookie_t               dma_cookie;           /*   192     4 */
        union {
                __u32              mark;                 /*           4 */
                __u32              dropcount;            /*           4 */
                __u32              reserved_tailroom;    /*           4 */
        };                                               /*   196     4 */
        sk_buff_data_t             inner_transport_header; /*   200     4 */
        sk_buff_data_t             inner_network_header; /*   204     4 */
        sk_buff_data_t             transport_header;     /*   208     4 */
        sk_buff_data_t             network_header;       /*   212     4 */
        sk_buff_data_t             mac_header;           /*   216     4 */
        sk_buff_data_t             tail;                 /*   220     4 */
        sk_buff_data_t             end;                  /*   224     4 */

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

        unsigned char *            head;                 /*   232     8 */
        unsigned char *            data;                 /*   240     8 */
        unsigned int               truesize;             /*   248     4 */
        atomic_t                   users;                /*   252     4 */
        /* --- cacheline 4 boundary (256 bytes) --- */

        /* size: 256, cachelines: 4, members: 62 */
        /* sum members: 246, holes: 4, sum holes: 10 */
        /* bit holes: 1, sum bit holes: 6 bits */
};

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Ben Hutchings]

On Mon, 2013-04-08 at 17:24 -0400, Paul Moore wrote:
> On Monday, April 08, 2013 05:15:12 PM David Miller wrote:
> > From: Paul Moore <pmoore@redhat.com>
> > Date: Mon, 08 Apr 2013 17:10:43 -0400
> > 
> > > On Monday, April 08, 2013 02:32:00 PM Paul Moore wrote:
> > >> On Monday, April 08, 2013 02:12:01 PM Paul Moore wrote:
> > >> > On Monday, April 08, 2013 10:47:47 AM Eric Dumazet wrote:
> > >> > > On Mon, 2013-04-08 at 13:40 -0400, Paul Moore wrote:
> > >> > > > Sort of a similar problem, but not really the same.  Also,
> > >> > > > arguably,
> > >> > > > there is no real associated sock/socket for a RST so orphaning the
> > >> > > > packet makes sense. In the case of a SYNACK we can, and should,
> > >> > > > associate the packet with a sock/socket.
> > >> > > 
> > >> > > What is the intent ?
> > >> > 
> > >> > We have to do a number of painful things in SELinux because we aren't
> > >> > allowed a proper security blob (void *security) in a sk_buff.  One of
> > >> > those things ...
> > >> 
> > >> Actually, I wonder if this problem means it is a good time to revisit the
> > >> no- security-blob-in-sk_buff decision?  The management of the blob would
> > >> be hidden behind the LSM hooks like everything else and it would have a
> > >> number of advantages including making problems like we are seeing here
> > >> easier to fix or avoid entirely.  It would also make life much easier for
> > >> those of working on LSM stuff and it would pave the way for including
> > >> network access controls in the stacked-LSM stuff Casey is kicking around.
> > > 
> > > No comment, or am I just too anxious?
> > 
> > There is no way I'm putting LSM overhead into sk_buff, it's already
> > too big.
> 
> If the void pointer is wrapped by a #ifdef (plenty of precedence for that) and 
> the management of that pointer is handled by LSM hooks why is it a concern?  I 
> apologize for pushing on the issue, but I'm having a hard time reconciling the 
> reason for the "no" with the comments/decisions about the regression fix; at 
> present there seems to be a level of contradiction between the two.

Most Linux users are running distribution kernels with one or more LSMs
built-in.  Anything you make dependent on CONFIG_SECURITY or similar
generic symbol will have a cost for all those users, whether or not they
actually use the LSM.

Ben.

> > I didn't comment because it wasn't worth a comment, but since you're
> > pushing me on the issue, I'll make the no explicit.
> 

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Mon, 08 Apr 2013 13:22:50 -0400

> Contrary to popular belief, my goal is to not destroy the scalability and/or 
> performance of our network stack, I just want to make sure we have a quality 
> network stack that is not only fast and scalable, but also preserves the 
> security functionality that makes Linux attractive to a number of users.

Get the violin out.

> To that end, we could put a #ifdef in the middle of
> tcp_make_synack(), but that seems very ugly to me and I think sets a
> bad precedence for the network stack and kernel as a whole.

Not an ifdef, a run time state test.

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Eric Dumazet]

On Mon, 2013-04-08 at 11:45 -0400, Paul Moore wrote:
> Commit 90ba9b1986b5ac4b2d184575847147ea7c4280a2 converted
> tcp_make_synack() to use alloc_skb() directly instead of calling
> sock_wmalloc(), the goal being the elimination of two atomic
> operations.  Unfortunately, in doing so the change broke certain
> SELinux/NetLabel configurations by no longer correctly assigning
> the sock to the outgoing packet.
> 
> This patch fixes this regression by doing the skb->sk assignment
> directly inside tcp_make_synack().
> 
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Signed-off-by: Paul Moore <pmoore@redhat.com>
> ---
>  net/ipv4/tcp_output.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 5d0b438..23cc295 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2705,6 +2705,8 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
>  		dst_release(dst);
>  		return NULL;
>  	}
> +	skb->sk = sk;
> +

Hmm...

Keeping a pointer on a socket without taking a refcount is not going to
work.

We are trying to make the stack scale, so you need to add a selinux call
to take a ref count only if needed.

That is : If selinux is not used, we don't need to slow down the stack.
 

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Sergei Shtylyov]

Hello.

On 04/08/2013 07:45 PM, Paul Moore wrote:

> Commit 90ba9b1986b5ac4b2d184575847147ea7c4280a2

    Please also specify that commit's summary line in parens
(DaveM seems to also require double quotes inside the parens).

> converted
> tcp_make_synack() to use alloc_skb() directly instead of calling
> sock_wmalloc(), the goal being the elimination of two atomic
> operations.  Unfortunately, in doing so the change broke certain
> SELinux/NetLabel configurations by no longer correctly assigning
> the sock to the outgoing packet.
>
> This patch fixes this regression by doing the skb->sk assignment
> directly inside tcp_make_synack().
>
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Signed-off-by: Paul Moore <pmoore@redhat.com>

WBR, Sergei

Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet

[Paul Moore]

On Monday, April 08, 2013 10:03:33 PM Sergei Shtylyov wrote:
> Hello.
> 
> On 04/08/2013 07:45 PM, Paul Moore wrote:
> > Commit 90ba9b1986b5ac4b2d184575847147ea7c4280a2
> 
>     Please also specify that commit's summary line in parens
> (DaveM seems to also require double quotes inside the parens).

Noted for future reference.

-- 
paul moore
security and virtualization @ redhat