Re: [PATCH for-next V6 3/5] IB/uverbs: Enable device removal when there are active user space applications

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Mon, Jul 06, 2015 at 05:08:08PM +0300, Yishai Hadas wrote:

> The patch that introduces this bug was added 5 years ago by Alex Chiang and
> Signed-off-by: Roland Dreier.
> 
> Look at commit ID:2a72f212263701b927559f6850446421d5906c41, it can be seen
> also at:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2a72f212263701b

Perhaps, this one also looks involved as well:

commit 055422ddbb0a7610c5f57a056743d7336a39e90f
Author: Alexander Chiang <achiang-VXdhtT5mjnY@public.gmane.org>
Date:   Tue Feb 2 19:07:49 2010 +0000

    IB/uverbs: Convert *cdev to cdev in struct ib_uverbs_device
    
    Instead of storing a pointer to a cdev, embed the entire struct cdev.

Embedding the cdev without using a parent kobject looks like the root
mistake.

> AFAIK V6 addressed all opened comments raised by Jason, including the last
> one that asked to use 2 separate krefs for both complete and free, it didn't
> introduced the problem above.

It does make it worse though, previously the module locking would make
it unlikely to ever hit any problem here, but now we have a naked
fully exposed race where release races with kfree resulting in
use-after-free. I'd think hitting it is quite likely if the new
feature is being used, and subtle memory corruption is not something
we want to see in the kernel.

So, I'd say, yes it is an old bug, but it is unlikely to hit it. This
patch series is making it much likely, so it needs to be fixed.

In any event, I'm not sure what you are complaining about - this
series absolutely reworks the lifetime model of ib_uverbs_device, why
on earth do you think it is OK to have a buggy new implementation just
because the previous version was buggy? *Especially* when someone
takes the time to point out the mistake and tells you exactly how to
fix it, and it is *trival* to do?

Even worse: I went through and audited the lifetime of V6's new model,
and I think that is *absolutely* something you should have done before
sending V1 :(

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html