| Message ID | 1431515438-24042-2-git-send-email-yishaih@mellanox.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, May 13, 2015 at 02:10:36PM +0300, Yishai Hadas wrote: > + struct srcu_struct disassociate_srcu; There is no need for rcu for this, use a rw sem. > @@ -1326,6 +1327,13 @@ ssize_t ib_uverbs_create_comp_channel(struct ib_uverbs_file *file, > return -EFAULT; > } > > + /* Taking ref count on uverbs_file to make sure that file won't be > + * freed till that event file is closed. It will enable accessing the > + * uverbs_device fields as part of closing the events file and making > + * sure that uverbs device is available by that time as well. > + * Note: similar is already done for the async event file. > + */ > + kref_get(&file->ref); Is this a bug today? It doesn't look like it, but this stuff does look wrong. Woulnd't this would make more sense for ib_uverbs_alloc_event_file to unconditionally grab the kref and unconditionally release it on release? The existing code for this looks broken, in ib_uverbs_get_context all the error paths between ib_uverbs_alloc_event_file and the kref_get(file->ref) are wrong - the will result in fput() which will call ib_uverbs_event_close, which will try to do kref_put and ib_unregister_event_handler - which are no longer paired. [I recommend moving the kref_get and ib_register_event_handler into ib_uverbs_alloc_event_file, so the 'create' and 'destroy' code paths are clearly paired instead of being partially open coded in call sites] Fix all this in a seperate patch to add the needed change in kref semantics please. > - if (!try_module_get(dev->ib_dev->owner)) { > - ret = -ENODEV; > + mutex_lock(&dev->disassociate_mutex); > + if (dev->disassociated) { > + ret = -EIO; > goto err; > } > > - file = kmalloc(sizeof *file, GFP_KERNEL); > + /* In case IB device supports disassociate ucontext, there is no hard > + * dependency between uverbs device and its low level device. > + */ > + module_dependent = !(dev->flags & UVERBS_FLAG_DISASSOCIATE); > + > + if (module_dependent) { > + if (!try_module_get(dev->ib_dev->owner)) { > + ret = -ENODEV; > + goto err; Again? Why I do I keep pointing this same basic thing to Mellanox people: If you hold a X then you hold the ref to X as well. So, if the core code is holding function pointers to module code, then the core code holds a module ref. When the core code null's those function pointers, then it can release the module ref. This might work today like this (I'm not entirely sure), but it makes no sense at all. I'll look more closely in a few weeks once the rwsem change is done. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-05-25 at 10:54 -0600, Jason Gunthorpe wrote: > On Wed, May 13, 2015 at 02:10:36PM +0300, Yishai Hadas wrote: > > > + struct srcu_struct disassociate_srcu; > > There is no need for rcu for this, use a rw sem. The rcu was used becuase it's on the hot path I assume. Do we have numbers on whether a rwsem vs. an rcu matters performance wise? If the rcu actually helps performance, then I'm inclined to leave it, but if it doesn't, then I'd agree that a rwsem is simpler and easier to deal with. > > @@ -1326,6 +1327,13 @@ ssize_t ib_uverbs_create_comp_channel(struct ib_uverbs_file *file, > > return -EFAULT; > > } > > > > + /* Taking ref count on uverbs_file to make sure that file won't be > > + * freed till that event file is closed. It will enable accessing the > > + * uverbs_device fields as part of closing the events file and making > > + * sure that uverbs device is available by that time as well. > > + * Note: similar is already done for the async event file. > > + */ > > + kref_get(&file->ref); > > Is this a bug today? It doesn't look like it, but this stuff does look wrong. > > Woulnd't this would make more sense for ib_uverbs_alloc_event_file to > unconditionally grab the kref and unconditionally release it on > release? > > The existing code for this looks broken, in ib_uverbs_get_context all > the error paths between ib_uverbs_alloc_event_file and the > kref_get(file->ref) are wrong - the will result in fput() which will > call ib_uverbs_event_close, which will try to do kref_put and > ib_unregister_event_handler - which are no longer paired. > > [I recommend moving the kref_get and ib_register_event_handler into > ib_uverbs_alloc_event_file, so the 'create' and 'destroy' code paths > are clearly paired instead of being partially open coded in call > sites] > > Fix all this in a seperate patch to add the needed change in kref > semantics please. Seconded. > > - if (!try_module_get(dev->ib_dev->owner)) { > > - ret = -ENODEV; > > + mutex_lock(&dev->disassociate_mutex); > > + if (dev->disassociated) { > > + ret = -EIO; > > goto err; > > } > > > > - file = kmalloc(sizeof *file, GFP_KERNEL); > > + /* In case IB device supports disassociate ucontext, there is no hard > > + * dependency between uverbs device and its low level device. > > + */ > > + module_dependent = !(dev->flags & UVERBS_FLAG_DISASSOCIATE); > > + > > + if (module_dependent) { > > + if (!try_module_get(dev->ib_dev->owner)) { > > + ret = -ENODEV; > > + goto err; > > Again? Why I do I keep pointing this same basic thing to Mellanox > people: > > If you hold a X then you hold the ref to X as well. > > So, if the core code is holding function pointers to module code, then > the core code holds a module ref. When the core code null's those > function pointers, then it can release the module ref. Seconded. > This might work today like this (I'm not entirely sure), but it makes > no sense at all. > > I'll look more closely in a few weeks once the rwsem change is done.
On Wed, May 27, 2015 at 12:04:29PM -0400, Doug Ledford wrote: > On Mon, 2015-05-25 at 10:54 -0600, Jason Gunthorpe wrote: > > On Wed, May 13, 2015 at 02:10:36PM +0300, Yishai Hadas wrote: > > > > > + struct srcu_struct disassociate_srcu; > > > > There is no need for rcu for this, use a rw sem. > > The rcu was used becuase it's on the hot path I assume. Perhaps, I looked at that a bit, it was used on syscall paths, but that wasn't even the big reason I made the comment.. The use of RCU is *strange*. There is no pointer, there is no call to rcu_derference. Stuff protected by the write lock is being touched without any read side locking (ie ib_uverbs_event_read sure looks funky). There is no Copy or Update phase as far as I can see. It fails two of the tests in Documentation/RCU/checklist.txt. I'm pretty sure what is intended here is that disassociated is actually an unlocked atomic boolean and the RCU is being used (abused?) to create both exclusion and a rendezvous.. I gave up at that point, if RCU is not being used properly, asking for rwsem is a way to force the author to deal with their locking sanely. Jasno -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/27/2015 7:04 PM, Doug Ledford wrote: > On Mon, 2015-05-25 at 10:54 -0600, Jason Gunthorpe wrote: >> On Wed, May 13, 2015 at 02:10:36PM +0300, Yishai Hadas wrote: >> >>> + struct srcu_struct disassociate_srcu; >> >> There is no need for rcu for this, use a rw sem. > > The rcu was used becuase it's on the hot path I assume. Do we have > numbers on whether a rwsem vs. an rcu matters performance wise? If the > rcu actually helps performance, then I'm inclined to leave it, but if it > doesn't, then I'd agree that a rwsem is simpler and easier to deal with. > That's correct, it was chosen from performance reasons to enable parallel commands as part of ib_uverbs_write with minimum synchronization overhead comparing the rwsem. Most of its usage is for read, upon low level device removal there is only one point when synchronize_srcu is used. >>> @@ -1326,6 +1327,13 @@ ssize_t ib_uverbs_create_comp_channel(struct ib_uverbs_file *file, >>> return -EFAULT; >>> } >>> >>> + /* Taking ref count on uverbs_file to make sure that file won't be >>> + * freed till that event file is closed. It will enable accessing the >>> + * uverbs_device fields as part of closing the events file and making >>> + * sure that uverbs device is available by that time as well. >>> + * Note: similar is already done for the async event file. >>> + */ >>> + kref_get(&file->ref); >> >> Is this a bug today? It doesn't look like it, but this stuff does look wrong. >> >> Woulnd't this would make more sense for ib_uverbs_alloc_event_file to >> unconditionally grab the kref and unconditionally release it on >> release? >> >> The existing code for this looks broken, in ib_uverbs_get_context all >> the error paths between ib_uverbs_alloc_event_file and the >> kref_get(file->ref) are wrong - the will result in fput() which will >> call ib_uverbs_event_close, which will try to do kref_put and >> ib_unregister_event_handler - which are no longer paired. >> >> [I recommend moving the kref_get and ib_register_event_handler into >> ib_uverbs_alloc_event_file, so the 'create' and 'destroy' code paths >> are clearly paired instead of being partially open coded in call >> sites] >> >> Fix all this in a seperate patch to add the needed change in kref >> semantics please. > > Seconded. OK, will add a separate patch to handle that. > >>> - if (!try_module_get(dev->ib_dev->owner)) { >>> - ret = -ENODEV; >>> + mutex_lock(&dev->disassociate_mutex); >>> + if (dev->disassociated) { >>> + ret = -EIO; >>> goto err; >>> } >>> >>> - file = kmalloc(sizeof *file, GFP_KERNEL); >>> + /* In case IB device supports disassociate ucontext, there is no hard >>> + * dependency between uverbs device and its low level device. >>> + */ >>> + module_dependent = !(dev->flags & UVERBS_FLAG_DISASSOCIATE); >>> + >>> + if (module_dependent) { >>> + if (!try_module_get(dev->ib_dev->owner)) { >>> + ret = -ENODEV; >>> + goto err; >> >> Again? Why I do I keep pointing this same basic thing to Mellanox >> people: >> >> If you hold a X then you hold the ref to X as well. >> >> So, if the core code is holding function pointers to module code, then >> the core code holds a module ref. When the core code null's those >> function pointers, then it can release the module ref. > > Seconded. The module get/put mechanism was previously used here and wasn't introduced by that patch. In case the low level driver can be unloaded (e.g. rmmod mlx4_ib) unconditionally to active uverbs clients we should prevent the module dependency by ignoring the get/put mechanism. Hope that it clarifies the usage here. > >> This might work today like this (I'm not entirely sure), but it makes >> no sense at all. >> >> I'll look more closely in a few weeks once the rwsem change is done. > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 27, 2015 at 8:43 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Wed, May 27, 2015 at 12:04:29PM -0400, Doug Ledford wrote: >> On Mon, 2015-05-25 at 10:54 -0600, Jason Gunthorpe wrote: >> > On Wed, May 13, 2015 at 02:10:36PM +0300, Yishai Hadas wrote: >> > >> > > + struct srcu_struct disassociate_srcu; >> > >> > There is no need for rcu for this, use a rw sem. >> >> The rcu was used becuase it's on the hot path I assume. > > Perhaps, I looked at that a bit, it was used on syscall paths, but > that wasn't even the big reason I made the comment.. Doug, what hot path we have in uverbs?! IB's stack hot path goes from user-space to the HW, right? Or. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-05-28 at 00:36 +0300, Or Gerlitz wrote: > On Wed, May 27, 2015 at 8:43 PM, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com> wrote: > > On Wed, May 27, 2015 at 12:04:29PM -0400, Doug Ledford wrote: > >> On Mon, 2015-05-25 at 10:54 -0600, Jason Gunthorpe wrote: > >> > On Wed, May 13, 2015 at 02:10:36PM +0300, Yishai Hadas wrote: > >> > > >> > > + struct srcu_struct disassociate_srcu; > >> > > >> > There is no need for rcu for this, use a rw sem. > >> > >> The rcu was used becuase it's on the hot path I assume. > > > > Perhaps, I looked at that a bit, it was used on syscall paths, but > > that wasn't even the big reason I made the comment.. > > Doug, what hot path we have in uverbs?! IB's stack hot path goes from > user-space to the HW, right? > > Or. For lots of stuff, yes. However, the function that they picked this for is ib_uverbs_write() and for certain things, that is a hot path. Examples would be things like the cmtime test program in librdmacm, and let's not forget that the whole reason that program was written was because we were chasing an issue that caused real world applications to fall over when they couldn't handle roughly 900 reverse route resolutions per second, so I consider the cmtime utility, and the stack's responsiveness to it, a valid item to optimize for.
On Wed, May 27, 2015 at 11:47:31PM +0300, Yishai Hadas wrote: > That's correct, it was chosen from performance reasons to enable parallel > commands as part of ib_uverbs_write with minimum synchronization overhead > comparing the rwsem. The locking scheme makes no sense, see my other email, design it for a rwsem and then consider rcu if necessary. Hint: Get rid of dev->disassociated and use ib_dev == null to accomplish the same thing. Now it is clear that ib_dev is what must be protected by a rwlock and the locking scheme will flow sanely from that point. If you must use RCU for performance then it is now clear that rcu_dereference must be applied to the ib_dev. Then the other locking can stop being so subtle: ----- ib_uverbs_close: down(lists_mutex) tmp = file->ucontext; if (tmp) { list_del(&file->list); file->ucontext = NULL; } up(lists_mutex); if (tmp) ib_uverbs_cleanup_ucontext(file, tmp); ----- ib_uverbs_free_hw_resources: down(lists_mutex) while (!lists_empty(&uverbs_dev->uverbs_file_list) { file = list_first_entry(&uverbs_dev->uverbs_file_list, list) tmp = file->ucontext; list_del(&file->list); file->ucontext = NULL; kref_get(&file->ref); up(list_mutex); ib_uverbs_cleanup_ucontext(file, tmp); down(list_mutex); kref_put(&file->ref,ib_uverbs_release_file); } up(list_mutex); Now the krefs are unquestionably paired, we don't leave a dangling ucontext pointer, we don't leave a dangling list entry, locking of the list is explicit and doesn't rely on an unlocked access with an implicit exclusion. Basically, it is actually obvious what each lock is protecting. > In case the low level driver can be unloaded (e.g. rmmod mlx4_ib) > unconditionally to active uverbs clients we should prevent the module > dependency by ignoring the get/put mechanism. Hope that it clarifies the > usage here. Okay, that does make sense. But I do suspect try_module_get should still be run, just immediately released. Otherwise the check for in-progress module removal is skipped. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] > Sent: Thursday, May 28, 2015 9:19 AM > Subject: Re: [PATCH V3 for-next 1/3] IB/uverbs: Enable device removal > when there are active user space applications > > On Wed, May 27, 2015 at 11:47:31PM +0300, Yishai Hadas wrote: > > That's correct, it was chosen from performance reasons to enable > parallel > > commands as part of ib_uverbs_write with minimum synchronization > overhead > > comparing the rwsem. > > The locking scheme makes no sense, see my other email, design it for a > rwsem and then consider rcu if necessary. > Jason, the RCU based scheme is important, as it allows the performance critical path (verbs such as ibv_reg_mr) to avoid cross core dependencies. > Hint: Get rid of dev->disassociated and use ib_dev == null to Initially, I liked this idea, as it makes it easier to prove the RCU correctness. However, it makes life really complicated: - We will need to latch ib_dev in uverbs_cmd, and handle possible NULL value. There are 17 uses of ib_dev in uverbs_cmd. This makes the patch even bigger. - To make life even more complicated, a large number of objects (i.e. PD, MR, QP, XRCD, etc.) are keeping a pointer to the same ib_dev struct. Making ib_dev NULL will not magically make these pointer NULL as well. As a result, attempting to RCU annotating all affected users will be impossible. - The release flows for the relevant uverbs object might be accessing the ib_dev pointer from time to time. Setting ib_dev to NULL before calling the relevant ib_verbs_cleanup_context seems like the recipe for a kernel OOPS for NULL pointer deref. > accomplish the same thing. Now it is clear that ib_dev is what must be > protected by a rwlock and the locking scheme will flow sanely from > that point. If you must use RCU for performance then it is now clear > that rcu_dereference must be applied to the ib_dev. See above, it will make the patch ~4x bigger, without performing any actual annotation value. > > Then the other locking can stop being so subtle: > Locks are subtle creatures by nature. I will explain why your scheme will break below. > ----- > ib_uverbs_close: > This is give or take the same as what is in the current code. > down(lists_mutex) This name is better than the current "disassociate_mutex" name we have for this mutex, will fix in next version. > tmp = file->ucontext; > if (tmp) { > list_del(&file->list); > file->ucontext = NULL; We avoid setting the file->ucontext to NULL early, to avoid a spurious NULL pointer dereferences. However, if you think that this is what will fix the readability, we can set it to NULL and avoid touching the pointer after this point. > } > up(lists_mutex); > > if (tmp) > ib_uverbs_cleanup_ucontext(file, tmp); > > ----- > ib_uverbs_free_hw_resources: > Here is where subtle locking is getting out of the bag. Your skeleton flow only handles the main uverbs file. However, this will get the user applications stuck. If the application is waiting for an event, we should raise a fatal "device disassociated" event to release the application. Doing so after you started NULLifying pointers and removing elements from lists seems the wrong way to do it, a way which can lead to large amount of oopsing and deadlocking later on. > down(lists_mutex) > while (!lists_empty(&uverbs_dev->uverbs_file_list) { > file = list_first_entry(&uverbs_dev->uverbs_file_list, list) > tmp = file->ucontext; > list_del(&file->list); This is where a bug will hit you - if ib_uverbs_close also unconditionally does list_del from file->list, you have a nice big race here. > file->ucontext = NULL; > kref_get(&file->ref); The current code takes reference to all files with a single locking, and after that releases them all without holding a lock. Your suggestion is busy bouncing the lock back and forth. To make it more interesting, if we try to use SRCU with your suggestion, we will need to sync the SRCU every time we drop the lock. Syncing the SRCU is really long operation (seconds). Doing it once during the device removal is OK. Doing it for every FD is not practical. > up(list_mutex); > > ib_uverbs_cleanup_ucontext(file, tmp); > > down(list_mutex); > kref_put(&file->ref,ib_uverbs_release_file); > } > up(list_mutex); > > Now the krefs are unquestionably paired, we don't leave a dangling > ucontext pointer, we don't leave a dangling list entry, locking of the > list is explicit and doesn't rely on an unlocked access with an > implicit exclusion. We don't leave a dangling list entry in our code either. The list locking in our scheme, while slightly less bold, is much more efficient, especially for the hot path of the IB verb operations. > Basically, it is actually obvious what each lock > is protecting. > > > In case the low level driver can be unloaded (e.g. rmmod mlx4_ib) > > unconditionally to active uverbs clients we should prevent the module > > dependency by ignoring the get/put mechanism. Hope that it clarifies > the > > usage here. > > Okay, that does make sense. But I do suspect try_module_get should still > be run, just immediately released. Otherwise the check for in-progress > module removal is skipped. > If the module is in the process of being removed, it will call the ib_uverbs_remove_one. This calls our disassociate code, and is synchronize using the list_mutex. This will prevent such issues, without dirtying another cache line in the good case. Thanks, --Shachar -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 29, 2015 at 10:30:52AM +0000, Shachar Raindel wrote: > > The locking scheme makes no sense, see my other email, design it for a > > rwsem and then consider rcu if necessary. > > Jason, the RCU based scheme is important, as it allows the performance > critical path (verbs such as ibv_reg_mr) to avoid cross core dependencies. The difference between RCU and rwsem is at worst a cache line bounce if simultaneously stressed by different readers on different CPUs (at best it might be faster). Can reg_mr even run concurrently on multiple cpus? Right now I want to see a locking scheme that makes sense, isn't so damn subtle and follows the RCU usage guidelines. > > Hint: Get rid of dev->disassociated and use ib_dev == null to > > Initially, I liked this idea, as it makes it easier to prove the RCU > correctness. However, it makes life really complicated: It doesn't make it 'easier to prove RCU correctness' it is 'the correct way to use RCU'. > - We will need to latch ib_dev in uverbs_cmd, and handle possible NULL > value. There are 17 uses of ib_dev in uverbs_cmd. This makes the patch > even bigger. rwsem avoids that, of course. But even so, it is a pretty small change to have ib_uverbs_write grab the ib_dev and ucontext under rcu and then pass them as function arguments to the callback. > - To make life even more complicated, a large number of objects (i.e. PD, > MR, QP, XRCD, etc.) are keeping a pointer to the same ib_dev > struct. > Making ib_dev NULL will not magically make these pointer NULL as well. > As a result, attempting to RCU annotating all affected users will be > impossible. This point makes no sense. I don't think you understand RCU. Only the member in the ib_uverbs_device device needs to be read while holding rcu. Once ib_dev is copied and kref incr'd into the PD it is outside of uverbs concern, and no longer covered by the RCU. > - The release flows for the relevant uverbs object might be accessing the > ib_dev pointer from time to time. Setting ib_dev to NULL before calling > the relevant ib_verbs_cleanup_context seems like the recipe for a kernel > OOPS for NULL pointer deref. Are you just guessing at this? Did you check them? I briefly did, and again just now. Looks OK. > > ib_uverbs_close: > > This is give or take the same as what is in the current code. It looks similar but works very differently. > > down(lists_mutex) > > This name is better than the current "disassociate_mutex" name we have for > this mutex, will fix in next version. > > > tmp = file->ucontext; > > if (tmp) { > > list_del(&file->list); > > file->ucontext = NULL; > > We avoid setting the file->ucontext to NULL early, to avoid a spurious > NULL pointer dereferences. Again, this comment makes no sense. You are concerned about a NULL dereference when this code is freeing the structure? No concern about use-after-free? Are you just guessing there might be a problem? Did you study this scenario? > However, if you think that this is what will fix the readability, we can > set it to NULL and avoid touching the pointer after this point. This has nothing to do with readability, it is mandatory that the null set be possible. > > } > > up(lists_mutex); > > > > if (tmp) > > ib_uverbs_cleanup_ucontext(file, tmp); > > > > ib_uverbs_free_hw_resources: > > > > Here is where subtle locking is getting out of the bag. > Your skeleton flow only handles the main uverbs file. > However, this will get the user applications stuck. If > the application is waiting for an event, we should raise > a fatal "device disassociated" event to release the application. I'm not going to code this for you, I just showed the basic idea. You need to extrapolate that to the other cases. > Doing so after you started NULLifying pointers and removing > elements from lists seems the wrong way to do it, a way which > can lead to large amount of oopsing and deadlocking later on. You should actually look at how event delivery works. > > down(lists_mutex) > > while (!lists_empty(&uverbs_dev->uverbs_file_list) { > > file = list_first_entry(&uverbs_dev->uverbs_file_list, list) > > tmp = file->ucontext; > > list_del(&file->list); > > This is where a bug will hit you - if ib_uverbs_close also > unconditionally does list_del from file->list, you have > a nice big race here. And #3 'this comment makes no sense' - seriously? down(lists_mutex) tmp = file->ucontext; if (tmp) { list_del(&file->list); file->ucontext = NULL; } up(lists_mutex); Does that look unconditional/unlocked to you? Look, this is wasting my time. If you are going to answer a detailed email then you had better study the subject matter deeply. > The current code takes reference to all files with a single > locking, and after that releases them all without holding a > lock. Your suggestion is busy bouncing the lock back and forth. This is removal, so any cost we pay is completely irrelevant. Be Clear. Be Clean. Be Correct. > To make it more interesting, if we try to use SRCU with your > suggestion, we will need to sync the SRCU every time we drop > the lock. Syncing the SRCU is really long operation (seconds). > Doing it once during the device removal is OK. Doing it for every > FD is not practical. You really don't understand RCU if you think that loop needs to synchronize_rcu. > > Now the krefs are unquestionably paired, we don't leave a dangling > > ucontext pointer, we don't leave a dangling list entry, locking of the > > list is explicit and doesn't rely on an unlocked access with an > > implicit exclusion. > > We don't leave a dangling list entry in our code either. I see two missing list_del's at least. > If the module is in the process of being removed, it will call the > ib_uverbs_remove_one. This calls our disassociate code, and is synchronize > using the list_mutex. This will prevent such issues, without dirtying > another cache line in the good case. You need to stop micro optimizing things that don't need to be micro optimized. It doesn't hurt things to follow the normal pattern and release the lock early. I feel like looking at this patch has been a waste of my time. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h index b716b08..4ef6b1c 100644 --- a/drivers/infiniband/core/uverbs.h +++ b/drivers/infiniband/core/uverbs.h @@ -63,6 +63,10 @@ (udata)->outlen = (olen); \ } while (0) +enum uverbs_flags { + UVERBS_FLAG_DISASSOCIATE = 1 +}; + /* * Our lifetime rules for these structs are the following: * @@ -94,6 +98,12 @@ struct ib_uverbs_device { struct cdev cdev; struct rb_root xrcd_tree; struct mutex xrcd_tree_mutex; + struct mutex disassociate_mutex; /* protect lists of files. */ + int disassociated; + u32 flags; + struct srcu_struct disassociate_srcu; + struct list_head uverbs_file_list; + struct list_head uverbs_events_file_list; }; struct ib_uverbs_event_file { @@ -105,6 +115,7 @@ struct ib_uverbs_event_file { wait_queue_head_t poll_wait; struct fasync_struct *async_queue; struct list_head event_list; + struct list_head list; }; struct ib_uverbs_file { @@ -114,6 +125,7 @@ struct ib_uverbs_file { struct ib_ucontext *ucontext; struct ib_event_handler event_handler; struct ib_uverbs_event_file *async_file; + struct list_head list; }; struct ib_uverbs_event { diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index a9f0489..4b6a9e6 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -39,6 +39,7 @@ #include <linux/sched.h> #include <asm/uaccess.h> +#include <linux/sched.h> #include "uverbs.h" #include "core_priv.h" @@ -1326,6 +1327,13 @@ ssize_t ib_uverbs_create_comp_channel(struct ib_uverbs_file *file, return -EFAULT; } + /* Taking ref count on uverbs_file to make sure that file won't be + * freed till that event file is closed. It will enable accessing the + * uverbs_device fields as part of closing the events file and making + * sure that uverbs device is available by that time as well. + * Note: similar is already done for the async event file. + */ + kref_get(&file->ref); fd_install(resp.fd, filp); return in_len; } diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index 88cce9b..d96d7c7 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -134,7 +134,12 @@ static void ib_uverbs_release_dev(struct kref *ref) struct ib_uverbs_device *dev = container_of(ref, struct ib_uverbs_device, ref); - complete(&dev->comp); + if (dev->disassociated) { + cleanup_srcu_struct(&dev->disassociate_srcu); + kfree(dev); + } else { + complete(&dev->comp); + } } static void ib_uverbs_release_event_file(struct kref *ref) @@ -307,7 +312,9 @@ static void ib_uverbs_release_file(struct kref *ref) struct ib_uverbs_file *file = container_of(ref, struct ib_uverbs_file, ref); - module_put(file->device->ib_dev->owner); + if (!(file->device->flags & UVERBS_FLAG_DISASSOCIATE)) + module_put(file->device->ib_dev->owner); + kref_put(&file->device->ref, ib_uverbs_release_dev); kfree(file); @@ -330,9 +337,14 @@ static ssize_t ib_uverbs_event_read(struct file *filp, char __user *buf, return -EAGAIN; if (wait_event_interruptible(file->poll_wait, - !list_empty(&file->event_list))) + (!list_empty(&file->event_list) || + file->uverbs_file->device->disassociated))) return -ERESTARTSYS; + /* We reach here when list is not empty or when device was disassociated */ + if (list_empty(&file->event_list) && file->uverbs_file->device->disassociated) + return -EIO; + spin_lock_irq(&file->lock); } @@ -405,12 +417,17 @@ static int ib_uverbs_event_close(struct inode *inode, struct file *filp) } spin_unlock_irq(&file->lock); - if (file->is_async) { - ib_unregister_event_handler(&file->uverbs_file->event_handler); - kref_put(&file->uverbs_file->ref, ib_uverbs_release_file); + mutex_lock(&file->uverbs_file->device->disassociate_mutex); + if (!file->uverbs_file->device->disassociated) { + list_del(&file->list); + if (file->is_async) + ib_unregister_event_handler(&file->uverbs_file->event_handler); } - kref_put(&file->ref, ib_uverbs_release_event_file); + mutex_unlock(&file->uverbs_file->device->disassociate_mutex); + + kref_put(&file->uverbs_file->ref, ib_uverbs_release_file); + kref_put(&file->ref, ib_uverbs_release_event_file); return 0; } @@ -546,7 +563,7 @@ struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, struct ib_uverbs_event_file *ev_file; struct file *filp; - ev_file = kmalloc(sizeof *ev_file, GFP_KERNEL); + ev_file = kzalloc(sizeof *ev_file, GFP_KERNEL); if (!ev_file) return ERR_PTR(-ENOMEM); @@ -561,10 +578,25 @@ struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, filp = anon_inode_getfile("[infinibandevent]", &uverbs_event_fops, ev_file, O_RDONLY); - if (IS_ERR(filp)) + if (IS_ERR(filp)) { kfree(ev_file); + return filp; + } + + mutex_lock(&uverbs_file->device->disassociate_mutex); + if (!uverbs_file->device->disassociated) { + list_add_tail(&ev_file->list, + &uverbs_file->device->uverbs_events_file_list); + mutex_unlock(&uverbs_file->device->disassociate_mutex); - return filp; + return filp; + } + + mutex_unlock(&uverbs_file->device->disassociate_mutex); + + fput(filp); + kfree(ev_file); + return ERR_PTR(-EIO); } /* @@ -602,6 +634,8 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, struct ib_uverbs_file *file = filp->private_data; struct ib_uverbs_cmd_hdr hdr; __u32 flags; + int srcu_key; + ssize_t ret; if (count < sizeof hdr) return -EINVAL; @@ -609,6 +643,12 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, if (copy_from_user(&hdr, buf, sizeof hdr)) return -EFAULT; + srcu_key = srcu_read_lock(&file->device->disassociate_srcu); + if (file->device->disassociated) { + ret = -EIO; + goto out; + } + flags = (hdr.command & IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT; @@ -616,26 +656,36 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, __u32 command; if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK | - IB_USER_VERBS_CMD_COMMAND_MASK)) - return -EINVAL; + IB_USER_VERBS_CMD_COMMAND_MASK)) { + ret = -EINVAL; + goto out; + } command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; if (command >= ARRAY_SIZE(uverbs_cmd_table) || - !uverbs_cmd_table[command]) - return -EINVAL; + !uverbs_cmd_table[command]) { + ret = -EINVAL; + goto out; + } if (!file->ucontext && - command != IB_USER_VERBS_CMD_GET_CONTEXT) - return -EINVAL; + command != IB_USER_VERBS_CMD_GET_CONTEXT) { + ret = -EINVAL; + goto out; + } - if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << command))) - return -ENOSYS; + if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << command))) { + ret = -ENOSYS; + goto out; + } - if (hdr.in_words * 4 != count) - return -EINVAL; + if (hdr.in_words * 4 != count) { + ret = -EINVAL; + goto out; + } - return uverbs_cmd_table[command](file, + ret = uverbs_cmd_table[command](file, buf + sizeof(hdr), hdr.in_words * 4, hdr.out_words * 4); @@ -650,47 +700,69 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, size_t written_count = count; if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK | - IB_USER_VERBS_CMD_COMMAND_MASK)) - return -EINVAL; + IB_USER_VERBS_CMD_COMMAND_MASK)) { + ret = -EINVAL; + goto out; + } command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK; if (command >= ARRAY_SIZE(uverbs_ex_cmd_table) || - !uverbs_ex_cmd_table[command]) - return -ENOSYS; + !uverbs_ex_cmd_table[command]) { + ret = -ENOSYS; + goto out; + } - if (!file->ucontext) - return -EINVAL; + if (!file->ucontext) { + ret = -EINVAL; + goto out; + } - if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull << command))) - return -ENOSYS; + if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull << command))) { + ret = -ENOSYS; + goto out; + } - if (count < (sizeof(hdr) + sizeof(ex_hdr))) - return -EINVAL; + if (count < (sizeof(hdr) + sizeof(ex_hdr))) { + ret = -EINVAL; + goto out; + } - if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr))) - return -EFAULT; + if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr))) { + ret = -EFAULT; + goto out; + } count -= sizeof(hdr) + sizeof(ex_hdr); buf += sizeof(hdr) + sizeof(ex_hdr); - if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count) - return -EINVAL; + if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count) { + ret = -EINVAL; + goto out; + } - if (ex_hdr.cmd_hdr_reserved) - return -EINVAL; + if (ex_hdr.cmd_hdr_reserved) { + ret = -EINVAL; + goto out; + } if (ex_hdr.response) { - if (!hdr.out_words && !ex_hdr.provider_out_words) - return -EINVAL; + if (!hdr.out_words && !ex_hdr.provider_out_words) { + ret = -EINVAL; + goto out; + } if (!access_ok(VERIFY_WRITE, (void __user *) (unsigned long) ex_hdr.response, - (hdr.out_words + ex_hdr.provider_out_words) * 8)) - return -EFAULT; + (hdr.out_words + ex_hdr.provider_out_words) * 8)) { + ret = -EFAULT; + goto out; + } } else { - if (hdr.out_words || ex_hdr.provider_out_words) - return -EINVAL; + if (hdr.out_words || ex_hdr.provider_out_words) { + ret = -EINVAL; + goto out; + } } INIT_UDATA_BUF_OR_NULL(&ucore, buf, (unsigned long) ex_hdr.response, @@ -707,22 +779,37 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, &uhw); if (err) - return err; - - return written_count; + ret = err; + else + ret = written_count; + } else { + ret = -ENOSYS; } - return -ENOSYS; +out: + srcu_read_unlock(&file->device->disassociate_srcu, srcu_key); + return ret; } static int ib_uverbs_mmap(struct file *filp, struct vm_area_struct *vma) { struct ib_uverbs_file *file = filp->private_data; + int ret = 0; + int srcu_key; + + srcu_key = srcu_read_lock(&file->device->disassociate_srcu); + if (file->device->disassociated) { + ret = -EIO; + goto out; + } if (!file->ucontext) - return -ENODEV; + ret = -ENODEV; else - return file->device->ib_dev->mmap(file->ucontext, vma); + ret = file->device->ib_dev->mmap(file->ucontext, vma); +out: + srcu_read_unlock(&file->device->disassociate_srcu, srcu_key); + return ret; } /* @@ -740,6 +827,7 @@ static int ib_uverbs_open(struct inode *inode, struct file *filp) struct ib_uverbs_device *dev; struct ib_uverbs_file *file; int ret; + int module_dependent; dev = container_of(inode->i_cdev, struct ib_uverbs_device, cdev); if (dev) @@ -747,15 +835,31 @@ static int ib_uverbs_open(struct inode *inode, struct file *filp) else return -ENXIO; - if (!try_module_get(dev->ib_dev->owner)) { - ret = -ENODEV; + mutex_lock(&dev->disassociate_mutex); + if (dev->disassociated) { + ret = -EIO; goto err; } - file = kmalloc(sizeof *file, GFP_KERNEL); + /* In case IB device supports disassociate ucontext, there is no hard + * dependency between uverbs device and its low level device. + */ + module_dependent = !(dev->flags & UVERBS_FLAG_DISASSOCIATE); + + if (module_dependent) { + if (!try_module_get(dev->ib_dev->owner)) { + ret = -ENODEV; + goto err; + } + } + + file = kzalloc(sizeof *file, GFP_KERNEL); if (!file) { ret = -ENOMEM; - goto err_module; + if (module_dependent) + goto err_module; + + goto err; } file->device = dev; @@ -765,6 +869,8 @@ static int ib_uverbs_open(struct inode *inode, struct file *filp) mutex_init(&file->mutex); filp->private_data = file; + list_add_tail(&file->list, &dev->uverbs_file_list); + mutex_unlock(&dev->disassociate_mutex); return nonseekable_open(inode, filp); @@ -772,6 +878,7 @@ err_module: module_put(dev->ib_dev->owner); err: + mutex_unlock(&dev->disassociate_mutex); kref_put(&dev->ref, ib_uverbs_release_dev); return ret; } @@ -779,9 +886,26 @@ err: static int ib_uverbs_close(struct inode *inode, struct file *filp) { struct ib_uverbs_file *file = filp->private_data; + struct ib_ucontext *ucontext = NULL; + int srcu_key; + + srcu_key = srcu_read_lock(&file->device->disassociate_srcu); + mutex_lock(&file->device->disassociate_mutex); + if (!file->device->disassociated) { + /* No need to remove from the list once alreday disassociated. + * Try doing that might race with ib_uverbs_free_hw_resources + * as mutex is not held by that time. + */ + list_del(&file->list); + ucontext = file->ucontext; + } + + mutex_unlock(&file->device->disassociate_mutex); - ib_uverbs_cleanup_ucontext(file, file->ucontext); + if (ucontext) + ib_uverbs_cleanup_ucontext(file, ucontext); + srcu_read_unlock(&file->device->disassociate_srcu, srcu_key); if (file->async_file) kref_put(&file->async_file->ref, ib_uverbs_release_event_file); @@ -873,6 +997,7 @@ static void ib_uverbs_add_one(struct ib_device *device) int devnum; dev_t base; struct ib_uverbs_device *uverbs_dev; + int ret; if (!device->alloc_ucontext) return; @@ -885,6 +1010,13 @@ static void ib_uverbs_add_one(struct ib_device *device) init_completion(&uverbs_dev->comp); uverbs_dev->xrcd_tree = RB_ROOT; mutex_init(&uverbs_dev->xrcd_tree_mutex); + mutex_init(&uverbs_dev->disassociate_mutex); + ret = init_srcu_struct(&uverbs_dev->disassociate_srcu); + if (ret) + goto err_init; + + INIT_LIST_HEAD(&uverbs_dev->uverbs_file_list); + INIT_LIST_HEAD(&uverbs_dev->uverbs_events_file_list); spin_lock(&map_lock); devnum = find_first_zero_bit(dev_map, IB_UVERBS_MAX_DEVICES); @@ -926,6 +1058,9 @@ static void ib_uverbs_add_one(struct ib_device *device) if (device_create_file(uverbs_dev->dev, &dev_attr_abi_version)) goto err_class; + if (device->disassociate_ucontext) + uverbs_dev->flags |= UVERBS_FLAG_DISASSOCIATE; + ib_set_client_data(device, &uverbs_client, uverbs_dev); return; @@ -941,15 +1076,71 @@ err_cdev: clear_bit(devnum, overflow_map); err: + cleanup_srcu_struct(&uverbs_dev->disassociate_srcu); + +err_init: kref_put(&uverbs_dev->ref, ib_uverbs_release_dev); wait_for_completion(&uverbs_dev->comp); kfree(uverbs_dev); return; } +static void ib_uverbs_free_hw_resources(struct ib_uverbs_device *uverbs_dev) +{ + struct ib_uverbs_file *file, *tmp_file; + struct ib_uverbs_event_file *event_file, *tmp_event_file; + struct ib_event event; + + mutex_lock(&uverbs_dev->disassociate_mutex); + uverbs_dev->disassociated = 1; + /* We must release the mutex before going ahead and calling + * disassociate_ucontext as a nested call to uverbs_close might + * be called as a result of freeing the resources (e.g mmput). + * In addition, we should take an extra ref count on files to prevent + * them being freed as part of parallel file closing, from other task + * or from event occurs internally from that one. + */ + list_for_each_entry(file, &uverbs_dev->uverbs_file_list, list) + kref_get(&file->ref); + list_for_each_entry(event_file, &uverbs_dev->uverbs_events_file_list, list) + kref_get(&event_file->ref); + mutex_unlock(&uverbs_dev->disassociate_mutex); + + /* Pending running commands to terminate */ + synchronize_srcu(&uverbs_dev->disassociate_srcu); + event.event = IB_EVENT_DEVICE_FATAL; + event.element.port_num = 0; + event.device = uverbs_dev->ib_dev; + + list_for_each_entry(file, &uverbs_dev->uverbs_file_list, list) { + ib_uverbs_event_handler(&file->event_handler, &event); + uverbs_dev->ib_dev->disassociate_ucontext(file->ucontext); + ib_uverbs_cleanup_ucontext(file, file->ucontext); + } + + list_for_each_entry(event_file, &uverbs_dev->uverbs_events_file_list, list) { + if (event_file->is_async) { + /* ib_device is freed once that function/remove_one is + * finished, must unregister the event handler before. + */ + ib_unregister_event_handler(&event_file->uverbs_file->event_handler); + } + + wake_up_interruptible(&event_file->poll_wait); + kill_fasync(&event_file->async_queue, SIGIO, POLL_IN); + } + + /* A safe iterator is needed as file might be freed as part of loop */ + list_for_each_entry_safe(file, tmp_file, &uverbs_dev->uverbs_file_list, list) + kref_put(&file->ref, ib_uverbs_release_file); + + list_for_each_entry_safe(event_file, tmp_event_file, &uverbs_dev->uverbs_events_file_list, list) + kref_put(&event_file->ref, ib_uverbs_release_event_file); +} static void ib_uverbs_remove_one(struct ib_device *device) { struct ib_uverbs_device *uverbs_dev = ib_get_client_data(device, &uverbs_client); + int wait_clients = 1; if (!uverbs_dev) return; @@ -963,9 +1154,31 @@ static void ib_uverbs_remove_one(struct ib_device *device) else clear_bit(uverbs_dev->devnum - IB_UVERBS_MAX_DEVICES, overflow_map); + if (uverbs_dev->flags & UVERBS_FLAG_DISASSOCIATE) { + /* We disassociate HW resources and immediately returning, not + * pending to active userspace clients. Upon returning ib_device + * may be freed internally and is not valid any more. + * uverbs_device is still available, when all clients close + * their files, the uverbs device ref count will be zero and its + * resources will be freed. + * Note: At that step no more files can be opened on that cdev + * as it was deleted, however active clients can still issue + * commands and close their open files. + */ + ib_uverbs_free_hw_resources(uverbs_dev); + wait_clients = 0; + /* ib device can no longer be accessed, it is freed when this + * function returns. + */ + uverbs_dev->ib_dev = NULL; + } + /* ref count taken as part of add one is put back in both modes. */ kref_put(&uverbs_dev->ref, ib_uverbs_release_dev); - wait_for_completion(&uverbs_dev->comp); - kfree(uverbs_dev); + if (wait_clients) { + wait_for_completion(&uverbs_dev->comp); + cleanup_srcu_struct(&uverbs_dev->disassociate_srcu); + kfree(uverbs_dev); + } } static char *uverbs_devnode(struct device *dev, umode_t *mode) diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 65994a1..d9501e7 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -1652,6 +1652,7 @@ struct ib_device { int (*destroy_flow)(struct ib_flow *flow_id); int (*check_mr_status)(struct ib_mr *mr, u32 check_mask, struct ib_mr_status *mr_status); + void (*disassociate_ucontext)(struct ib_ucontext *ibcontext); struct ib_dma_mapping_ops *dma_ops;