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

Lee Duncan
2022-04-08T17:41:03Z

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>