linux-kernel - Re: [PATCH rdma-next v3 8/8] IB/cm: Protect cm_dev, cm_ports and mad

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210525200057.GA3469742@nvidia.com>
Date:   Tue, 25 May 2021 17:00:57 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     Doug Ledford <dledford@...hat.com>,
        Mark Zhang <markzhang@...dia.com>,
        linux-kernel@...r.kernel.org, linux-rdma@...r.kernel.org,
        Sean Hefty <sean.hefty@...el.com>
Subject: Re: [PATCH rdma-next v3 8/8] IB/cm: Protect cm_dev, cm_ports and
 mad_agent with kref and lock

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