On Thu, Feb 21, 2019 at 11:37:02AM -0700, Jason Gunthorpe wrote:
> > + return xa_alloc(&uobj->ufile->idr, &uobj->id, uobj, xa_limit_32b,
> > + GFP_KERNEL);
>
> This really does need to store NULL here. We can't allow
> lookup_get_idr_uobject to see the pointer until the object is
> compelted initialized.. The basic high level flow is something like
Braino on my part. I remember looking at this, analysing the flow and
concluding it really did need to store NULL there ... and then my fingers
wrote uobj instead. Fixed in my tree.
> > - if (id < 0 || id > ULONG_MAX)
> > + if ((u64)id > ULONG_MAX)
>
> 'id' is controlled by userspace, negative values in the s64 are
> strictly forbidden, so this isn't an equivalent transformation.
Yet another braino. I meant to type:
if ((u64)id > LONG_MAX)
> > @@ -587,16 +549,11 @@ static int alloc_commit_idr_uobject(struct ib_uobject *uobj)
> > {
> > struct ib_uverbs_file *ufile = uobj->ufile;
> >
> > - spin_lock(&ufile->idr_lock);
> > /*
> > - * We already allocated this IDR with a NULL object, so
> > - * this shouldn't fail.
> > - *
> > - * NOTE: Once we set the IDR we loose ownership of our kref on uobj.
> > + * NOTE: Storing the uobj transfers our kref on uobj to the XArray.
> > * It will be put by remove_commit_idr_uobject()
> > */
> > - WARN_ON(idr_replace(&ufile->idr, uobj, uobj->id));
> > - spin_unlock(&ufile->idr_lock);
> > + xa_store(&ufile->idr, uobj->id, uobj, 0);
>
> What does the GFP flags == 0 do? I've seen you use this as a marker
> for stores that cannot fail?
It's for stores which we know we're just overwriting, so there won't
be any need to allocate memory. But I've started to move away from
this idea -- it's a quirk of the current implementation that it won't
need to allocate memory, and not even guaranteed true by the current
implementation (if you try to store a 2-byte aligned pointer in index 0,
it will have to allocate memory).
So I think I'm actually going to replace this 0 with a GFP_KERNEL (I just
checked the calling context, and it can absolutely sleep to allocate
memory).
