Re: [PATCH rdma-next v5 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: 1489 bytes --]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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