On Thu, Apr 07, 2022 at 07:13:10PM -0500, Mike Christie wrote:
> If a driver raises a connection error before the connection is bound, we
> can leave a cleanup_work queued that can later run and disconnect/stop a
> connection that is logged in. The problem is that drivers can call
> iscsi_conn_error_event for endpoints that are connected but not yet bound
> when something like the network port they are using is brought down.
> iscsi_cleanup_conn_work_fn will check for this and exit early, but if the
> cleanup_work is stuck behind other works, it might not get run until after
> userspace has done ep_disconnect. Because the endpoint is not yet bound
> there was no way for ep_disconnect to flush the work.
>
> The bug of leaving stop_conns queued was added in:
>
> Commit 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")
>
> and:
>
> Commit 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely in
> kernel space")
>
> was supposed to fix it, but left this case.
>
> This patch moves the conn state check to before we even queue the work
> so we can avoid queueing.
>
> Fixes: 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely in
> kernel space")
> Signed-off-by: Mike Christie <michael.christie@xxxxxxxxxx>
> ---
> drivers/scsi/scsi_transport_iscsi.c | 65 ++++++++++++++++-------------
> 1 file changed, 36 insertions(+), 29 deletions(-)
Reviewed-by: Chris Leech <cleech@xxxxxxxxxx>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 63a4f0c022fd..2c0dd64159b0 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2201,10 +2201,10 @@ static void iscsi_stop_conn(struct iscsi_cls_conn *conn, int flag)
>
> switch (flag) {
> case STOP_CONN_RECOVER:
> - conn->state = ISCSI_CONN_FAILED;
> + WRITE_ONCE(conn->state, ISCSI_CONN_FAILED);
> break;
> case STOP_CONN_TERM:
> - conn->state = ISCSI_CONN_DOWN;
> + WRITE_ONCE(conn->state, ISCSI_CONN_DOWN);
> break;
> default:
> iscsi_cls_conn_printk(KERN_ERR, conn, "invalid stop flag %d\n",
> @@ -2222,7 +2222,7 @@ static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
> struct iscsi_endpoint *ep;
>
> ISCSI_DBG_TRANS_CONN(conn, "disconnect ep.\n");
> - conn->state = ISCSI_CONN_FAILED;
> + WRITE_ONCE(conn->state, ISCSI_CONN_FAILED);
>
> if (!conn->ep || !session->transport->ep_disconnect)
> return;
> @@ -2321,21 +2321,6 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
> struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
>
> mutex_lock(&conn->ep_mutex);
> - /*
> - * If we are not at least bound there is nothing for us to do. Userspace
> - * will do a ep_disconnect call if offload is used, but will not be
> - * doing a stop since there is nothing to clean up, so we have to clear
> - * the cleanup bit here.
> - */
> - if (conn->state != ISCSI_CONN_BOUND && conn->state != ISCSI_CONN_UP) {
> - ISCSI_DBG_TRANS_CONN(conn, "Got error while conn is already failed. Ignoring.\n");
> - spin_lock_irq(&conn->lock);
> - clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
> - spin_unlock_irq(&conn->lock);
> - mutex_unlock(&conn->ep_mutex);
> - 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.
> @@ -2391,7 +2376,7 @@ iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
> INIT_WORK(&conn->cleanup_work, iscsi_cleanup_conn_work_fn);
> conn->transport = transport;
> conn->cid = cid;
> - conn->state = ISCSI_CONN_DOWN;
> + WRITE_ONCE(conn->state, ISCSI_CONN_DOWN);
>
> /* this is released in the dev's release function */
> if (!get_device(&session->dev))
> @@ -2590,10 +2575,30 @@ void iscsi_conn_error_event(struct iscsi_cls_conn *conn, enum iscsi_err error)
> struct iscsi_internal *priv;
> int len = nlmsg_total_size(sizeof(*ev));
> unsigned long flags;
> + int state;
>
> spin_lock_irqsave(&conn->lock, flags);
> - if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags))
> - queue_work(iscsi_conn_cleanup_workq, &conn->cleanup_work);
> + /*
> + * Userspace will only do a stop call if we are at least bound. And, we
> + * only need to do the in kernel cleanup if in the UP state so cmds can
> + * be released to upper layers. If in other states just wait for
> + * userspace to avoid races that can leave the cleanup_work queued.
> + */
> + state = READ_ONCE(conn->state);
> + switch (state) {
> + case ISCSI_CONN_BOUND:
> + case ISCSI_CONN_UP:
> + if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP,
> + &conn->flags)) {
> + queue_work(iscsi_conn_cleanup_workq,
> + &conn->cleanup_work);
> + }
> + break;
> + default:
> + ISCSI_DBG_TRANS_CONN(conn, "Got conn error in state %d\n",
> + state);
> + break;
> + }
> spin_unlock_irqrestore(&conn->lock, flags);
>
> priv = iscsi_if_transport_lookup(conn->transport);
> @@ -2944,7 +2949,7 @@ iscsi_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev)
> char *data = (char*)ev + sizeof(*ev);
> struct iscsi_cls_conn *conn;
> struct iscsi_cls_session *session;
> - int err = 0, value = 0;
> + int err = 0, value = 0, state;
>
> if (ev->u.set_param.len > PAGE_SIZE)
> return -EINVAL;
> @@ -2961,8 +2966,8 @@ iscsi_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev)
> session->recovery_tmo = value;
> break;
> default:
> - if ((conn->state == ISCSI_CONN_BOUND) ||
> - (conn->state == ISCSI_CONN_UP)) {
> + state = READ_ONCE(conn->state);
> + if (state == ISCSI_CONN_BOUND || state == ISCSI_CONN_UP) {
> err = transport->set_param(conn, ev->u.set_param.param,
> data, ev->u.set_param.len);
> } else {
> @@ -3758,7 +3763,7 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
> ev->u.b_conn.transport_eph,
> ev->u.b_conn.is_leading);
> if (!ev->r.retcode)
> - conn->state = ISCSI_CONN_BOUND;
> + WRITE_ONCE(conn->state, ISCSI_CONN_BOUND);
>
> if (ev->r.retcode || !transport->ep_connect)
> break;
> @@ -3777,7 +3782,8 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
> case ISCSI_UEVENT_START_CONN:
> ev->r.retcode = transport->start_conn(conn);
> if (!ev->r.retcode)
> - conn->state = ISCSI_CONN_UP;
> + WRITE_ONCE(conn->state, ISCSI_CONN_UP);
> +
> break;
> case ISCSI_UEVENT_SEND_PDU:
> pdu_len = nlh->nlmsg_len - sizeof(*nlh) - sizeof(*ev);
> @@ -4084,10 +4090,11 @@ static ssize_t show_conn_state(struct device *dev,
> {
> struct iscsi_cls_conn *conn = iscsi_dev_to_conn(dev->parent);
> const char *state = "unknown";
> + int conn_state = READ_ONCE(conn->state);
>
> - if (conn->state >= 0 &&
> - conn->state < ARRAY_SIZE(connection_state_names))
> - state = connection_state_names[conn->state];
> + if (conn_state >= 0 &&
> + conn_state < ARRAY_SIZE(connection_state_names))
> + state = connection_state_names[conn_state];
>
> return sysfs_emit(buf, "%s\n", state);
> }
> --
> 2.25.1
>
