On Mon, Jun 4, 2018 at 6:46 PM, Jason Gunthorpe <[email protected]> wrote: > On Mon, Jun 04, 2018 at 06:23:20PM +0200, Gi-Oh Kim wrote: >> On Fri, Jun 1, 2018 at 8:31 PM, Cong Wang <[email protected]> wrote: >> > ucma_process_join() will free the new allocated "mc" struct, >> > if there is any error after that, especially the copy_to_user(). >> > >> > But in parallel, ucma_leave_multicast() could find this "mc" >> > through idr_find() before ucma_process_join() frees it, since it >> > is already published. >> > >> > So "mc" could be used in ucma_leave_multicast() after it is been >> > allocated and freed in ucma_process_join(), since we don't refcnt >> > it. >> > >> > Fix this by separating "publish" from ID allocation, so that we >> > can get an ID first and publish it later after copy_to_user(). >> > >> > Fixes c8f6a362bf3e ("RDMA/cma: Add multicast communication support") >> > Reported-by: Noam Rathaus <[email protected]> >> > Cc: Sean Hefty <[email protected]> >> > Cc: Doug Ledford <[email protected]> >> > Cc: Jason Gunthorpe <[email protected]> >> > Cc: [email protected] >> > Signed-off-by: Cong Wang <[email protected]> >> > drivers/infiniband/core/ucma.c | 6 +++++- >> > 1 file changed, 5 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/infiniband/core/ucma.c >> > b/drivers/infiniband/core/ucma.c >> > index eab43b17e9cf..ec8fb289621f 100644 >> > +++ b/drivers/infiniband/core/ucma.c >> > @@ -235,7 +235,7 @@ static struct ucma_multicast* >> > ucma_alloc_multicast(struct ucma_context *ctx) >> > return NULL; >> > >> > mutex_lock(&mut); >> > - mc->id = idr_alloc(&multicast_idr, mc, 0, 0, GFP_KERNEL); >> > + mc->id = idr_alloc(&multicast_idr, NULL, 0, 0, GFP_KERNEL); >> > mutex_unlock(&mut); >> > if (mc->id < 0) >> > goto error; >> > @@ -1421,6 +1421,10 @@ static ssize_t ucma_process_join(struct ucma_file >> > *file, >> > goto err3; >> > } >> > >> > + mutex_lock(&mut); >> > + idr_replace(&multicast_idr, mc, mc->id); >> > + mutex_unlock(&mut); >> > + >> > mutex_unlock(&file->mut); >> > ucma_put_ctx(ctx); >> > return 0; >> > >> >> >> Hi, >> >> Your patch is reasonable to me. >> Can I ask a question for that? >> Could it be solved by asymmetric locking as following? > > No, there are many other paths that touch multicast_idr that don't > hold both locks, we should protect all of them from accessing an > incompletely initialized structure. > > Jason
Understood. Thank you. -- GIOH KIM Linux Kernel Entwickler ProfitBricks GmbH Greifswalder Str. 207 D - 10405 Berlin Tel: +49 176 2697 8962 Fax: +49 30 577 008 299 Email: [email protected] URL: https://www.profitbricks.de Sitz der Gesellschaft: Berlin Registergericht: Amtsgericht Charlottenburg, HRB 125506 B Geschäftsführer: Achim Weiss, Matthias Steinberg, Christoph Steffens
