Hi Jason, > -----Original Message----- > From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- > owner@xxxxxxxxxxxxxxx] On Behalf Of Jason Gunthorpe > Sent: Thursday, March 22, 2018 11:03 PM > To: Parav Pandit <parav@xxxxxxxxxxxx> > Cc: linux-rdma@xxxxxxxxxxxxxxx; Leon Romanovsky <leonro@xxxxxxxxxxxx>; > Mark Bloch <markb@xxxxxxxxxxxx>; Dmitry Vyukov <dvyukov@xxxxxxxxxx>; > syzbot <syzbot+3b4acab09b6463472d0a@xxxxxxxxxxxxxxxxxxxxxxxxx>; Daniel > Jurgens <danielj@xxxxxxxxxxxx>; dledford@xxxxxxxxxx; Johannes Berg > <johannes.berg@xxxxxxxxx>; syzkaller-bugs@xxxxxxxxxxxxxxxx > 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@xxxxxxxx] > > > Sent: Thursday, March 22, 2018 5:31 PM > > > To: Parav Pandit <parav@xxxxxxxxxxxx> > > > Cc: linux-rdma@xxxxxxxxxxxxxxx; Leon Romanovsky > > > <leonro@xxxxxxxxxxxx>; Mark Bloch <markb@xxxxxxxxxxxx>; Dmitry > > > Vyukov <dvyukov@xxxxxxxxxx>; syzbot > > > <syzbot+3b4acab09b6463472d0a@xxxxxxxxxxxxxxxxxxxxxxxxx>; Daniel > > > Jurgens <danielj@xxxxxxxxxxxx>; dledford@xxxxxxxxxx; Johannes Berg > > > <johannes.berg@xxxxxxxxx>; syzkaller-bugs@xxxxxxxxxxxxxxxx > > > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
