| Message ID | CAJX1YtZNdwiwPmj5gGTqURjvLqa5g7Ly_h+TEZfNJSW=mrqRiQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Mon, Jun 4, 2018 at 9:23 AM, Gi-Oh Kim <gi-oh.kim@profitbricks.com> wrote: > Hi, > > Your patch is reasonable to me. > Can I ask a question for that? > Could it be solved by asymmetric locking as following? Maybe, if you are sure taking file->mut is safe in that place. Can you double check atomic_inc_not_zero(&mc->ctx->ref)? I am too far away from familiar with infiniband. ;) -- 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 Mon, Jun 04, 2018 at 06:23:20PM +0200, Gi-Oh Kim wrote: > On Fri, Jun 1, 2018 at 8:31 PM, Cong Wang <xiyou.wangcong@gmail.com> 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 <noamr@beyondsecurity.com> > > Cc: Sean Hefty <sean.hefty@intel.com> > > Cc: Doug Ledford <dledford@redhat.com> > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > Cc: linux-rdma@vger.kernel.org > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > 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 -- 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 Mon, Jun 4, 2018 at 6:46 PM, Jason Gunthorpe <jgg@ziepe.ca> 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 <xiyou.wangcong@gmail.com> 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 <noamr@beyondsecurity.com> >> > Cc: Sean Hefty <sean.hefty@intel.com> >> > Cc: Doug Ledford <dledford@redhat.com> >> > Cc: Jason Gunthorpe <jgg@ziepe.ca> >> > Cc: linux-rdma@vger.kernel.org >> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >> > 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.
diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c index eab43b17e9cf..d8b256baec31 100644 --- a/drivers/infiniband/core/ucma.c +++ b/drivers/infiniband/core/ucma.c @@ -1493,6 +1493,7 @@ static ssize_t ucma_leave_multicast(struct ucma_file *file, if (copy_from_user(&cmd, inbuf, sizeof(cmd))) return -EFAULT; + mutex_lock(&mc->ctx->file->mut); mutex_lock(&mut); mc = idr_find(&multicast_idr, cmd.id);