On Wed, Feb 20, 2019 at 04:20:45PM -0800, Matthew Wilcox wrote:
> The word 'idr' is scattered throughout the API, so I haven't changed it,
> but the 'idr' variable is now an XArray.
>
> Signed-off-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> drivers/infiniband/core/rdma_core.c | 78 +++++++----------------------
> drivers/infiniband/core/uverbs.h | 4 +-
> 2 files changed, 18 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> index 6c4747e61d2b..1084685ddce7 100644
> +++ b/drivers/infiniband/core/rdma_core.c
> @@ -296,25 +296,8 @@ static struct ib_uobject *alloc_uobj(struct ib_uverbs_file *ufile,
>
> static int idr_add_uobj(struct ib_uobject *uobj)
> {
> - int ret;
> -
> - idr_preload(GFP_KERNEL);
> - spin_lock(&uobj->ufile->idr_lock);
> -
> - /*
> - * We start with allocating an idr pointing to NULL. This represents an
> - * object which isn't initialized yet. We'll replace it later on with
> - * the real object once we commit.
> - */
> - ret = idr_alloc(&uobj->ufile->idr, NULL, 0,
> - min_t(unsigned long, U32_MAX - 1, INT_MAX), GFP_NOWAIT);
> - if (ret >= 0)
> - uobj->id = ret;
> -
> - spin_unlock(&uobj->ufile->idr_lock);
> - idr_preload_end();
> -
> - return ret < 0 ? ret : 0;
> + 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
xa_alloc(&id, XA_ZERO_ENTRY)
copy_to_user(id)
xa_store(id, uobj);
Where the xa_store must only happen if the copy_to_user succeeded.
> }
>
> /* Returns the ib_uobject or an error. The caller should check for IS_ERR. */
> @@ -324,29 +307,14 @@ lookup_get_idr_uobject(const struct uverbs_api_object *obj,
> enum rdma_lookup_mode mode)
> {
> struct ib_uobject *uobj;
> - unsigned long idrno = id;
>
> - if (id < 0 || id > ULONG_MAX)
> + if ((u64)id > ULONG_MAX)
> return ERR_PTR(-EINVAL);
'id' is controlled by userspace, negative values in the s64 are
strictly forbidden, so this isn't an equivalent transformation.
> @@ -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?
IMHO it would be nice to have a xa_store_reserved() idea which returns
void and WARNS_ON if it is mis-used.
Jason
