[PATCH] infiniband: fix a subtle race condition

[PATCH] infiniband: fix a subtle race condition

[Cong Wang]

In ucma_event_handler() we lock the mutex like this:

mutex_lock(&ctx->file->mut);
...
mutex_unlock(&ctx->file->mut);

which seems correct, but we could translate it into this:

f = ctx->file;
mutex_lock(&f->mut);
...
f = ctx->file;
mutex_unlock(&f->mut);

as the compiler does. And, because ucma_event_handler() is
called in a workqueue so it could race with ucma_migrate_id(),
so the following race condition could happen:

CPU0			CPU1
f = ctx->file;
			ucma_lock_files(f, new_file);
			ctx->file = new_file
			ucma_lock_files(f, new_file);
mutex_lock(&f->mut); // still the old file!
...
f = ctx->file; // now the new one!!
mutex_unlock(&f->mut); // unlock new file!

Fix this by reading ctx->file once before mutex_lock(), so we
won't unlock a different mutex any more.

Reported-by: syzbot+e5579222b6a3edd96522@syzkaller.appspotmail.com
Cc: Doug Ledford <dledford@redhat.com>
Cc: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 drivers/infiniband/core/ucma.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
index ec8fb289621f..8729d6acf981 100644
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -341,13 +341,15 @@ static int ucma_event_handler(struct rdma_cm_id *cm_id,
 {
 	struct ucma_event *uevent;
 	struct ucma_context *ctx = cm_id->context;
+	struct ucma_file *cur_file;
 	int ret = 0;
 
 	uevent = kzalloc(sizeof(*uevent), GFP_KERNEL);
 	if (!uevent)
 		return event->event == RDMA_CM_EVENT_CONNECT_REQUEST;
 
-	mutex_lock(&ctx->file->mut);
+	cur_file = ctx->file;
+	mutex_lock(&cur_file->mut);
 	uevent->cm_id = cm_id;
 	ucma_set_event_context(ctx, event, uevent);
 	uevent->resp.event = event->event;
@@ -382,12 +384,12 @@ static int ucma_event_handler(struct rdma_cm_id *cm_id,
 		goto out;
 	}
 
-	list_add_tail(&uevent->list, &ctx->file->event_list);
-	wake_up_interruptible(&ctx->file->poll_wait);
+	list_add_tail(&uevent->list, &cur_file->event_list);
+	wake_up_interruptible(&cur_file->poll_wait);
 	if (event->event == RDMA_CM_EVENT_DEVICE_REMOVAL)
 		ucma_removal_event_handler(cm_id);
 out:
-	mutex_unlock(&ctx->file->mut);
+	mutex_unlock(&cur_file->mut);
 	return ret;
 }
 
-- 
2.14.4

Re: [PATCH] infiniband: fix a subtle race condition

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

On Wed, Jun 13, 2018 at 04:49:47PM -0700, Cong Wang wrote:
> In ucma_event_handler() we lock the mutex like this:
>
> mutex_lock(&ctx->file->mut);
> ...
> mutex_unlock(&ctx->file->mut);
>
> which seems correct, but we could translate it into this:
>
> f = ctx->file;
> mutex_lock(&f->mut);
> ...
> f = ctx->file;
> mutex_unlock(&f->mut);
>
> as the compiler does. And, because ucma_event_handler() is
> called in a workqueue so it could race with ucma_migrate_id(),
> so the following race condition could happen:
>
> CPU0			CPU1
> f = ctx->file;
> 			ucma_lock_files(f, new_file);
> 			ctx->file = new_file
> 			ucma_lock_files(f, new_file);
> mutex_lock(&f->mut); // still the old file!
> ...
> f = ctx->file; // now the new one!!
> mutex_unlock(&f->mut); // unlock new file!
>
> Fix this by reading ctx->file once before mutex_lock(), so we
> won't unlock a different mutex any more.

Hi Cong,

If the compiler optimizes the first line (mutex_lock) as you wrote,
it will reuse "f" for the second line (mutex_unlock) too.

You need to ensure that ucma_modify_id() doesn't run in parallel to
anything that uses "ctx->file" directly and indirectly.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] infiniband: fix a subtle race condition

[Cong Wang]

On Wed, Jun 13, 2018 at 10:34 PM, Leon Romanovsky <leon@kernel.org> wrote:
>
> Hi Cong,
>
> If the compiler optimizes the first line (mutex_lock) as you wrote,
> it will reuse "f" for the second line (mutex_unlock) too.

Nope, check the assembly if you don't trust me, at least
my compiler always fetches ctx->file without this patch.

I can show you the assembly code tomorrow (too late to
access my dev machine now).


>
> You need to ensure that ucma_modify_id() doesn't run in parallel to
> anything that uses "ctx->file" directly and indirectly.
>

Talk is easy, show me the code. :) I knew there is probably
some other race with this code even after my patch, possibly with
->close() for example, but for this specific unlock warning, this patch
is sufficient. I can't solve all the races in one patch.

Re: [PATCH] infiniband: fix a subtle race condition

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

On Wed, Jun 13, 2018 at 11:21:54PM -0700, Cong Wang wrote:
> On Wed, Jun 13, 2018 at 10:34 PM, Leon Romanovsky <leon@kernel.org> wrote:
> >
> > Hi Cong,
> >
> > If the compiler optimizes the first line (mutex_lock) as you wrote,
> > it will reuse "f" for the second line (mutex_unlock) too.
>
> Nope, check the assembly if you don't trust me, at least
> my compiler always fetches ctx->file without this patch.
>
> I can show you the assembly code tomorrow (too late to
> access my dev machine now).

I trust you, so don't need to check it however wanted to emphasize
that your solution is compiler specific and not universally true.

>
>
> >
> > You need to ensure that ucma_modify_id() doesn't run in parallel to
> > anything that uses "ctx->file" directly and indirectly.
> >
>
> Talk is easy, show me the code. :) I knew there is probably
> some other race with this code even after my patch, possibly with
> ->close() for example, but for this specific unlock warning, this patch
> is sufficient. I can't solve all the races in one patch.

We do prefer complete solution once the problem is fully understood.

It looks like you are one step away from final patch. It will be conversion
of mutex to be rwlock and separating between read (almost in all places)
and write (in ucma_migrate_id) paths.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] infiniband: fix a subtle race condition

[Jason Gunthorpe]

On Thu, Jun 14, 2018 at 10:01:08AM +0300, Leon Romanovsky wrote:
> On Wed, Jun 13, 2018 at 11:21:54PM -0700, Cong Wang wrote:
> > On Wed, Jun 13, 2018 at 10:34 PM, Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > Hi Cong,
> > >
> > > If the compiler optimizes the first line (mutex_lock) as you wrote,
> > > it will reuse "f" for the second line (mutex_unlock) too.
> >
> > Nope, check the assembly if you don't trust me, at least
> > my compiler always fetches ctx->file without this patch.
> >
> > I can show you the assembly code tomorrow (too late to
> > access my dev machine now).
> 
> I trust you, so don't need to check it however wanted to emphasize
> that your solution is compiler specific and not universally true.
> 
> >
> >
> > >
> > > You need to ensure that ucma_modify_id() doesn't run in parallel to
> > > anything that uses "ctx->file" directly and indirectly.
> > >
> >
> > Talk is easy, show me the code. :) I knew there is probably
> > some other race with this code even after my patch, possibly with
> > ->close() for example, but for this specific unlock warning, this patch
> > is sufficient. I can't solve all the races in one patch.
> 
> We do prefer complete solution once the problem is fully understood.
> 
> It looks like you are one step away from final patch. It will be conversion
> of mutex to be rwlock and separating between read (almost in all places)
> and write (in ucma_migrate_id) paths.

This was my brief reaction too, this code path almost certainly has a
use-after-free, and we should fix the concurrency between the two
places in some correct way..

Jason

Re: [PATCH] infiniband: fix a subtle race condition

[Cong Wang]

On Thu, Jun 14, 2018 at 7:24 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> This was my brief reaction too, this code path almost certainly has a
> use-after-free, and we should fix the concurrency between the two
> places in some correct way..

First of all, why use-after-free could trigger an imbalance unlock?
IOW, why do we have to solve use-after-free to fix this imbalance
unlock?

Second of all, my patch is _not_ intended to solve any use-after-free,
it only solves the imbalance unlock. I never claim it solves more
anywhere.

Third of all, the use-after-free I can see (race with ->close) exists
before my patch, this patch doesn't make it better or worse, nor
I have any intend to fix it.

Re: [PATCH] infiniband: fix a subtle race condition

[Jason Gunthorpe]

On Thu, Jun 14, 2018 at 10:03:09AM -0700, Cong Wang wrote:
> On Thu, Jun 14, 2018 at 7:24 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
> >
> > This was my brief reaction too, this code path almost certainly has a
> > use-after-free, and we should fix the concurrency between the two
> > places in some correct way..
> 
> First of all, why use-after-free could trigger an imbalance unlock?
> IOW, why do we have to solve use-after-free to fix this imbalance
> unlock?

The issue syzkaller hit is that accessing ctx->file does not seem
locked in any way and can race with other manipulations of ctx->file.

So.. for this patch to be correct we need to understand how this
statement:

   f = ctx->file

Avoids f becoming a dangling pointer - and without locking, my
suspicion is that it doesn't - because missing locking around
ctx->file is probably the actual bug syzkaller found.

If this is not the case, then add a comment explaining how f's
lifetime is OK.

Otherwise, we need some kind of locking and guessing we need to hold a
kref for f?

> Third of all, the use-after-free I can see (race with ->close) exists
> before my patch, this patch doesn't make it better or worse, nor
> I have any intend to fix it.

I'm not sure that race exists, there should be something that flushes
the WQ on the path to close... (though I have another email that
perhaps that is broken, sigh)

Jason

Re: [PATCH] infiniband: fix a subtle race condition

[Cong Wang]

On Thu, Jun 14, 2018 at 10:24 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
> On Thu, Jun 14, 2018 at 10:03:09AM -0700, Cong Wang wrote:
>> On Thu, Jun 14, 2018 at 7:24 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
>> >
>> > This was my brief reaction too, this code path almost certainly has a
>> > use-after-free, and we should fix the concurrency between the two
>> > places in some correct way..
>>
>> First of all, why use-after-free could trigger an imbalance unlock?
>> IOW, why do we have to solve use-after-free to fix this imbalance
>> unlock?
>
> The issue syzkaller hit is that accessing ctx->file does not seem
> locked in any way and can race with other manipulations of ctx->file.
>
> So.. for this patch to be correct we need to understand how this
> statement:
>
>    f = ctx->file
>
> Avoids f becoming a dangling pointer - and without locking, my


It doesn't, because this is not the point, this is not the cause
of the unlock imbalance either. syzbot didn't report use-after-free
or a kernel segfault here.


> suspicion is that it doesn't - because missing locking around
> ctx->file is probably the actual bug syzkaller found.

Does my patch make it lockless or dangling? Apparently no.

Before my patch:

        mutex_lock(&ctx->file->mut);

After my patch:

        cur_file = ctx->file;
        mutex_lock(&cur_file->mut);

The deference is same as before, it was lockless and it is lockless
after my patch.

Look at the assembly code *without* my patch:

ffffffff819354f0:       49 8b 7c 24 78          mov    0x78(%r12),%rdi
ffffffff819354f5:       48 89 c3                mov    %rax,%rbx
ffffffff819354f8:       31 f6                   xor    %esi,%esi
ffffffff819354fa:       e8 d8 dd 40 00          callq
ffffffff81d432d7 <mutex_lock_nested>

Apparently the pointer is dereferenced before lock.

What difference does my patch make?

ffffffff819354f2:       4d 8b 74 24 78          mov    0x78(%r12),%r14
ffffffff819354f7:       48 89 c3                mov    %rax,%rbx
ffffffff819354fa:       31 f6                   xor    %esi,%esi
ffffffff819354fc:       4c 89 f7                mov    %r14,%rdi
ffffffff819354ff:       e8 9b df 40 00          callq
ffffffff81d4349f <mutex_lock_nested>
...
ffffffff8193567d:       4c 89 f7                mov    %r14,%rdi
ffffffff81935680:       e8 98 dd 40 00          callq
ffffffff81d4341d <mutex_unlock>


The %r14 here is the whole point of my patch.


>
> If this is not the case, then add a comment explaining how f's
> lifetime is OK.
>
> Otherwise, we need some kind of locking and guessing we need to hold a
> kref for f?


I agree with you, but again, this is not necessary for unlock
imbalance.


>
>> Third of all, the use-after-free I can see (race with ->close) exists
>> before my patch, this patch doesn't make it better or worse, nor
>> I have any intend to fix it.
>
> I'm not sure that race exists, there should be something that flushes
> the WQ on the path to close... (though I have another email that
> perhaps that is broken, sigh)
>

This is not related to my patch, but to convince you, let me explain:

struct ucma_file is not refcnt'ed, I know you cancel the work in
rdma_destroy_id(), but after ucma_migrate_id() the ctx has already
been moved to the new file, for the old file, it won't cancel the
ctx flying with workqueue. So, I think the following use-after-free
could happen:


ucma_event_handler():
cur_file = ctx->file; // old file

ucma_migrate_id():
lock();
list_move_tail(&ctx->list, &new_file->ctx_list);
ctx->file = new_file;
unlock();

ucma_close():
// retrieve old file via filp->private_data
// the loop won't cover the ctx moved to the new_file
kfree(file);

ucma_event_handler():
// continued from above
lock(&cur_file->mux); // already freed!


This is _not_ the cause of the unlock imbalance, and is _not_ expected
to solve by patch either.

Re: [PATCH] infiniband: fix a subtle race condition

[Jason Gunthorpe]

On Thu, Jun 14, 2018 at 04:14:13PM -0700, Cong Wang wrote:
> On Thu, Jun 14, 2018 at 10:24 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
> > On Thu, Jun 14, 2018 at 10:03:09AM -0700, Cong Wang wrote:
> >> On Thu, Jun 14, 2018 at 7:24 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
> >> >
> >> > This was my brief reaction too, this code path almost certainly has a
> >> > use-after-free, and we should fix the concurrency between the two
> >> > places in some correct way..
> >>
> >> First of all, why use-after-free could trigger an imbalance unlock?
> >> IOW, why do we have to solve use-after-free to fix this imbalance
> >> unlock?
> >
> > The issue syzkaller hit is that accessing ctx->file does not seem
> > locked in any way and can race with other manipulations of ctx->file.
> >
> > So.. for this patch to be correct we need to understand how this
> > statement:
> >
> >    f = ctx->file
> >
> > Avoids f becoming a dangling pointer - and without locking, my
> 
> It doesn't, because this is not the point, this is not the cause
> of the unlock imbalance either. syzbot didn't report use-after-free
> or a kernel segfault here.

No, it *is* the point - you've proposed a solution, one of many, and
we need to see an actual sensible design for how the locking around
ctx->file should work correctly.

We need solutions that solve the underlying problem, not just paper
over the symptoms.

Stated another way, for a syzkaller report like this there are a few
really obvious fixes.

1) Capture the lock pointer on the stack:
  f = ctx->file
  mutex_lock(&f->mut);
  mutex_unlock(&f->mut);

2) Prevent ctx->file from changing, eg add more locking:
  mutex_lock(&mut);
  mutex_lock(&ctx->file->mut);
  mutex_unlock(&ctx->file->mut));
  mutex_unlock(&mut);

3) Prevent ctx->file from being changing/freed by flushing the
   WQ at the right times:

   rdma_addr_cancel(...);
   ctx->file = XYZ;

This patch proposed #1. An explanation is required why that is a
correct locking design for this code. It sure looks like it isn't.

Looking at this *just a bit*, I wonder why not do something like
this:

  mutex_lock(&mut);
  f = ctx->file;
  mutex_lock(&f->mutex);
  mutex_unlock(&mut);
 
? At least that *might* make sense. Though probably it deadlocks as it
looks like we call rdma_addr_cancel() while holding mut. Yuk.

But maybe that sequence could be done before launching the work..

> > I'm not sure that race exists, there should be something that flushes
> > the WQ on the path to close... (though I have another email that
> > perhaps that is broken, sigh)
> 
> This is not related to my patch, but to convince you, let me explain:
> 
> struct ucma_file is not refcnt'ed, I know you cancel the work in
> rdma_destroy_id(), but after ucma_migrate_id() the ctx has already
> been moved to the new file, for the old file, it won't cancel the
> ctx flying with workqueue. So, I think the following use-after-free
> could happen:
> 
> ucma_event_handler():
> cur_file = ctx->file; // old file
> 
> ucma_migrate_id():
> lock();
> list_move_tail(&ctx->list, &new_file->ctx_list);
> ctx->file = new_file;
> unlock();
> 
> ucma_close():
> // retrieve old file via filp->private_data
> // the loop won't cover the ctx moved to the new_file
> kfree(file);

Yep. That sure seems like the right analysis!

> This is _not_ the cause of the unlock imbalance, and is _not_ expected
> to solve by patch either.

What do you mean? Not calling rdma_addr_cancel() prior to freeing the
file is *exactly* the cause of the lock imbalance.

The design of this code *assumes* that rdma_addr_cancel() will be
called before altering/freeing/etc any of the state it is working on,
migration makes a state change that violates that invariant.

Jason

Re: [PATCH] infiniband: fix a subtle race condition

[Cong Wang]

On Thu, Jun 14, 2018 at 7:57 PM, Jason Gunthorpe <jgg@mellanox.com> wrote:
> On Thu, Jun 14, 2018 at 04:14:13PM -0700, Cong Wang wrote:
>> On Thu, Jun 14, 2018 at 10:24 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
>> > On Thu, Jun 14, 2018 at 10:03:09AM -0700, Cong Wang wrote:
>> >> On Thu, Jun 14, 2018 at 7:24 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
>> >> >
>> >> > This was my brief reaction too, this code path almost certainly has a
>> >> > use-after-free, and we should fix the concurrency between the two
>> >> > places in some correct way..
>> >>
>> >> First of all, why use-after-free could trigger an imbalance unlock?
>> >> IOW, why do we have to solve use-after-free to fix this imbalance
>> >> unlock?
>> >
>> > The issue syzkaller hit is that accessing ctx->file does not seem
>> > locked in any way and can race with other manipulations of ctx->file.
>> >
>> > So.. for this patch to be correct we need to understand how this
>> > statement:
>> >
>> >    f = ctx->file
>> >
>> > Avoids f becoming a dangling pointer - and without locking, my
>>
>> It doesn't, because this is not the point, this is not the cause
>> of the unlock imbalance either. syzbot didn't report use-after-free
>> or a kernel segfault here.
>
> No, it *is* the point - you've proposed a solution, one of many, and
> we need to see an actual sensible design for how the locking around
> ctx->file should work correctly.

I proposed to a solution for imbalance unlock, you ask a design
for use-after-free, which is *irrelevant*. So why it is the point?


>
> We need solutions that solve the underlying problem, not just paper
> over the symptoms.


Sure, but your underlying problem exists before my patch, and it
is _not_ the cause of the imbalance unlock. Why does my patch
have an obligation to fix an irrelevant bug??

Since when we need to fix two bugs in one patch when it only aims
to fix one??

>
> Stated another way, for a syzkaller report like this there are a few
> really obvious fixes.
>
> 1) Capture the lock pointer on the stack:
>   f = ctx->file
>   mutex_lock(&f->mut);
>   mutex_unlock(&f->mut);

This is my patch.

>
> 2) Prevent ctx->file from changing, eg add more locking:
>   mutex_lock(&mut);
>   mutex_lock(&ctx->file->mut);
>   mutex_unlock(&ctx->file->mut));
>   mutex_unlock(&mut);


Obvious deadlock.


>
> 3) Prevent ctx->file from being changing/freed by flushing the
>    WQ at the right times:
>
>    rdma_addr_cancel(...);
>    ctx->file = XYZ;
>

Let's me REPEAT for a 3rd time: *Irrelevant*. Can you hear me?



> This patch proposed #1. An explanation is required why that is a
> correct locking design for this code. It sure looks like it isn't.


Ask yourself: does _my_ patch ever change any locking?
If you agree it is not, then you don't have any point to blame
locking on it.

I am _not_ against your redesign of locking, but you really have
to provide a separate patch for it, because it is a _different_ bug.


>
> Looking at this *just a bit*, I wonder why not do something like
> this:
>
>   mutex_lock(&mut);
>   f = ctx->file;
>   mutex_lock(&f->mutex);
>   mutex_unlock(&mut);
>
> ? At least that *might* make sense. Though probably it deadlocks as it
> looks like we call rdma_addr_cancel() while holding mut. Yuk.


Since you know it is deadlock, why even bring it up?


>
> But maybe that sequence could be done before launching the work..
>
>> > I'm not sure that race exists, there should be something that flushes
>> > the WQ on the path to close... (though I have another email that
>> > perhaps that is broken, sigh)
>>
>> This is not related to my patch, but to convince you, let me explain:
>>
>> struct ucma_file is not refcnt'ed, I know you cancel the work in
>> rdma_destroy_id(), but after ucma_migrate_id() the ctx has already
>> been moved to the new file, for the old file, it won't cancel the
>> ctx flying with workqueue. So, I think the following use-after-free
>> could happen:
>>
>> ucma_event_handler():
>> cur_file = ctx->file; // old file
>>
>> ucma_migrate_id():
>> lock();
>> list_move_tail(&ctx->list, &new_file->ctx_list);
>> ctx->file = new_file;
>> unlock();
>>
>> ucma_close():
>> // retrieve old file via filp->private_data
>> // the loop won't cover the ctx moved to the new_file
>> kfree(file);
>
> Yep. That sure seems like the right analysis!
>
>> This is _not_ the cause of the unlock imbalance, and is _not_ expected
>> to solve by patch either.
>
> What do you mean? Not calling rdma_addr_cancel() prior to freeing the
> file is *exactly* the cause of the lock imbalance.

Please do yourself a favor:

RUN THE REPRODUCER

See if you can still trigger imbalance with my patch which apparently
doesn't fix any use-after-free. You don't trust me, can you trust the
reproducer?


>
> The design of this code *assumes* that rdma_addr_cancel() will be
> called before altering/freeing/etc any of the state it is working on,
> migration makes a state change that violates that invariant.
>

I agree with you, but again, why do you think it is the cause
of imbalance unlock?

Re: [PATCH] infiniband: fix a subtle race condition

[Jason Gunthorpe]

On Fri, Jun 15, 2018 at 10:57:49AM -0700, Cong Wang wrote:

> > No, it *is* the point - you've proposed a solution, one of many, and
> > we need to see an actual sensible design for how the locking around
> > ctx->file should work correctly.
> 
> I proposed to a solution for imbalance unlock, you ask a design
> for use-after-free, which is *irrelevant*. So why it is the point?

The point is, I don't care about the imbalance report.

I care about the actual bug, which you have identified as
ucma_migrate_id() running concurrently with ucma_event_handler(). That
seems like a great analysis, BTW.

Stop that from happening and the lock imbalance warning will naturally go
away. So will the use after free.

I gave you some general ideas on how to do that, obviously they are
not easy to do eg somehow solving the dealock with mut would be
tricky. But maybe there is still some kind of simple solution..

Another option might be to just fail ucma_migrate_id() when
ucma_event_handler() is outstanding.. That *might* be OK.. We've
talked about doing things like this for other ucma syzkaller
bugs. Also a bit complicated.

Anyhow I'm NAK'ing this patch, since it just doesn't move things
forward, and removes a warning that is pointing at a bunch of
different bugs.

Jason

Re: [PATCH] infiniband: fix a subtle race condition

[Cong Wang]

On Fri, Jun 15, 2018 at 12:08 PM, Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> The point is, I don't care about the imbalance report.

OK, we are not on the same page.

Sorry for wasting my time.

Re: [PATCH] infiniband: fix a subtle race condition

[Cong Wang]

On Thu, Jun 14, 2018 at 12:01 AM, Leon Romanovsky <leon@kernel.org> wrote:
> On Wed, Jun 13, 2018 at 11:21:54PM -0700, Cong Wang wrote:
>> On Wed, Jun 13, 2018 at 10:34 PM, Leon Romanovsky <leon@kernel.org> wrote:
>> >
>> > Hi Cong,
>> >
>> > If the compiler optimizes the first line (mutex_lock) as you wrote,
>> > it will reuse "f" for the second line (mutex_unlock) too.
>>
>> Nope, check the assembly if you don't trust me, at least
>> my compiler always fetches ctx->file without this patch.
>>
>> I can show you the assembly code tomorrow (too late to
>> access my dev machine now).
>
> I trust you, so don't need to check it however wanted to emphasize
> that your solution is compiler specific and not universally true.

So are you saying even with my patch compiler could still re-fetch
ctx->file? I doubt...


>
>>
>>
>> >
>> > You need to ensure that ucma_modify_id() doesn't run in parallel to
>> > anything that uses "ctx->file" directly and indirectly.
>> >
>>
>> Talk is easy, show me the code. :) I knew there is probably
>> some other race with this code even after my patch, possibly with
>> ->close() for example, but for this specific unlock warning, this patch
>> is sufficient. I can't solve all the races in one patch.
>
> We do prefer complete solution once the problem is fully understood.
>

The unlock imbalance problem is fully understood and is clearly shown
in my changelog.

My patch never intends to solve any other problem except this one.


> It looks like you are one step away from final patch. It will be conversion
> of mutex to be rwlock and separating between read (almost in all places)
> and write (in ucma_migrate_id) paths.
>

Excuse me. How does this even solve the imbalance problem?

f = ctx->file;
                        ucma_lock_files(f, new_file); // write sem
                        ctx->file = new_file
                        ucma_lock_files(f, new_file); // write sem
down_read(&f->rw); // still the old file, nothing change
f = ctx->file; // new file
up_read(&f->rw); // still imbalance