On Tue, Aug 27, 2019 at 07:28:14PM +0300, Yuval Shaia wrote:
> 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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > Signed-off-by: Shamir Rabinovitch <srabinov7@xxxxxxxxx>
> > > 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)
Hurm. That refcount is kind of broken/racey as is, I'm not clear if it
can be used for this. More changes would probably be needed..
It would be more understandable to start with a dedicated refcount and
then have a patch to consolidate
> > > +
> > > + /*
> > > + * 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.
I mean, you should just extract it from the hw_object directly, some
how. Ie have a get_refcount accessor in the object definition.
Jason
