Re: [PATCH rdma-next v6 3/8] RDMA/restrack: Add general infrastructure to track RDMA resources

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]

On Thu, Jan 25, 2018 at 10:45:29AM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 25, 2018 at 05:12:22PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > +
> > +/**
> > + * struct rdma_restrack_entry - metadata per-entry
> > + */
> > +struct rdma_restrack_entry {
> > +	/**
> > +	 * @valid: validity indicator
> > +	 *
> > +	 * The entries are filled during rdma_restrack_add,
> > +	 * can be attempted to be free during rdma_restrack_del.
> > +	 *
> > +	 * As an example for that, see mlx5 QPs with type MLX5_IB_QPT_HW_GSI
> > +	 */
> > +	bool			valid;
> > +	/*
> > +	 * @kref: Protect destroy of the resource
> > +	 */
> > +	struct kref		kref;
>
> Sticking a kref in a random structure that is itself not malloc'd is a
> big red flag:

It is not "rand", but embedded part of ib_qp,ib_pd and ib_cq. It is
malloced as part of their creation.

>
> @@ -1746,6 +1756,11 @@ struct ib_qp {
> +       struct rdma_restrack_root     res;
>
>
> Not seeing how this works at all.
>
> qp does this:
>
> +       rdma_restrack_del(&qp->res);
>         ret = qp->device->destroy_qp(qp);
>
> And for instance the mlx5 implementation of destroy_qp() does:
>
> int mlx5_ib_destroy_qp(struct ib_qp *qp)
> {
>         struct mlx5_ib_qp *mqp = to_mqp(qp);
> [..]
>
>         kfree(mqp);
>
> So we kfree the kref containing memory outside the kref's release
> function. Super big red flag.

It is worth to look on the actual implementation, the
rdma_restrack_del() doesn't do a lot: removes from the list and marks
the resource as not valid.

>
> rdma_restrack_del is just:
>
> +       return kref_put(&res->kref, restrack_release);
>
> So we don't do anything to block progress to the kfree unless
> the kref drefs to 0. But the reader holds a get/put across its
> critical section so we never hit the 0 case when there is another
> user.
>
> Not even remotely right.
>
> Adding a kref means it has to be added to struct ib_qp and then kfree's
> in the drivers need to change to kref_put. That is alot of work.
>
> The best idea I've had to make this locking work is what I emailed you
> a few days ago..

It is more or less the same idea, but needs to move
down_write(&dev->res.rwsem) to be part of rdma_restrack_del and not
release routine. It will cause to block the progress of release.

Thanks

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]