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

From: Jason Gunthorpe <jgg@nvidia.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>,
	Mark Zhang <markzhang@nvidia.com>,
	linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
	Sean Hefty <sean.hefty@intel.com>
Subject: Re: [PATCH rdma-next v3 8/8] IB/cm: Protect cm_dev, cm_ports and mad_agent with kref and lock
Date: Tue, 25 May 2021 17:00:57 -0300	[thread overview]
Message-ID: <20210525200057.GA3469742@nvidia.com> (raw)
In-Reply-To: <7ca9e316890a3755abadefdd7fe3fc1dc4a1e79f.1620720467.git.leonro@nvidia.com>

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