Re: [PATCH 04/10] scsi: iscsi: Fix endpoint reuse regression

[Lee Duncan <lduncan@suse.com>]

On 4/7/22 17:13, Mike Christie wrote:
> This patch fixes a bug where when using iscsi offload we can free a
> endpoint while userspace still thinks it's active. That then causes the
> endpoint ID to be reused for a new connection's endpoint while userspace
> still thinks the ID is for the original connection. Userspace will then
> end up disconnecting a running connection's endpoint or trying to bind
> to another connection's endpoint.
> 
> This bug is a regression added in:
> 
> Commit 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")
> 
> where we added a in kernel ep_disconnect call to fix a bug in:
> 
> Commit 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely in
> kernel space")
> 
> where we would call stop_conn without having done ep_disconnect. This
> early ep_disconnect call will then free the endpoint and it's ID while
> userspace still thinks the ID is valid.
> 
> This patch fixes the early release of the ID by having the in kernel
> recovery code keep a reference to the endpoint until userspace has called
> into the kernel to finish cleaning up the endpoint/connection. It requires
> the previous patch "scsi: iscsi: Release endpoint ID when its freed."
> which moved the freeing of the ID until when the endpoint is released.
> 
> Fixes: 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/scsi_transport_iscsi.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 1fc7c6bfbd67..f200da049f3b 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2247,7 +2247,11 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
>   		mutex_unlock(&conn->ep_mutex);
>   
>   		flush_work(&conn->cleanup_work);
> -
> +		/*
> +		 * Userspace is now done with the EP so we can release the ref
> +		 * iscsi_cleanup_conn_work_fn took.
> +		 */
> +		iscsi_put_endpoint(ep);
>   		mutex_lock(&conn->ep_mutex);
>   	}
>   }
> @@ -2322,6 +2326,12 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
>   		return;
>   	}
>   
> +	/*
> +	 * Get a ref to the ep, so we don't release its ID until after
> +	 * userspace is done referencing it in iscsi_if_disconnect_bound_ep.
> +	 */
> +	if (conn->ep)
> +		get_device(&conn->ep->dev);
>   	iscsi_ep_disconnect(conn, false);
>   
>   	if (system_state != SYSTEM_RUNNING) {

Reviewed-by: Lee Duncan <lduncan@suse.com>