From: Nicholas Bellinger nab@linux-iscsi.org

commit 21aaa23b0ebbd19334fa461370c03cbb076b3295 upstream.

This patch addresses a long standing race where obtaining se_node_acl->acl_kref in __transport_register_session() happens a bit too late, and leaves open the potential for core_tpg_del_initiator_node_acl() to hit a NULL pointer dereference.

Instead, take ->acl_kref in core_tpg_get_initiator_node_acl() while se_portal_group->acl_node_mutex is held, and move the final target_put_nacl() from transport_deregister_session() into transport_free_session() so that fabric driver login failure handling using the modern method to still work as expected.

Also, update core_tpg_get_initiator_node_acl() to take an extra reference for dynamically generated acls for demo-mode, before returning to fabric caller. Also update iscsi-target sendtargets special case handling to use target_tpg_has_node_acl() when checking if demo_mode_discovery == true during discovery lookup.

Note the existing wait_for_completion(&acl->acl_free_comp) in core_tpg_del_initiator_node_acl() does not change.

Cc: Sagi Grimberg sagig@mellanox.com Cc: Christoph Hellwig hch@lst.de Cc: Hannes Reinecke hare@suse.de Cc: Andy Grover agrover@redhat.com Cc: Mike Christie michaelc@cs.wisc.edu Signed-off-by: Nicholas Bellinger nab@linux-iscsi.org --- drivers/target/iscsi/iscsi_target.c | 2 +- drivers/target/target_core_tpg.c | 44 ++++++++++++++++++++++++++++++++-- drivers/target/target_core_transport.c | 18 +++++++++----- include/target/target_core_fabric.h | 2 ++ 4 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 7444640a..d6cb9e8 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -3449,7 +3449,7 @@ iscsit_build_sendtargets_response(struct iscsi_cmd *cmd,

if ((tpg->tpg_attrib.generate_node_acls == 0) && (tpg->tpg_attrib.demo_mode_discovery == 0) && - (!core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg, + (!target_tpg_has_node_acl(&tpg->tpg_se_tpg, cmd->conn->sess->sess_ops->InitiatorName))) { continue; } diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c index 47f0644..6c350f0 100644 --- a/drivers/target/target_core_tpg.c +++ b/drivers/target/target_core_tpg.c @@ -110,9 +110,21 @@ struct se_node_acl *core_tpg_get_initiator_node_acl( unsigned char *initiatorname) { struct se_node_acl *acl; - + /* + * Obtain se_node_acl->acl_kref using fabric driver provided + * initiatorname[] during node acl endpoint lookup driven by + * new se_session login. + * + * The reference is held until se_session shutdown -> release + * occurs via fabric driver invoked transport_deregister_session() + * or transport_free_session() code. + */ spin_lock_irq(&tpg->acl_node_lock); acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname); + if (acl) { + if (!kref_get_unless_zero(&acl->acl_kref)) + acl = NULL; + } spin_unlock_irq(&tpg->acl_node_lock);

return acl; @@ -254,6 +266,25 @@ static int core_create_device_list_for_node(struct se_node_acl *nacl) return 0; }

+bool target_tpg_has_node_acl(struct se_portal_group *tpg, + const char *initiatorname) +{ + struct se_node_acl *acl; + bool found = false; + + spin_lock_irq(&tpg->acl_node_lock); + list_for_each_entry(acl, &tpg->acl_node_list, acl_list) { + if (!strcmp(acl->initiatorname, initiatorname)) { + found = true; + break; + } + } + spin_unlock_irq(&tpg->acl_node_lock); + + return found; +} +EXPORT_SYMBOL(target_tpg_has_node_acl); + /* core_tpg_check_initiator_node_acl() * * @@ -274,10 +305,19 @@ struct se_node_acl *core_tpg_check_initiator_node_acl( acl = tpg->se_tpg_tfo->tpg_alloc_fabric_acl(tpg); if (!acl) return NULL; + /* + * When allocating a dynamically generated node_acl, go ahead + * and take the extra kref now before returning to the fabric + * driver caller. + * + * Note this reference will be released at session shutdown + * time within transport_free_session() code. + */ + kref_init(&acl->acl_kref); + kref_get(&acl->acl_kref);

INIT_LIST_HEAD(&acl->acl_list); INIT_LIST_HEAD(&acl->acl_sess_list); - kref_init(&acl->acl_kref); init_completion(&acl->acl_free_comp); spin_lock_init(&acl->device_list_lock); spin_lock_init(&acl->nacl_sess_lock); diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index b7d27b8..3bf46bf 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -359,7 +359,6 @@ void __transport_register_session( &buf[0], PR_REG_ISID_LEN); se_sess->sess_bin_isid = get_unaligned_be64(&buf[0]); } - kref_get(&se_nacl->acl_kref);

spin_lock_irq(&se_nacl->nacl_sess_lock); /* @@ -456,6 +455,7 @@ void target_put_nacl(struct se_node_acl *nacl) { kref_put(&nacl->acl_kref, target_complete_nacl); } +EXPORT_SYMBOL(target_put_nacl);

void transport_deregister_session_configfs(struct se_session *se_sess) { @@ -488,6 +488,15 @@ EXPORT_SYMBOL(transport_deregister_session_configfs);

void transport_free_session(struct se_session *se_sess) { + struct se_node_acl *se_nacl = se_sess->se_node_acl; + /* + * Drop the se_node_acl->nacl_kref obtained from within + * core_tpg_get_initiator_node_acl(). + */ + if (se_nacl) { + se_sess->se_node_acl = NULL; + target_put_nacl(se_nacl); + } if (se_sess->sess_cmd_map) { percpu_ida_destroy(&se_sess->sess_tag_pool); if (is_vmalloc_addr(se_sess->sess_cmd_map)) @@ -505,7 +514,6 @@ void transport_deregister_session(struct se_session *se_sess) const struct target_core_fabric_ops *se_tfo; struct se_node_acl *se_nacl; unsigned long flags; - bool comp_nacl = true;

if (!se_tpg) { transport_free_session(se_sess); @@ -533,9 +541,9 @@ void transport_deregister_session(struct se_session *se_sess) spin_unlock_irqrestore(&se_tpg->acl_node_lock, flags); core_tpg_wait_for_nacl_pr_ref(se_nacl); core_free_device_list_for_node(se_nacl, se_tpg); + se_sess->se_node_acl = NULL; se_tfo->tpg_release_fabric_acl(se_tpg, se_nacl);

- comp_nacl = false; spin_lock_irqsave(&se_tpg->acl_node_lock, flags); } } @@ -546,10 +554,8 @@ void transport_deregister_session(struct se_session *se_sess) /* * If last kref is dropping now for an explicit NodeACL, awake sleeping * ->acl_free_comp caller to wakeup configfs se_node_acl->acl_group - * removal context. + * removal context from within transport_free_session() code. */ - if (se_nacl && comp_nacl) - target_put_nacl(se_nacl);

transport_free_session(se_sess); } diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h index 24c8d9d..419a8c8 100644 --- a/include/target/target_core_fabric.h +++ b/include/target/target_core_fabric.h @@ -171,6 +171,8 @@ int transport_lookup_tmr_lun(struct se_cmd *, u32);

struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg, unsigned char *); +bool target_tpg_has_node_acl(struct se_portal_group *tpg, + const char *); struct se_node_acl *core_tpg_check_initiator_node_acl(struct se_portal_group *, unsigned char *); void core_tpg_clear_object_luns(struct se_portal_group *);

From: Bart Van Assche bart.vanassche@sandisk.com

commit 59b6986dbfcdab96a971f9663221849de79a7556 upstream.

Allocate a task management request structure for all task management requests, including task reassignment. This change avoids that the se_tmr->response assignment dereferences an uninitialized se_tmr pointer.

Reported-by: Moshe David mdavid@infinidat.com Signed-off-by: Bart Van Assche bart.vanassche@sandisk.com Reviewed-by: Hannes Reinecke hare@suse.com Reviewed-by: Christoph Hellwig hch@lst.de Cc: Moshe David mdavid@infinidat.com Signed-off-by: Nicholas Bellinger nab@linux-iscsi.org --- drivers/target/iscsi/iscsi_target.c | 19 +++++++------------ include/target/target_core_base.h | 1 + 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index d6cb9e8..ee24b76 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -1755,7 +1755,7 @@ iscsit_handle_task_mgt_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, struct iscsi_tm *hdr; int out_of_order_cmdsn = 0, ret; bool sess_ref = false; - u8 function; + u8 function, tcm_function = TMR_UNKNOWN;

hdr = (struct iscsi_tm *) buf; hdr->flags &= ~ISCSI_FLAG_CMD_FINAL; @@ -1801,10 +1801,6 @@ iscsit_handle_task_mgt_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, * LIO-Target $FABRIC_MOD */ if (function != ISCSI_TM_FUNC_TASK_REASSIGN) { - - u8 tcm_function; - int ret; - transport_init_se_cmd(&cmd->se_cmd, &iscsi_ops, conn->sess->se_sess, 0, DMA_NONE, TCM_SIMPLE_TAG, cmd->sense_buffer + 2); @@ -1840,15 +1836,14 @@ iscsit_handle_task_mgt_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, return iscsit_add_reject_cmd(cmd, ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf); } - - ret = core_tmr_alloc_req(&cmd->se_cmd, cmd->tmr_req, - tcm_function, GFP_KERNEL); - if (ret < 0) - return iscsit_add_reject_cmd(cmd, + } + ret = core_tmr_alloc_req(&cmd->se_cmd, cmd->tmr_req, tcm_function, + GFP_KERNEL); + if (ret < 0) + return iscsit_add_reject_cmd(cmd, ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);

- cmd->tmr_req->se_tmr_req = cmd->se_cmd.se_tmr_req; - } + cmd->tmr_req->se_tmr_req = cmd->se_cmd.se_tmr_req;

cmd->iscsi_opcode = ISCSI_OP_SCSI_TMFUNC; cmd->i_state = ISTATE_SEND_TASKMGTRSP; diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 3ab9ddb..c978ad2 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -228,6 +228,7 @@ enum tcm_tmreq_table { TMR_LUN_RESET = 5, TMR_TARGET_WARM_RESET = 6, TMR_TARGET_COLD_RESET = 7, + TMR_UNKNOWN = 0xff, };

/* fabric independent task management response values */

From: Jiang Yi jiangyilism@gmail.com

commit 5e0cf5e6c43b9e19fc0284f69e5cd2b4a47523b0 upstream.

There are three timing problems in the kthread usages of iscsi_target_mod:

- np_thread of struct iscsi_np - rx_thread and tx_thread of struct iscsi_conn

In iscsit_close_connection(), it calls

send_sig(SIGINT, conn->tx_thread, 1); kthread_stop(conn->tx_thread);

In conn->tx_thread, which is iscsi_target_tx_thread(), when it receive SIGINT the kthread will exit without checking the return value of kthread_should_stop().

So if iscsi_target_tx_thread() exit right between send_sig(SIGINT...) and kthread_stop(...), the kthread_stop() will try to stop an already stopped kthread.

This is invalid according to the documentation of kthread_stop().

(Fix -ECONNRESET logout handling in iscsi_target_tx_thread and early iscsi_target_rx_thread failure case - nab)

Signed-off-by: Jiang Yi jiangyilism@gmail.com Signed-off-by: Nicholas Bellinger nab@linux-iscsi.org --- drivers/target/iscsi/iscsi_target.c | 28 ++++++++++++++++++++++------ drivers/target/iscsi/iscsi_target_erl0.c | 6 +++++- drivers/target/iscsi/iscsi_target_erl0.h | 2 +- drivers/target/iscsi/iscsi_target_login.c | 4 ++++ 4 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index ee24b76..89d0194 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -3964,6 +3964,8 @@ int iscsi_target_tx_thread(void *arg) { int ret = 0; struct iscsi_conn *conn = arg; + bool conn_freed = false; + /* * Allow ourselves to be interrupted by SIGINT so that a * connection recovery / failure event can be triggered externally. @@ -3989,12 +3991,14 @@ get_immediate: goto transport_err;

ret = iscsit_handle_response_queue(conn); - if (ret == 1) + if (ret == 1) { goto get_immediate; - else if (ret == -ECONNRESET) + } else if (ret == -ECONNRESET) { + conn_freed = true; goto out; - else if (ret < 0) + } else if (ret < 0) { goto transport_err; + } }

transport_err: @@ -4004,8 +4008,13 @@ transport_err: * responsible for cleaning up the early connection failure. */ if (conn->conn_state != TARG_CONN_STATE_IN_LOGIN) - iscsit_take_action_for_connection_exit(conn); + iscsit_take_action_for_connection_exit(conn, &conn_freed); out: + if (!conn_freed) { + while (!kthread_should_stop()) { + msleep(100); + } + } return 0; }

@@ -4112,6 +4121,7 @@ int iscsi_target_rx_thread(void *arg) u32 checksum = 0, digest = 0; struct iscsi_conn *conn = arg; struct kvec iov; + bool conn_freed = false; /* * Allow ourselves to be interrupted by SIGINT so that a * connection recovery / failure event can be triggered externally. @@ -4123,7 +4133,7 @@ int iscsi_target_rx_thread(void *arg) */ rc = wait_for_completion_interruptible(&conn->rx_login_comp); if (rc < 0 || iscsi_target_check_conn_state(conn)) - return 0; + goto out;

if (conn->conn_transport->transport_type == ISCSI_INFINIBAND) { struct completion comp; @@ -4208,7 +4218,13 @@ int iscsi_target_rx_thread(void *arg) transport_err: if (!signal_pending(current)) atomic_set(&conn->transport_failed, 1); - iscsit_take_action_for_connection_exit(conn); + iscsit_take_action_for_connection_exit(conn, &conn_freed); +out: + if (!conn_freed) { + while (!kthread_should_stop()) { + msleep(100); + } + } return 0; }

diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c index 959a14c..6011644 100644 --- a/drivers/target/iscsi/iscsi_target_erl0.c +++ b/drivers/target/iscsi/iscsi_target_erl0.c @@ -930,8 +930,10 @@ static void iscsit_handle_connection_cleanup(struct iscsi_conn *conn) } }

-void iscsit_take_action_for_connection_exit(struct iscsi_conn *conn) +void iscsit_take_action_for_connection_exit(struct iscsi_conn *conn, bool *conn_freed) { + *conn_freed = false; + spin_lock_bh(&conn->state_lock); if (atomic_read(&conn->connection_exit)) { spin_unlock_bh(&conn->state_lock); @@ -942,6 +944,7 @@ void iscsit_take_action_for_connection_exit(struct iscsi_conn *conn) if (conn->conn_state == TARG_CONN_STATE_IN_LOGOUT) { spin_unlock_bh(&conn->state_lock); iscsit_close_connection(conn); + *conn_freed = true; return; }

@@ -955,6 +958,7 @@ void iscsit_take_action_for_connection_exit(struct iscsi_conn *conn) spin_unlock_bh(&conn->state_lock);

iscsit_handle_connection_cleanup(conn); + *conn_freed = true; }

/* diff --git a/drivers/target/iscsi/iscsi_target_erl0.h b/drivers/target/iscsi/iscsi_target_erl0.h index 21acc9a..42673fb 100644 --- a/drivers/target/iscsi/iscsi_target_erl0.h +++ b/drivers/target/iscsi/iscsi_target_erl0.h @@ -9,7 +9,7 @@ extern int iscsit_stop_time2retain_timer(struct iscsi_session *); extern void iscsit_connection_reinstatement_rcfr(struct iscsi_conn *); extern void iscsit_cause_connection_reinstatement(struct iscsi_conn *, int); extern void iscsit_fall_back_to_erl0(struct iscsi_session *); -extern void iscsit_take_action_for_connection_exit(struct iscsi_conn *); +extern void iscsit_take_action_for_connection_exit(struct iscsi_conn *, bool *); extern int iscsit_recover_from_unknown_opcode(struct iscsi_conn *);

#endif /*** ISCSI_TARGET_ERL0_H ***/ diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index ee3a4bb..0e75ea8 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -1510,5 +1510,9 @@ int iscsi_target_login_thread(void *arg) break; }

+ while (!kthread_should_stop()) { + msleep(100); + } + return 0; }

From: Nicholas Bellinger nab@linux-iscsi.org

commit 6f48655facfd7f7ccfe6d252ac0fe319ab02e4dd upstream.

This patch fixes a generate_node_acls = 1 + cache_dynamic_acls = 0 regression, that was introduced by

commit 01d4d673558985d9a118e1e05026633c3e2ade9b Author: Nicholas Bellinger nab@linux-iscsi.org Date: Wed Dec 7 12:55:54 2016 -0800

which originally had the proper list_del_init() usage, but was dropped during list review as it was thought unnecessary by HCH.

However, list_del_init() usage is required during the special generate_node_acls = 1 + cache_dynamic_acls = 0 case when transport_free_session() does a list_del(&se_nacl->acl_list), followed by target_complete_nacl() doing the same thing.

This was manifesting as a general protection fault as reported by Justin:

kernel: general protection fault: 0000 [#1] SMP kernel: Modules linked in: kernel: CPU: 0 PID: 11047 Comm: iscsi_ttx Not tainted 4.13.0-rc2.x86_64.1+ #20 kernel: Hardware name: Intel Corporation S5500BC/S5500BC, BIOS S5500.86B.01.00.0064.050520141428 05/05/2014 kernel: task: ffff88026939e800 task.stack: ffffc90007884000 kernel: RIP: 0010:target_put_nacl+0x49/0xb0 kernel: RSP: 0018:ffffc90007887d70 EFLAGS: 00010246 kernel: RAX: dead000000000200 RBX: ffff8802556ca000 RCX: 0000000000000000 kernel: RDX: dead000000000100 RSI: 0000000000000246 RDI: ffff8802556ce028 kernel: RBP: ffffc90007887d88 R08: 0000000000000001 R09: 0000000000000000 kernel: R10: ffffc90007887df8 R11: ffffea0009986900 R12: ffff8802556ce020 kernel: R13: ffff8802556ce028 R14: ffff8802556ce028 R15: ffffffff88d85540 kernel: FS: 0000000000000000(0000) GS:ffff88027fc00000(0000) knlGS:0000000000000000 kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 kernel: CR2: 00007fffe36f5f94 CR3: 0000000009209000 CR4: 00000000003406f0 kernel: DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 kernel: DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 kernel: Call Trace: kernel: transport_free_session+0x67/0x140 kernel: transport_deregister_session+0x7a/0xc0 kernel: iscsit_close_session+0x92/0x210 kernel: iscsit_close_connection+0x5f9/0x840 kernel: iscsit_take_action_for_connection_exit+0xfe/0x110 kernel: iscsi_target_tx_thread+0x140/0x1e0 kernel: ? wait_woken+0x90/0x90 kernel: kthread+0x124/0x160 kernel: ? iscsit_thread_get_cpumask+0x90/0x90 kernel: ? kthread_create_on_node+0x40/0x40 kernel: ret_from_fork+0x22/0x30 kernel: Code: 00 48 89 fb 4c 8b a7 48 01 00 00 74 68 4d 8d 6c 24 08 4c 89 ef e8 e8 28 43 00 48 8b 93 20 04 00 00 48 8b 83 28 04 00 00 4c 89 ef <48> 89 42 08 48 89 10 48 b8 00 01 00 00 00 00 ad de 48 89 83 20 kernel: RIP: target_put_nacl+0x49/0xb0 RSP: ffffc90007887d70 kernel: ---[ end trace f12821adbfd46fed ]---

To address this, go ahead and use proper list_del_list() for all cases of se_nacl->acl_list deletion.

Reported-by: Justin Maggard jmaggard01@gmail.com Tested-by: Justin Maggard jmaggard01@gmail.com Cc: Justin Maggard jmaggard01@gmail.com Cc: stable@vger.kernel.org # 4.1+ Signed-off-by: Nicholas Bellinger nab@linux-iscsi.org --- drivers/target/target_core_tpg.c | 4 ++-- drivers/target/target_core_transport.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c index 03f43ce..7077172 100644 --- a/drivers/target/target_core_tpg.c +++ b/drivers/target/target_core_tpg.c @@ -500,7 +500,7 @@ int core_tpg_del_initiator_node_acl( if (acl->dynamic_node_acl) { acl->dynamic_node_acl = 0; } - list_del(&acl->acl_list); + list_del_init(&acl->acl_list); tpg->num_node_acls--; spin_unlock_irq(&tpg->acl_node_lock);

@@ -801,7 +801,7 @@ int core_tpg_deregister(struct se_portal_group *se_tpg) spin_lock_irq(&se_tpg->acl_node_lock); list_for_each_entry_safe(nacl, nacl_tmp, &se_tpg->acl_node_list, acl_list) { - list_del(&nacl->acl_list); + list_del_init(&nacl->acl_list); se_tpg->num_node_acls--; spin_unlock_irq(&se_tpg->acl_node_lock);

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 64cd004..958b0dc 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -457,7 +457,7 @@ static void target_complete_nacl(struct kref *kref) }

spin_lock_irqsave(&se_tpg->acl_node_lock, flags); - list_del(&nacl->acl_list); + list_del_init(&nacl->acl_list); se_tpg->num_node_acls--; spin_unlock_irqrestore(&se_tpg->acl_node_lock, flags);

@@ -530,7 +530,7 @@ void transport_free_session(struct se_session *se_sess) spin_unlock(&se_nacl->nacl_sess_lock);

if (se_nacl->dynamic_stop) - list_del(&se_nacl->acl_list); + list_del_init(&se_nacl->acl_list); } spin_unlock_irqrestore(&se_tpg->acl_node_lock, flags);