On Tue, Jan 23, 2018 at 10:54:33AM -0700, Jason Gunthorpe wrote:
> On Mon, Jan 22, 2018 at 02:51:14PM +0200, Leon Romanovsky wrote:
> > + mutex_lock(&dev->res.mutex);
> > + hash_del_rcu(&res->node);
> > + mutex_unlock(&dev->res.mutex);
> > +
> > + res->valid = false;
> > +
> > + if (res->task)
> > + put_task_struct(res->task);
> > + synchronize_srcu(&dev->res.srcu);
> > +}
>
> This locking is wrong..
>
> Nothing can invalidate internal elements of 'res' until
> synchronize_srcu() returns, otherwise it creates races.
>
> Eg here:
>
> + key = srcu_read_lock(&device->res.srcu);
> + hash_for_each_possible_rcu(device->res.hash, res, node, RDMA_RESTRACK_QP) {
> + if (idx < start) {
> + idx++;
> + continue;
> + }
> +
> + if ((rdma_is_kernel_res(res) &&
> + task_active_pid_ns(current) != &init_pid_ns) ||
> + (!rdma_is_kernel_res(res) &&
> + task_active_pid_ns(current) != task_active_pid_ns(res->task)))
>
> Will use-after-put res->task
>
> Didn't audit closely enough to tell if valid is OK or not. Why isn't
> the hash_del sufficient? Why does valid exist?
Because, there are mlx5 QPs which partially flows through core code to register
and deregister. Such QPs were created "manually" but destroyed with ib_destroy_qp.
>
> 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
Attachment:
signature.asc
Description: PGP signature
