On Thu, Jan 25, 2018 at 09:44:36PM +0200, Leon Romanovsky wrote:
> > > + /*
> > > + * @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.
I mean the kref isn't in any way linked the lifetime of the malloc
that contains it. So it isn't really a kref, it is just a refcount.
> > @@ -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.
It doesn't even do that if the kref count is elevated.
Which is the whole point, the rdma_restrack_del *doesn't* prevent the
use-after-free on the read side.
> > 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.
That doesn't help - the kref is still totally non-functional as a kref:
CPU0 CPU1
down_read()
rdma_restrack_get()
up_read()
down_write()
restrack_release()
up_write()
kfree(qp)
fill_res_qp_entry(qp)
// boom, use after free
A kref is not a lock, and a kref that *doesn't* put kfree in the
release function is probably not a kref but a refcount.
What I sent you wasn't remotely like this, it had two nested locks,
the outer mutex and a very special open coded inner read/write lock
that doesn't deadlock when nested..
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
