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? Jason -- 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
