Re: [PATCH] infiniband: fix a subtle race condition
On Fri, Jun 15, 2018 at 12:08 PM, Jason Gunthorpe 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
On Fri, Jun 15, 2018 at 12:08 PM, Jason Gunthorpe 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
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
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
On Thu, Jun 14, 2018 at 7:57 PM, Jason Gunthorpe 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 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 >> >> 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(>mut); > mutex_unlock(>mut); This is my patch. > > 2) Prevent ctx->file from changing, eg add more locking: > mutex_lock(); > mutex_lock(>file->mut); > mutex_unlock(>file->mut)); > mutex_unlock(); 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(); > f = ctx->file; > mutex_lock(>mutex); > mutex_unlock(); > > ? 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(>list, _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
On Thu, Jun 14, 2018 at 7:57 PM, Jason Gunthorpe 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 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 >> >> 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(>mut); > mutex_unlock(>mut); This is my patch. > > 2) Prevent ctx->file from changing, eg add more locking: > mutex_lock(); > mutex_lock(>file->mut); > mutex_unlock(>file->mut)); > mutex_unlock(); 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(); > f = ctx->file; > mutex_lock(>mutex); > mutex_unlock(); > > ? 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(>list, _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
On Thu, Jun 14, 2018 at 04:14:13PM -0700, Cong Wang wrote: > On Thu, Jun 14, 2018 at 10:24 AM, Jason Gunthorpe 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 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(>mut); mutex_unlock(>mut); 2) Prevent ctx->file from changing, eg add more locking: mutex_lock(); mutex_lock(>file->mut); mutex_unlock(>file->mut)); mutex_unlock(); 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(); f = ctx->file; mutex_lock(>mutex); mutex_unlock(); ? 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(>list, _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
On Thu, Jun 14, 2018 at 04:14:13PM -0700, Cong Wang wrote: > On Thu, Jun 14, 2018 at 10:24 AM, Jason Gunthorpe 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 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(>mut); mutex_unlock(>mut); 2) Prevent ctx->file from changing, eg add more locking: mutex_lock(); mutex_lock(>file->mut); mutex_unlock(>file->mut)); mutex_unlock(); 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(); f = ctx->file; mutex_lock(>mutex); mutex_unlock(); ? 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(>list, _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
On Thu, Jun 14, 2018 at 10:24 AM, Jason Gunthorpe 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 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(>file->mut); After my patch: cur_file = ctx->file; mutex_lock(_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: 819354f0: 49 8b 7c 24 78 mov0x78(%r12),%rdi 819354f5: 48 89 c3mov%rax,%rbx 819354f8: 31 f6 xor%esi,%esi 819354fa: e8 d8 dd 40 00 callq 81d432d7 Apparently the pointer is dereferenced before lock. What difference does my patch make? 819354f2: 4d 8b 74 24 78 mov0x78(%r12),%r14 819354f7: 48 89 c3mov%rax,%rbx 819354fa: 31 f6 xor%esi,%esi 819354fc: 4c 89 f7mov%r14,%rdi 819354ff: e8 9b df 40 00 callq 81d4349f ... 8193567d: 4c 89 f7mov%r14,%rdi 81935680: e8 98 dd 40 00 callq 81d4341d 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(>list, _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(_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
On Thu, Jun 14, 2018 at 10:24 AM, Jason Gunthorpe 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 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(>file->mut); After my patch: cur_file = ctx->file; mutex_lock(_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: 819354f0: 49 8b 7c 24 78 mov0x78(%r12),%rdi 819354f5: 48 89 c3mov%rax,%rbx 819354f8: 31 f6 xor%esi,%esi 819354fa: e8 d8 dd 40 00 callq 81d432d7 Apparently the pointer is dereferenced before lock. What difference does my patch make? 819354f2: 4d 8b 74 24 78 mov0x78(%r12),%r14 819354f7: 48 89 c3mov%rax,%rbx 819354fa: 31 f6 xor%esi,%esi 819354fc: 4c 89 f7mov%r14,%rdi 819354ff: e8 9b df 40 00 callq 81d4349f ... 8193567d: 4c 89 f7mov%r14,%rdi 81935680: e8 98 dd 40 00 callq 81d4341d 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(>list, _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(_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
On Thu, Jun 14, 2018 at 10:03:09AM -0700, Cong Wang wrote: > On Thu, Jun 14, 2018 at 7:24 AM, Jason Gunthorpe 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
On Thu, Jun 14, 2018 at 10:03:09AM -0700, Cong Wang wrote: > On Thu, Jun 14, 2018 at 7:24 AM, Jason Gunthorpe 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
On Thu, Jun 14, 2018 at 7:24 AM, Jason Gunthorpe 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
On Thu, Jun 14, 2018 at 7:24 AM, Jason Gunthorpe 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
On Thu, Jun 14, 2018 at 12:01 AM, 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 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(>rw); // still the old file, nothing change f = ctx->file; // new file up_read(>rw); // still imbalance
Re: [PATCH] infiniband: fix a subtle race condition
On Thu, Jun 14, 2018 at 12:01 AM, 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 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(>rw); // still the old file, nothing change f = ctx->file; // new file up_read(>rw); // still imbalance
Re: [PATCH] infiniband: fix a subtle race condition
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 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
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 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
On Wed, Jun 13, 2018 at 11:21:54PM -0700, Cong Wang wrote: > On Wed, Jun 13, 2018 at 10:34 PM, Leon Romanovsky 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 signature.asc Description: PGP signature
Re: [PATCH] infiniband: fix a subtle race condition
On Wed, Jun 13, 2018 at 11:21:54PM -0700, Cong Wang wrote: > On Wed, Jun 13, 2018 at 10:34 PM, Leon Romanovsky 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 signature.asc Description: PGP signature
Re: [PATCH] infiniband: fix a subtle race condition
On Wed, Jun 13, 2018 at 10:34 PM, Leon Romanovsky 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
On Wed, Jun 13, 2018 at 10:34 PM, Leon Romanovsky 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
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(>file->mut); > ... > mutex_unlock(>file->mut); > > which seems correct, but we could translate it into this: > > f = ctx->file; > mutex_lock(>mut); > ... > f = ctx->file; > mutex_unlock(>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(>mut); // still the old file! > ... > f = ctx->file; // now the new one!! > mutex_unlock(>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 signature.asc Description: PGP signature
Re: [PATCH] infiniband: fix a subtle race condition
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(>file->mut); > ... > mutex_unlock(>file->mut); > > which seems correct, but we could translate it into this: > > f = ctx->file; > mutex_lock(>mut); > ... > f = ctx->file; > mutex_unlock(>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(>mut); // still the old file! > ... > f = ctx->file; // now the new one!! > mutex_unlock(>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 signature.asc Description: PGP signature
