On Thu, Feb 21, 2019 at 11:06:58AM -0800, Matthew Wilcox wrote:
> 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).
In that case the error code has to be returned here..
But, IMHO, a major reason to use xa_reserve is to create a situation
where store can't fail, so it is disappointing to hear that isn't
possible anymore.
Jason
