| Message ID | VI1PR0502MB3008BCA1BEF667DD55450CF2D1A90@VI1PR0502MB3008.eurprd05.prod.outlook.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Mar 22, 2018 at 02:59:24PM -0600, Parav Pandit wrote: > Thanks for looking into it. > Though I didn't ack on the ML, I had debugged it similar report few > days back, fixed and verified it differently by rdma_addr_cancel() > doing cancel_delayed_work_sync() and completing the request after > that. > And if already cancelled than process_one_req to skip processing it. > I wasn't sure if work item itself can cancel itself reliably while > it is running. Do you know? Yes, the work pointer can be free'd within the worker, the workqueue code does not touch the pointer after the function runs to allow for this. > cancel_delayed_work_sync() appears more robust to me following similar other users. > This also avoid canceling it on every execution in process_one_req(). I like yours better because it ensures the callback will not be called after rdma_addr_cancel() which is overall much saner behavior, and I think possibly another bug? But let just do that with flush_workqueue() ? > Assuming cancel_delayed_work can cancel itself based on kdoc comment > that it can be called from any context, patch you posted should work > too. Minor modified hunk to reuse code in process_one_req() and > process_req() on top of your patch below. Yes. 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
> -----Original Message----- > From: Jason Gunthorpe > Sent: Thursday, March 22, 2018 4:22 PM > To: Parav Pandit <parav@mellanox.com> > Cc: linux-rdma@vger.kernel.org; Leon Romanovsky <leonro@mellanox.com>; > Mark Bloch <markb@mellanox.com>; Dmitry Vyukov <dvyukov@google.com>; > syzbot <syzbot+3b4acab09b6463472d0a@syzkaller.appspotmail.com>; Daniel > Jurgens <danielj@mellanox.com>; dledford@redhat.com; Johannes Berg > <johannes.berg@intel.com>; syzkaller-bugs@googlegroups.com > Subject: Re: [PATCH rdma-rc] RDMA/rdma_cm: Fix use after free race with > process_one_req > > On Thu, Mar 22, 2018 at 02:59:24PM -0600, Parav Pandit wrote: > > > Thanks for looking into it. > > Though I didn't ack on the ML, I had debugged it similar report few > > days back, fixed and verified it differently by rdma_addr_cancel() > > doing cancel_delayed_work_sync() and completing the request after > > that. > > > And if already cancelled than process_one_req to skip processing it. > > I wasn't sure if work item itself can cancel itself reliably while it > > is running. Do you know? > > Yes, the work pointer can be free'd within the worker, the workqueue code does > not touch the pointer after the function runs to allow for this. > > > cancel_delayed_work_sync() appears more robust to me following similar > other users. > > This also avoid canceling it on every execution in process_one_req(). > > I like yours better because it ensures the callback will not be called after > rdma_addr_cancel() which is overall much saner behavior, and I think possibly > another bug? > > But let just do that with flush_workqueue() ? > flush_workqueue() will force to execute all the work items for all pending entries, all must have to completed. Those pending delayed entries are unrelated to this work item/request in progress, and if they are large number of entries having 1 sec timeout, flush_workqueue() might take long. So one rdma_destroy_id will wait for other requests to be completed, which I think we should avoid. > > Assuming cancel_delayed_work can cancel itself based on kdoc comment > > that it can be called from any context, patch you posted should work > > too. Minor modified hunk to reuse code in process_one_req() and > > process_req() on top of your patch below. > > Yes. > > 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 Thu, Mar 22, 2018 at 09:30:37PM +0000, Parav Pandit wrote: > flush_workqueue() will force to execute all the work items for all > pending entries, all must have to completed. Those pending delayed > entries are unrelated to this work item/request in progress, and if > they are large number of entries having 1 sec timeout, > flush_workqueue() might take long. OK then > So one rdma_destroy_id will wait for other requests to be completed, > which I think we should avoid. I looked at this for a bit.. I really don't like how this code works. The idea that rdma_destroy_id() doesn't fence the callback is a bad design, but doesn't apparently cause any bug I can see. I also can't understand why the rdma_addr_client nonsense should exist, it seems to be rolled into the idea that cancel doesn't actually cancel. :( So lets just use the one line patch and save the rest for some other day.. 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
> -----Original Message----- > From: Jason Gunthorpe [mailto:jgg@ziepe.ca] > Sent: Thursday, March 22, 2018 5:31 PM > To: Parav Pandit <parav@mellanox.com> > Cc: linux-rdma@vger.kernel.org; Leon Romanovsky <leonro@mellanox.com>; > Mark Bloch <markb@mellanox.com>; Dmitry Vyukov <dvyukov@google.com>; > syzbot <syzbot+3b4acab09b6463472d0a@syzkaller.appspotmail.com>; Daniel > Jurgens <danielj@mellanox.com>; dledford@redhat.com; Johannes Berg > <johannes.berg@intel.com>; syzkaller-bugs@googlegroups.com > Subject: Re: [PATCH rdma-rc] RDMA/rdma_cm: Fix use after free race with > process_one_req > > On Thu, Mar 22, 2018 at 09:30:37PM +0000, Parav Pandit wrote: > > > flush_workqueue() will force to execute all the work items for all > > pending entries, all must have to completed. Those pending delayed > > entries are unrelated to this work item/request in progress, and if > > they are large number of entries having 1 sec timeout, > > flush_workqueue() might take long. > > OK then > > > So one rdma_destroy_id will wait for other requests to be completed, > > which I think we should avoid. > > I looked at this for a bit.. I really don't like how this code works. > > The idea that rdma_destroy_id() doesn't fence the callback is a bad design, but > doesn't apparently cause any bug I can see. > > I also can't understand why the rdma_addr_client nonsense should exist, it > seems to be rolled into the idea that cancel doesn't actually cancel. :( > > So lets just use the one line patch and save the rest for some other day.. > Ok. A helper function in the hunk is preferred as the code is same in both the functions. I will test it on Friday once. > 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 Fri, Mar 23, 2018 at 03:16:20AM +0000, Parav Pandit wrote: > > > > From: Jason Gunthorpe [mailto:jgg@ziepe.ca] > > Sent: Thursday, March 22, 2018 5:31 PM > > To: Parav Pandit <parav@mellanox.com> > > Cc: linux-rdma@vger.kernel.org; Leon Romanovsky <leonro@mellanox.com>; > > Mark Bloch <markb@mellanox.com>; Dmitry Vyukov <dvyukov@google.com>; > > syzbot <syzbot+3b4acab09b6463472d0a@syzkaller.appspotmail.com>; Daniel > > Jurgens <danielj@mellanox.com>; dledford@redhat.com; Johannes Berg > > <johannes.berg@intel.com>; syzkaller-bugs@googlegroups.com > > Subject: Re: [PATCH rdma-rc] RDMA/rdma_cm: Fix use after free race with > > process_one_req > > > > On Thu, Mar 22, 2018 at 09:30:37PM +0000, Parav Pandit wrote: > > > > > flush_workqueue() will force to execute all the work items for all > > > pending entries, all must have to completed. Those pending delayed > > > entries are unrelated to this work item/request in progress, and if > > > they are large number of entries having 1 sec timeout, > > > flush_workqueue() might take long. > > > > OK then > > > > > So one rdma_destroy_id will wait for other requests to be completed, > > > which I think we should avoid. > > > > I looked at this for a bit.. I really don't like how this code works. > > > > The idea that rdma_destroy_id() doesn't fence the callback is a bad design, but > > doesn't apparently cause any bug I can see. > > > > I also can't understand why the rdma_addr_client nonsense should exist, it > > seems to be rolled into the idea that cancel doesn't actually cancel. :( > > > > So lets just use the one line patch and save the rest for some other day.. > > Ok. A helper function in the hunk is preferred as the code is same > in both the functions. I will test it on Friday once. Now that I've looked at this closely enough to find the syzkaller bug, my preference is to tidy up the whole thing: https://github.com/jgunthorpe/linux/commits/cma-fix - Remove duplication, only one function processes work. Get rid of hold over list sorting, use the workqueue infrastructure itself - Make rdma_addr_cancel() a sane and safe API by having it fence - Get rid of the totally useless struct rdma_add_client and related Which is a pretty sweet little cleanup: drivers/infiniband/core/addr.c | 132 +++++++++++++++++++++++++++++++------------------------------------------------------------------ drivers/infiniband/core/cma.c | 6 +---- include/rdma/ib_addr.h | 20 +-------------- 3 files changed, 44 insertions(+), 114 deletions(-) What do you think Parav? 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 Jason, > -----Original Message----- > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > owner@vger.kernel.org] On Behalf Of Jason Gunthorpe > Sent: Thursday, March 22, 2018 11:03 PM > To: Parav Pandit <parav@mellanox.com> > Cc: linux-rdma@vger.kernel.org; Leon Romanovsky <leonro@mellanox.com>; > Mark Bloch <markb@mellanox.com>; Dmitry Vyukov <dvyukov@google.com>; > syzbot <syzbot+3b4acab09b6463472d0a@syzkaller.appspotmail.com>; Daniel > Jurgens <danielj@mellanox.com>; dledford@redhat.com; Johannes Berg > <johannes.berg@intel.com>; syzkaller-bugs@googlegroups.com > Subject: Re: [PATCH rdma-rc] RDMA/rdma_cm: Fix use after free race with > process_one_req > > On Fri, Mar 23, 2018 at 03:16:20AM +0000, Parav Pandit wrote: > > > > > > > From: Jason Gunthorpe [mailto:jgg@ziepe.ca] > > > Sent: Thursday, March 22, 2018 5:31 PM > > > To: Parav Pandit <parav@mellanox.com> > > > Cc: linux-rdma@vger.kernel.org; Leon Romanovsky > > > <leonro@mellanox.com>; Mark Bloch <markb@mellanox.com>; Dmitry > > > Vyukov <dvyukov@google.com>; syzbot > > > <syzbot+3b4acab09b6463472d0a@syzkaller.appspotmail.com>; Daniel > > > Jurgens <danielj@mellanox.com>; dledford@redhat.com; Johannes Berg > > > <johannes.berg@intel.com>; syzkaller-bugs@googlegroups.com > > > Subject: Re: [PATCH rdma-rc] RDMA/rdma_cm: Fix use after free race > > > with process_one_req > > > > > > On Thu, Mar 22, 2018 at 09:30:37PM +0000, Parav Pandit wrote: > > > > > > > flush_workqueue() will force to execute all the work items for all > > > > pending entries, all must have to completed. Those pending > > > > delayed entries are unrelated to this work item/request in > > > > progress, and if they are large number of entries having 1 sec > > > > timeout, > > > > flush_workqueue() might take long. > > > > > > OK then > > > > > > > So one rdma_destroy_id will wait for other requests to be > > > > completed, which I think we should avoid. > > > > > > I looked at this for a bit.. I really don't like how this code works. > > > > > > The idea that rdma_destroy_id() doesn't fence the callback is a bad > > > design, but doesn't apparently cause any bug I can see. > > > > > > I also can't understand why the rdma_addr_client nonsense should > > > exist, it seems to be rolled into the idea that cancel doesn't > > > actually cancel. :( > > > > > > So lets just use the one line patch and save the rest for some other day.. > > > > Ok. A helper function in the hunk is preferred as the code is same in > > both the functions. I will test it on Friday once. > > Now that I've looked at this closely enough to find the syzkaller bug, my > preference is to tidy up the whole thing: > > https://github.com/jgunthorpe/linux/commits/cma-fix > > - Remove duplication, only one function processes work. Get rid > of hold over list sorting, use the workqueue infrastructure itself > - Make rdma_addr_cancel() a sane and safe API by having it fence > - Get rid of the totally useless struct rdma_add_client and related > > Which is a pretty sweet little cleanup: > > drivers/infiniband/core/addr.c | 132 +++++++++++++++++++++++++++++++------ > ------------------------------------------------------------ > drivers/infiniband/core/cma.c | 6 +---- > include/rdma/ib_addr.h | 20 +-------------- > 3 files changed, 44 insertions(+), 114 deletions(-) > > What do you think Parav? > I added comments inline in the github. -- 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, Mar 23, 2018 at 04:56:57AM +0000, Parav Pandit wrote: > > What do you think Parav? > > > I added comments inline in the github. Yep, thanks, that spinlock thing is good. I updated it again, if you like it, then I can send it through Mellanox QA next week, no rush. https://github.com/jgunthorpe/linux/commits/cma-fix But I will apply the one liner for syzkaller, right? https://patchwork.kernel.org/patch/10302205/ 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/addr.c b/drivers/infiniband/core/addr.c index b0a52c9..0679f4f 100644 --- a/drivers/infiniband/core/addr.c +++ b/drivers/infiniband/core/addr.c @@ -561,6 +561,23 @@ static int addr_resolve(struct sockaddr *src_in, return ret; } +static void complete_addr_req(struct addr_req *req) +{ + /* + * Although the work will normally have been canceled by the + * workqueue, it can still be requeued as long as it is on the + * req_list by rdma_addr_cancel(), so it could have been requeued + * before we grabbed &lock. + * We need to cancel it after it is removed from req_list to really be + * sure it is safe to free. + */ + cancel_delayed_work(&req->work); + req->callback(req->status, (struct sockaddr *) &req->src_addr, + req->addr, req->context); + put_client(req->client); + kfree(req); +} + static void process_one_req(struct work_struct *_work) { struct addr_req *req; @@ -586,10 +603,7 @@ static void process_one_req(struct work_struct *_work) list_del(&req->list); mutex_unlock(&lock); - req->callback(req->status, (struct sockaddr *)&req->src_addr, - req->addr, req->context); - put_client(req->client); - kfree(req); + complete_addr_req(req); } static void process_req(struct work_struct *work) @@ -621,15 +635,7 @@ static void process_req(struct work_struct *work) list_for_each_entry_safe(req, temp_req, &done_list, list) { list_del(&req->list); - /* It is safe to cancel other work items from this work item - * because at a time there can be only one work item running - * with this single threaded work queue. - */ - cancel_delayed_work(&req->work); - req->callback(req->status, (struct sockaddr *) &req->src_addr, - req->addr, req->context); - put_client(req->client); - kfree(req); + complete_addr_req(req); } }