On Tue, Mar 20, 2018 at 06:47:36PM +0200, Leon Romanovsky wrote:
> On Tue, Mar 20, 2018 at 10:34:48AM -0600, Jason Gunthorpe wrote:
> > On Tue, Mar 20, 2018 at 04:18:40PM +0000, Hefty, Sean wrote:
> > > > ctx->uid = cmd.uid;
> > > > - ctx->cm_id = rdma_create_id(current->nsproxy->net_ns,
> > > > - ucma_event_handler, ctx, cmd.ps, qp_type);
> > > > - if (IS_ERR(ctx->cm_id)) {
> > > > - ret = PTR_ERR(ctx->cm_id);
> > > > + cm_id = rdma_create_id(current->nsproxy->net_ns,
> > > > + ucma_event_handler, ctx, cmd.ps, qp_type);
> > > > + if (IS_ERR(cm_id)) {
> > > > + ret = PTR_ERR(cm_id);
> > > > goto err1;
> > > > }
> > > > + ctx->cm_id = cm_id;
> > > >
> > > > resp.id = ctx->id;
> > > > if (copy_to_user((void __user *)(unsigned long)cmd.response,
> > >
> > > If copy_to_user() fails, we destroy the cm_id and ctx. But there's
> > > a gap immediately before that where user space could conceivably
> > > acquire the ctx and start using it. If we don't set ctx->cm_id
> > > until after copy_to_user() succeeds, then I think we close any races
> > > (with your fix in get_ctx to verify that cm_id is not NULL).
> >
> > Agree.
> >
> > It is clearly wrong to publish the pointer to other threads without
> > holding the ref and going through the wait_completion stuff prior to
> > destroy.
> >
> > We can avoid doing that by moving the 'commit' stage to the end.
>
> Jason,
>
> Can you please apply this fixup?
>
> diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
> index 1be62311b105..e5a1e7d81326 100644
> +++ b/drivers/infiniband/core/ucma.c
> @@ -483,7 +483,6 @@ static ssize_t ucma_create_id(struct ucma_file *file, const char __user *inbuf,
> ret = PTR_ERR(cm_id);
> goto err1;
> }
> - ctx->cm_id = cm_id;
>
> resp.id = ctx->id;
> if (copy_to_user((void __user *)(unsigned long)cmd.response,
> @@ -491,10 +490,12 @@ static ssize_t ucma_create_id(struct ucma_file *file, const char __user *inbuf,
> ret = -EFAULT;
> goto err2;
> }
> +
> + ctx->cm_id = cm_id;
> return 0;
>
> err2:
> - rdma_destroy_id(ctx->cm_id);
> + rdma_destroy_id(cm_id);
> err1:
> mutex_lock(&mut);
> idr_remove(&ctx_idr, ctx->id);
Okay, done and applied to for-rc
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
