On Mon, Mar 19, 2018 at 02:18:24PM -0600, Jason Gunthorpe wrote: > On Mon, Mar 19, 2018 at 07:24:11PM +0000, Hefty, Sean wrote: > > > > > +++ b/drivers/infiniband/core/ucma.c > > > > > @@ -132,7 +132,7 @@ static inline struct ucma_context > > > > > *_ucma_find_context(int id, > > > > > ctx = idr_find(&ctx_idr, id); > > > > > if (!ctx) > > > > > ctx = ERR_PTR(-ENOENT); > > > > > - else if (ctx->file != file) > > > > > + else if (ctx->file != file || !ctx->cm_id) > > > > > > > > After looking at the use-after-free patch, we may want this check to > > > > be > > > > > > > > !IS_ERR_OR_NULL(ctx->cm_id) > > > > > > Please don't store ERR_PTR outside stack variables, just makes > > > everything harder :( > > > > A NULL check should be fine if the ctx->cm_id is not set until > > immediately before returning from ucma_create_id(). The two bugs > > that Leon is addressing are related, and ERR_PTR looks possible with > > the current (unpatched) code. > > Well, even better would be to not set the IDR until the object is > ready to go, then the existing check works as-is. A two stage add > where we add NULL to the idr to atomically reserve the ID then once > done commit by updating the IDR index with the valid pointer. ucma_alloc_ctx() is called twice, I don't think that it is worth effort to refactor code to make two functions instead of one. Thanks > > Jason
Attachment:
signature.asc
Description: PGP signature
