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

Paul Moore
2013-04-08T17:22:54Z

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