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@xxxxxxxxxxxx>
> > +
> > +/**
> > + * 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:
signature.asc
Description: PGP signature
