| Message ID | 1434984438-21733-2-git-send-email-yishaih@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Jun 22, 2015 at 05:47:14PM +0300, Yishai Hadas wrote: > fd_install(resp.async_fd, filp); > @@ -386,6 +376,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file, > return in_len; > > err_file: > + ib_uverbs_free_async_event_file(file); > fput(filp); This looks really weird. > +void ib_uverbs_free_async_event_file(struct ib_uverbs_file *file) > +{ > + kref_put(&file->async_file->ref, ib_uverbs_release_event_file); > + file->async_file = NULL; > +} So that put is supposed to pair with this get? > + uverbs_file->async_file = ev_file; > + kref_get(&uverbs_file->async_file->ref); [..] > + fput(filp); > + uverbs_file->async_file = NULL; But isn't it null? Again again, WTF? async_file is a kref'd thing, if you copy or assign to it you need to manipulate the kref, so the null assign should be dropping the ref. Whis looks good enough to remove ib_uverbs_free_async_event_file, if the flip is created OK then the uverbs_file->async file can remain set until the uverbs file is closed. 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 6/24/2015 8:57 PM, Jason Gunthorpe wrote: > On Mon, Jun 22, 2015 at 05:47:14PM +0300, Yishai Hadas wrote: >> fd_install(resp.async_fd, filp); >> @@ -386,6 +376,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file, >> return in_len; >> >> err_file: >> + ib_uverbs_free_async_event_file(file); >> fput(filp); > > This looks really weird. We need to cleanup all by that step otherwise we might leak that async file, see my last comment below which clarifies that point. As ib_uverbs_release_event_file is static function in uverbs_main it makes sense to expose this cleanup function similar to ib_uverbs_alloc_event_file which is exposed from uverbs_main and make the allocation. >> +void ib_uverbs_free_async_event_file(struct ib_uverbs_file *file) >> +{ >> + kref_put(&file->async_file->ref, ib_uverbs_release_event_file); >> + file->async_file = NULL; >> +} > > So that put is supposed to pair with this get? Yes > >> + uverbs_file->async_file = ev_file; >> + kref_get(&uverbs_file->async_file->ref); > > [..] > >> + fput(filp); >> + uverbs_file->async_file = NULL; > > But isn't it null? > No, this NULL is set when ib_uverbs_alloc_event_file failed internally, the above flow occurs when file was created OK but later it fails via copy_to_user. > Again again, WTF? async_file is a kref'd thing, if you copy or assign > to it you need to manipulate the kref, so the null assign should be > dropping the ref. > The logic in ib_uverbs_alloc_event_file if kref oriented, the original code which uses free didn't follow this convention, currently in case ib_uverbs_alloc_event_file failed internally it uses kref_put(&ev_file->ref, ib_uverbs_release_event_file) which internally frees the memory, in case we had an error after that the file was created (e.g ib_register_event_handler) need to use fput(filp) to make an extra cleanup which again uses ref count handling. The uverbs_file->async_file = NULL command just cleans the previous setting of uverbs_file->async_file = ev_file that was done before to prevent it be cleanup when the uverbs_file itself is closed. > Whis looks good enough to remove ib_uverbs_free_async_event_file, if > the flip is created OK then the uverbs_file->async file can remain set > until the uverbs file is closed. This is wrong, as file->ucontext is NULL because of the failure in copy_to_user, application might recall to ib_uverbs_get_context in that case we leak that previous async_file, that's why the code must cleanup and put NULL by that step. To your request put WARN_ON(uverbs_file->async_file) as part of ib_uverbs_alloc_event_file to make sure that we don't hit that case. This patch fixes a bug that currently exists upstream that you pointed on that may cause an oops, hope that above clarifies the issues and you can ACK on. -- 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, Jun 25, 2015 at 02:46:02PM +0300, Yishai Hadas wrote: > On 6/24/2015 8:57 PM, Jason Gunthorpe wrote: > >On Mon, Jun 22, 2015 at 05:47:14PM +0300, Yishai Hadas wrote: > >> fd_install(resp.async_fd, filp); > >>@@ -386,6 +376,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file, > >> return in_len; > >> > >> err_file: > >>+ ib_uverbs_free_async_event_file(file); > >> fput(filp); > > > >This looks really weird. > > We need to cleanup all by that step otherwise we might leak that async file, > see my last comment below which clarifies that point. > As ib_uverbs_release_event_file is static function in uverbs_main it makes > sense to expose this cleanup function similar to ib_uverbs_alloc_event_file > which is exposed from uverbs_main and make the allocation. Okay, sure, that makes sense. > >Again again, WTF? async_file is a kref'd thing, if you copy or assign > >to it you need to manipulate the kref, so the null assign should be > >dropping the ref. > > The logic in ib_uverbs_alloc_event_file if kref oriented, the original code > which uses free didn't follow this convention, currently in case Yes, and you fixed it, this is good: @@ -557,15 +563,38 @@ struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, ev_file = kmalloc(sizeof *ev_file, GFP_KERNEL); kref_init(&ev_file->ref); if (IS_ERR(filp)) + goto err; +err: + kref_put(&ev_file->ref, ib_uverbs_release_event_file); The kref_init and the kref_put are a matching pair, great! > had an error after that the file was created (e.g ib_register_event_handler) > need to use fput(filp) to make an extra cleanup which again uses ref count > handling. Yep, that is a good idea too. But understand what it means, When this happens: filp = anon_inode_getfile("[infinibandevent]", &uverbs_event_fops, ev_file, O_RDONLY); The unref pairs for these gets: kref_init(&ev_file->ref); + kref_get(&uverbs_file->ref); Are logically moved into flip, and now fput(flip) will call ib_uverbs_event_close() and both those krefs will be released. For this reason, for clarity, I would also move the 'kref_get(&uverbs_file->ref);' to be before anon_inode_getfile() Which means this: @@ -557,15 +563,38 @@ struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, + kref_get(&uverbs_file->ref); + if (is_async) { + if (ret) + goto put_file; +put_file: + fput(filp); +err: + kref_put(&ev_file->ref, ib_uverbs_release_event_file); Is a double put on ev_file, since fput -> ib_uverbs_event_close does one, and then err does another. (remember, the ref was moved into the flip) Next: + if (is_async) { + uverbs_file->async_file = ev_file; + kref_get(&uverbs_file->async_file->ref); + if (ret) + goto put_file; +put_file: + uverbs_file->async_file = NULL; Where is the paired kref_put? There are two I can see: +void ib_uverbs_free_async_event_file(struct ib_uverbs_file *file) +{ + kref_put(&file->async_file->ref, ib_uverbs_release_event_file); ----- static int ib_uverbs_close(struct inode *inode, struct file *filp) kref_put(&file->async_file->ref, ib_uverbs_release_event_file); As far as I can tell, neither of these are called, so we have an unbalanced put. Which brings me back to my statement: > >Again again, WTF? async_file is a kref'd thing, if you copy or assign > >to it you need to manipulate the kref, so the null assign should be > >dropping the ref. So, the two possible fixes are 1) Make the call to ib_uverbs_free_async_event_file() happen even if ib_uverbs_alloc_event_file() fails 2) Do in ib_uverbs_alloc_event_file + kref_put(&uverbs_file->async_file->ref, ib_uverbs_release_event_file); + uverbs_file->async_file = NULL; Which may as well just be a call to ib_uverbs_free_async_event_file() In either situation the naked async_file = NULL is fixed to have a kref beside it. This is basic invariant of how kref works. Read the 'rules' in Documentation/kref.txt So, as I reviewer, I see a kref'd variable (async_file) being manipulated without corresponding kref code. I *KNOW* something is wrong. You answer didn't address this, you needed to ID where the pair'd kref_put was and justify having the '= NULL' so far away from it. > >Whis looks good enough to remove ib_uverbs_free_async_event_file, if > >the flip is created OK then the uverbs_file->async file can remain set > >until the uverbs file is closed. > > This is wrong, as file->ucontext is NULL because of the failure in > copy_to_user, application might recall to ib_uverbs_get_context in that case > we leak that previous async_file, that's why the code must cleanup and put > NULL by that step. To your request put WARN_ON(uverbs_file->async_file) as > part of ib_uverbs_alloc_event_file to make sure that we don't hit that case. Yes, that makes sense. 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 6/25/2015 8:52 PM, Jason Gunthorpe wrote: > On Thu, Jun 25, 2015 at 02:46:02PM +0300, Yishai Hadas wrote: >> On 6/24/2015 8:57 PM, Jason Gunthorpe wrote: >>> On Mon, Jun 22, 2015 at 05:47:14PM +0300, Yishai Hadas wrote: >>>> fd_install(resp.async_fd, filp); >>>> @@ -386,6 +376,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file, >>>> return in_len; >>>> >>>> err_file: >>>> + ib_uverbs_free_async_event_file(file); >>>> fput(filp); >>> >>> This looks really weird. >> >> We need to cleanup all by that step otherwise we might leak that async file, >> see my last comment below which clarifies that point. >> As ib_uverbs_release_event_file is static function in uverbs_main it makes >> sense to expose this cleanup function similar to ib_uverbs_alloc_event_file >> which is exposed from uverbs_main and make the allocation. > > Okay, sure, that makes sense. > >>> Again again, WTF? async_file is a kref'd thing, if you copy or assign >>> to it you need to manipulate the kref, so the null assign should be >>> dropping the ref. >> >> The logic in ib_uverbs_alloc_event_file if kref oriented, the original code >> which uses free didn't follow this convention, currently in case > > Yes, and you fixed it, this is good: > > @@ -557,15 +563,38 @@ struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, > ev_file = kmalloc(sizeof *ev_file, GFP_KERNEL); > kref_init(&ev_file->ref); > if (IS_ERR(filp)) > + goto err; > +err: > + kref_put(&ev_file->ref, ib_uverbs_release_event_file); > > The kref_init and the kref_put are a matching pair, great! > >> had an error after that the file was created (e.g ib_register_event_handler) >> need to use fput(filp) to make an extra cleanup which again uses ref count >> handling. > > Yep, that is a good idea too. But understand what it means, > > When this happens: > filp = anon_inode_getfile("[infinibandevent]", &uverbs_event_fops, > ev_file, O_RDONLY); > The unref pairs for these gets: > kref_init(&ev_file->ref); > + kref_get(&uverbs_file->ref); > > Are logically moved into flip, and now fput(flip) will call > ib_uverbs_event_close() and both those krefs will be released. > > For this reason, for clarity, I would also move the > 'kref_get(&uverbs_file->ref);' to be before anon_inode_getfile() Taking kref_get(&uverbs_file->ref) before anon_inode_getfile has succeeded is not correct, only when anon_inode_getfile succeeds we can call fput(filp) which internally makes the kref_get on the uverbs_file as part of ib_uverbs_event_close. In current code we take ref count on the ev_file as part kref_init(&ev_file->ref) which is put as part of the "err" label err: kref_put(&ev_file->ref, ib_uverbs_release_event_file); In case anon_inode_getfile has succeeded we take also the ref_count on the uverbs_file which will be put back upon closing the file and the kref put/get are fully symmetric. For the async case we also fully symmetric here, see next comment below. > Which means this: > > @@ -557,15 +563,38 @@ struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, > + kref_get(&uverbs_file->ref); > + if (is_async) { > + if (ret) > + goto put_file; > +put_file: > + fput(filp); > +err: > + kref_put(&ev_file->ref, ib_uverbs_release_event_file); > > Is a double put on ev_file, since fput -> ib_uverbs_event_close does one, and > then err does another. (remember, the ref was moved into the flip) > > Next: > > + if (is_async) { > + uverbs_file->async_file = ev_file; > + kref_get(&uverbs_file->async_file->ref); > + if (ret) > + goto put_file; > +put_file: > + uverbs_file->async_file = NULL; > > Where is the paired kref_put? There are two I can see: > > +void ib_uverbs_free_async_event_file(struct ib_uverbs_file *file) > +{ > + kref_put(&file->async_file->ref, ib_uverbs_release_event_file); > ----- > static int ib_uverbs_close(struct inode *inode, struct file *filp) > kref_put(&file->async_file->ref, ib_uverbs_release_event_file); > > As far as I can tell, neither of these are called, so we have an > unbalanced put. You are wrong here, we have here balanced put, the first is done as part of fput(filp) -> ib_uverbs_event_close_file -> kref_put(&file->ref, ib_uverbs_release_event_file) and the second at the end of this function as part of the err: label kref_put(&ev_file->ref, ib_uverbs_release_event_file); > So, as I reviewer, I see a kref'd variable (async_file) being > manipulated without corresponding kref code. I *KNOW* something is > wrong. You answer didn't address this, you needed to ID where the > pair'd kref_put was and justify having the '= NULL' so far away from > it. See my comment above, we have here pair kref_put, the uverbs_file->async_file = NULL comes to prevent a third in-correct kref_put as part of ib_uverbs_close. Please review and let me know whether you can ACK on this patch. -- 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 Sun, Jun 28, 2015 at 05:33:04PM +0300, Yishai Hadas wrote: > You are wrong here, we have here balanced put, the first is done as > part of fput(filp) -> ib_uverbs_event_close_file -> > kref_put(&file->ref, ib_uverbs_release_event_file) and the second at > the end of this function as part of the err: label > kref_put(&ev_file->ref, ib_uverbs_release_event_file); Ugh, that is so gross, seriously? Sure, the counts work out, but that is *totally unreadable*. Sigh, just use this: struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, int is_async) { struct ib_uverbs_event_file *ev_file; struct file *filp; + int ret; - ev_file = kmalloc(sizeof *ev_file, GFP_KERNEL); + ev_file = kzalloc(sizeof(*ev_file), GFP_KERNEL); if (!ev_file) return ERR_PTR(-ENOMEM); @@ -555,15 +561,41 @@ struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, INIT_LIST_HEAD(&ev_file->event_list); init_waitqueue_head(&ev_file->poll_wait); ev_file->uverbs_file = uverbs_file; + kref_get(&ev_file->uverbs_file->ref); ev_file->async_queue = NULL; - ev_file->is_async = is_async; ev_file->is_closed = 0; filp = anon_inode_getfile("[infinibandevent]", &uverbs_event_fops, ev_file, O_RDONLY); if (IS_ERR(filp)) - kfree(ev_file); + goto err_put_refs; + + if (is_async) { + WARN_ON(uverbs_file->async_file); + uverbs_file->async_file = ev_file; + kref_get(&uverbs_file->async_file->ref); + INIT_IB_EVENT_HANDLER(&uverbs_file->event_handler, + uverbs_file->device->ib_dev, + ib_uverbs_event_handler); + ret = ib_register_event_handler(&uverbs_file->event_handler); + if (ret) + goto err_put_file; + + /* At that point async file stuff was fully set */ + ev_file->is_async = 1; + } + + return filp; + +err_put_file: + fput(filp); + kref_put(&uverbs_file->async_file->ref, ib_uverbs_release_file); + uverbs_file->async_file = NULL; + return ERR_PTR(ret); +err_put_refs: + kref_put(&ev_file->uverbs_file->ref, ib_uverbs_release_file); + kref_put(&ev_file->ref, ib_uverbs_release_event_file); return filp; } 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 6/29/2015 8:40 PM, Jason Gunthorpe wrote: > On Sun, Jun 28, 2015 at 05:33:04PM +0300, Yishai Hadas wrote: > >> You are wrong here, we have here balanced put, the first is done as >> part of fput(filp) -> ib_uverbs_event_close_file -> >> kref_put(&file->ref, ib_uverbs_release_event_file) and the second at >> the end of this function as part of the err: label >> kref_put(&ev_file->ref, ib_uverbs_release_event_file); > > Ugh, that is so gross, seriously? Sure, the counts work out, but that > is *totally unreadable*. > > Sigh, just use this: > > struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, > int is_async) > { > struct ib_uverbs_event_file *ev_file; > struct file *filp; > + int ret; > > - ev_file = kmalloc(sizeof *ev_file, GFP_KERNEL); > + ev_file = kzalloc(sizeof(*ev_file), GFP_KERNEL); > if (!ev_file) > return ERR_PTR(-ENOMEM); > > @@ -555,15 +561,41 @@ struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, > INIT_LIST_HEAD(&ev_file->event_list); > init_waitqueue_head(&ev_file->poll_wait); > ev_file->uverbs_file = uverbs_file; > + kref_get(&ev_file->uverbs_file->ref); > ev_file->async_queue = NULL; > - ev_file->is_async = is_async; > ev_file->is_closed = 0; > > filp = anon_inode_getfile("[infinibandevent]", &uverbs_event_fops, > ev_file, O_RDONLY); > if (IS_ERR(filp)) > - kfree(ev_file); > + goto err_put_refs; > + > + if (is_async) { > + WARN_ON(uverbs_file->async_file); > + uverbs_file->async_file = ev_file; > + kref_get(&uverbs_file->async_file->ref); > + INIT_IB_EVENT_HANDLER(&uverbs_file->event_handler, > + uverbs_file->device->ib_dev, > + ib_uverbs_event_handler); > + ret = ib_register_event_handler(&uverbs_file->event_handler); > + if (ret) > + goto err_put_file; > + > + /* At that point async file stuff was fully set */ > + ev_file->is_async = 1; > + } > + > + return filp; > + > +err_put_file: > + fput(filp); > + kref_put(&uverbs_file->async_file->ref, ib_uverbs_release_file); It should be: kref_put(&uverbs_file->async_file->ref, ib_uverbs_release_event_file) instead of: kref_put(&uverbs_file->async_file->ref, ib_uverbs_release_file); Please note that in that approach we duplicate above line as it appears here and in below err_put_refs label as uverbs_file->async_file is really ev_file. In my patch we used one line instead of those duplicated lines, however as you think that it clarifies things will go with your suggestion after fixing above note, thanks. > + uverbs_file->async_file = NULL; > + return ERR_PTR(ret); > > +err_put_refs: > + kref_put(&ev_file->uverbs_file->ref, ib_uverbs_release_file); > + kref_put(&ev_file->ref, ib_uverbs_release_event_file); > return filp; > } > > 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 Tue, Jun 30, 2015 at 12:22:02AM +0300, Yishai Hadas wrote: > It should be: > kref_put(&uverbs_file->async_file->ref, ib_uverbs_release_event_file) > instead of: > kref_put(&uverbs_file->async_file->ref, ib_uverbs_release_file); Right > Please note that in that approach we duplicate above line as it appears here > and in below err_put_refs label as uverbs_file->async_file is really > ev_file. Well, sort of. Yes, it is the same value, but no, it isn't the same kref. Just like locking always works on data, not code, krefs should always be applied to the pointer not to the pointed data. The kref manipulation should always be near the pointer value change it is working for, and have a clear relation to the pointer it is krefing. > In my patch we used one line instead of those duplicated lines, however as > you think that it clarifies things will go with your suggestion after fixing > above note, thanks. You shouldn't think of them as duplicates, they are for different things, even if the actual functionality is the same. It is *documentation* 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 ba365b6..60e6e3d 100644 --- a/drivers/infiniband/core/uverbs.h +++ b/drivers/infiniband/core/uverbs.h @@ -178,6 +178,7 @@ void idr_remove_uobj(struct idr *idp, struct ib_uobject *uobj); struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, int is_async); +void ib_uverbs_free_async_event_file(struct ib_uverbs_file *uverbs_file); struct ib_uverbs_event_file *ib_uverbs_lookup_comp_file(int fd); void ib_uverbs_release_ucq(struct ib_uverbs_file *file, diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index bbb02ff..5720a92 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -367,16 +367,6 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file, goto err_file; } - file->async_file = filp->private_data; - - INIT_IB_EVENT_HANDLER(&file->event_handler, file->device->ib_dev, - ib_uverbs_event_handler); - ret = ib_register_event_handler(&file->event_handler); - if (ret) - goto err_file; - - kref_get(&file->async_file->ref); - kref_get(&file->ref); file->ucontext = ucontext; fd_install(resp.async_fd, filp); @@ -386,6 +376,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file, return in_len; err_file: + ib_uverbs_free_async_event_file(file); fput(filp); err_fd: diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index f6eef2d..ed54e4e 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -406,10 +406,9 @@ static int ib_uverbs_event_close(struct inode *inode, struct file *filp) } spin_unlock_irq(&file->lock); - if (file->is_async) { + if (file->is_async) ib_unregister_event_handler(&file->uverbs_file->event_handler); - kref_put(&file->uverbs_file->ref, ib_uverbs_release_file); - } + kref_put(&file->uverbs_file->ref, ib_uverbs_release_file); kref_put(&file->ref, ib_uverbs_release_event_file); return 0; @@ -541,13 +540,20 @@ void ib_uverbs_event_handler(struct ib_event_handler *handler, NULL, NULL); } +void ib_uverbs_free_async_event_file(struct ib_uverbs_file *file) +{ + kref_put(&file->async_file->ref, ib_uverbs_release_event_file); + file->async_file = NULL; +} + struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, int is_async) { struct ib_uverbs_event_file *ev_file; struct file *filp; + int ret; - ev_file = kmalloc(sizeof *ev_file, GFP_KERNEL); + ev_file = kzalloc(sizeof(*ev_file), GFP_KERNEL); if (!ev_file) return ERR_PTR(-ENOMEM); @@ -557,15 +563,38 @@ struct file *ib_uverbs_alloc_event_file(struct ib_uverbs_file *uverbs_file, init_waitqueue_head(&ev_file->poll_wait); ev_file->uverbs_file = uverbs_file; ev_file->async_queue = NULL; - ev_file->is_async = is_async; ev_file->is_closed = 0; filp = anon_inode_getfile("[infinibandevent]", &uverbs_event_fops, ev_file, O_RDONLY); if (IS_ERR(filp)) - kfree(ev_file); + goto err; + + kref_get(&uverbs_file->ref); + if (is_async) { + WARN_ON(uverbs_file->async_file); + uverbs_file->async_file = ev_file; + kref_get(&uverbs_file->async_file->ref); + INIT_IB_EVENT_HANDLER(&uverbs_file->event_handler, + uverbs_file->device->ib_dev, + ib_uverbs_event_handler); + ret = ib_register_event_handler(&uverbs_file->event_handler); + if (ret) + goto put_file; + + /* At that point async file stuff was fully set */ + ev_file->is_async = 1; + } return filp; + +put_file: + fput(filp); + uverbs_file->async_file = NULL; + filp = ERR_PTR(ret); +err: + kref_put(&ev_file->ref, ib_uverbs_release_event_file); + return filp; } /*