Re: [PATCH v1 05/24] IB/core: ib_uobject need HW object reference count

[Yuval Shaia <yuval.shaia@oracle.com>]

On Wed, Aug 21, 2019 at 02:53:29PM +0000, Jason Gunthorpe wrote:
> On Wed, Aug 21, 2019 at 05:21:06PM +0300, Yuval Shaia wrote:
> > From: Shamir Rabinovitch <shamir.rabinovitch@oracle.com>
> > 
> > This new refcnt will points to the refcnt member of the HW object and will
> > behaves as expected by refcnt, i.e. will be increased and decreased as a
> > result of usage changes and will destroy the object when reaches to zero.
> > For a non-shared object refcnt will remain NULL.
> > 
> > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com>
> > Signed-off-by: Shamir Rabinovitch <srabinov7@gmail.com>
> >  drivers/infiniband/core/rdma_core.c | 23 +++++++++++++++++++++--
> >  include/rdma/ib_verbs.h             |  7 +++++++
> >  2 files changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> > index ccf4d069c25c..651625f632d7 100644
> > +++ b/drivers/infiniband/core/rdma_core.c
> > @@ -516,7 +516,26 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
> >  	const struct uverbs_obj_idr_type *idr_type =
> >  		container_of(uobj->uapi_object->type_attrs,
> >  			     struct uverbs_obj_idr_type, type);
> > -	int ret = idr_type->destroy_object(uobj, why, attrs);
> > +	static DEFINE_MUTEX(lock);
> > +	int ret, count;
> > +
> > +	mutex_lock(&lock);
> > +
> > +	if (uobj->refcnt) {
> > +		count = atomic_dec_return(uobj->refcnt);
> > +		WARN_ON(count < 0); /* use after free! */
> 
> Use a proper refcount_t

uobj->refcnt points to HW object's refcnt (e.x ib_pd.refcnt)

> 
> > +		if (count) {
> > +			mutex_unlock(&lock);
> > +			goto skip;
> > +		}
> > +	}
> > +
> > +	ret = idr_type->destroy_object(uobj, why, attrs);
> > +
> > +	if (ret)
> > +		atomic_inc(uobj->refcnt);
> > +
> > +	mutex_unlock(&lock);
> >  
> >  	/*
> >  	 * We can only fail gracefully if the user requested to destroy the
> > @@ -525,7 +544,7 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
> >  	 */
> >  	if (ib_is_destroy_retryable(ret, why, uobj))
> >  		return ret;
> > -
> > +skip:
> 
> This doesn't seem to properly define who owns the rdmacg count - it
> should belong to the HW object not to the uobject.
> 
> > +
> > +	/*
> > +	 * ib_X HW object sharing support
> > +	 * - NULL for HW objects that are not shareable
> > +	 * - Pointer to ib_X reference counter for shareable HW objects
> > +	 */
> > +	atomic_t	       *refcnt;		/* ib_X object ref count */
> 
> Gross, shouldn't this actually be in the hw object?

It is belongs to the HW object, this one just *points* to it and it is
defined here since we like to maintain it when destroy_hw_idr_uobject is
called.

The actual assignment is only for object that *supports* import and it is
set in function ib_uverbs_import_ib_pd (patch "IB/uverbs: Add PD import
verb").

Hope it make sense.

> 
> Jason