Re: [PATCH -next v2] unix stream: Fix use-after-free crashes

[Eric Dumazet <eric.dumazet@gmail.com>]

Le mardi 06 septembre 2011 à 15:08 -0700, Tim Chen a écrit :
> On Tue, 2011-09-06 at 22:19 +0200, Eric Dumazet wrote:
> 
> > 
> > unless scm_ref really means scm_noref ?
> > 
> > I really hate this patch. I mean it. 
> > 
> > I read it 10 times, spent 2 hours and still dont understand it.
> > 
> 
> Eric,
> 
> I've tried another patch to fix my original one.  I've used a boolean
> ref_avail to indicate if there is an outstanding ref to scm not yet
> encoded into the skb.  Hopefully the logic is clearer in this new patch.
> 
> > 
> > As I said, we should revert the buggy patch, and rewrite a performance
> > fix from scratch, with not a single get_pid()/put_pid() in fast path.
> > 
> > read()/write() on AF_UNIX sockets should not use a single
> > get_pid()/put_pid().
> > 
> > This is a serious regression we should fix at 100%, not 50% or even 75%,
> > adding serious bugs.
> 
> That will be ideal if there is another way to fix it 100%, other than reverting
> commit 7361c36c.  Probably if there is some way we know beforehand that 
> both sender and receiver share the same pid, which is quite common, a
> lot of these pid code can be bypassed. 
> 

Let me restate : Its should be obvious to fix the performance hit for
good.

If namespaces are not used (CONFIG_PID_NS is not set), we can use the
old code, prior to commit 7361c36c : store pid/uid/gid in skb->cb[]

But more generally, when a write() is done on AF_UNIX socket, we pass a
NULL siocb->scm to unix_{dgram|stream}_sendmsg()

if (NULL == siocb->scm)
	siocb->scm = &tmp_scm;

There is no need in this case to copy in each skb->cb, pointers to
struct pid and struct cred with their atomic reference being changed in
the sender and receiver.

We try to remove _all_ atomic ops on refcounts not only because atomic
ops are expensive by themselves, but also because of the cache line ping
pongs. 


> Tim
> 
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---
> 

When a patch is wrong, you can admit it and ask for a revert, instead of
obfuscating the code so much that even a netdev guy like me doesnt
understand it anymore.

We speak of a very recent patch in net-next, not yet published to Linus
tree. There is no shame to revert it right now and work on a new patch.

I want to be able to track future bugs on this code, and your patch and
their fixes made functions too hard to read.

If you dont want to work on it, I'll do it myself.