Skip to content

Commit

Permalink
scsi: iscsi: Fix unbound endpoint error handling
Browse files Browse the repository at this point in the history 
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 23d6fef ("scsi: iscsi: Fix in-kernel conn failure handling")

and:

Commit 0ab7104 ("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.

Link: https://lore.kernel.org/r/[email protected]
Fixes: 0ab7104 ("scsi: iscsi: Perform connection failure entirely in kernel space")
Tested-by: Manish Rangankar <[email protected]>
Reviewed-by: Lee Duncan <lduncan@@suse.com>
Reviewed-by: Chris Leech <[email protected]>
Signed-off-by: Mike Christie <[email protected]>
Signed-off-by: Martin K. Petersen <[email protected]>
  • Loading branch information
@mikechristie @martinkpetersen
mikechristie authored and martinkpetersen committed Apr 12, 2022
1 parent 7c6e99c commit 03690d8
Showing 1 changed file with 36 additions and 29 deletions.
65 changes: 36 additions & 29 deletions drivers/scsi/scsi_transport_iscsi.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down

0 comments on commit 03690d8

Please sign in to comment.