Re: [PATCH rdma-next v3 8/8] IB/cm: Protect cm_dev, cm_ports and mad_agent with kref and lock

[Jason Gunthorpe]

On Tue, May 11, 2021 at 11:22:12AM +0300, Leon Romanovsky wrote:
> @@ -2139,6 +2197,8 @@ static int cm_req_handler(struct cm_work *work)
>  		sa_path_set_dmac(&work->path[0],
>  				 cm_id_priv->av.ah_attr.roce.dmac);
>  	work->path[0].hop_limit = grh->hop_limit;
> +
> +	cm_destroy_av(&cm_id_priv->av);
>  	ret = cm_init_av_by_path(&work->path[0], gid_attr, &cm_id_priv->av);
>  	if (ret) {
>  		int err;

Why add cm_destroy_av() here? The cm_id_priv was freshly created at
the top of this function and hasn't left the stack frame yet?

> @@ -4419,12 +4486,19 @@ static void cm_remove_one(struct ib_device *ib_device, void *client_data)
>  		 * after that we can call the unregister_mad_agent
>  		 */
>  		flush_workqueue(cm.wq);
> -		ib_unregister_mad_agent(port->mad_agent);
> +		/*
> +		 * The above ensures no call paths from the work are running,
> +		 * the remaining paths all take the unregistration lock

"unregistration lock" is "mad_agent_lock"

> +		 */
> +		spin_lock(&cm_dev->mad_agent_lock);
> +		port->mad_agent = NULL;
> +		spin_unlock(&cm_dev->mad_agent_lock);
> +		ib_unregister_mad_agent(mad_agent);
>  		cm_remove_port_fs(port);
> -		kfree(port);
>  	}
>  
> -	kfree(cm_dev);
> +	/* All touches can only be on call path from the work */

Not sure anymore what this comment means, the work was flushed? I
think it is saying all touches can only be on a place outside the
work.

Other than these little details it all looks OK

Jason