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
--- a/drivers/infiniband/core/ucma.c
+++ 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);
Thanks
>
> Jason
Attachment:
signature.asc
Description: PGP signature
