On 01/06/2016 06:59 AM, Christoph Hellwig wrote:
On Tue, Jan 05, 2016 at 02:48:57PM +0100, Bart Van Assche wrote:
Target drivers like ib_srpt can call transport_deregister_session()
while core_tpg_del_initiator_node_acl() is processing sess_acl_list.
Avoid that this scenario triggers a use-after-free by postponing
freeing a session object until core_tpg_del_initiator_node_acl() has
finished accessing that session object. Keep the se_tpg and
fabric_sess_ptr member variables as long as the session object
exists.
Wouldn't the simple patch below also fix that issue?
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 5fb9dd7..7fb63ab 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -308,7 +308,6 @@ struct se_node_acl *core_tpg_add_initiator_node_acl(
void core_tpg_del_initiator_node_acl(struct se_node_acl *acl)
{
struct se_portal_group *tpg = acl->se_tpg;
- LIST_HEAD(sess_list);
struct se_session *sess, *sess_tmp;
unsigned long flags;
int rc;
@@ -323,26 +322,23 @@ void core_tpg_del_initiator_node_acl(struct se_node_acl *acl)
spin_lock_irqsave(&acl->nacl_sess_lock, flags);
acl->acl_stop = 1;
-
+restart:
list_for_each_entry_safe(sess, sess_tmp, &acl->acl_sess_list,
sess_acl_list) {
if (sess->sess_tearing_down != 0)
continue;
-
- target_get_session(sess);
- list_move(&sess->sess_acl_list, &sess_list);
- }
- spin_unlock_irqrestore(&acl->nacl_sess_lock, flags);
-
- list_for_each_entry_safe(sess, sess_tmp, &sess_list, sess_acl_list) {
+
list_del(&sess->sess_acl_list);
+ spin_unlock_irqrestore(&acl->nacl_sess_lock, flags);
rc = tpg->se_tpg_tfo->shutdown_session(sess);
- target_put_session(sess);
- if (!rc)
- continue;
- target_put_session(sess);
+ if (rc)
+ target_put_session(sess);
+ spin_lock_irqsave(&acl->nacl_sess_lock, flags);
+ goto restart;
}
+ spin_unlock_irqrestore(&acl->nacl_sess_lock, flags);
+
target_put_nacl(acl);
/*
* Wait for last target_put_nacl() to complete in target_complete_nacl()
Hello Christoph,
Eliminating "sess_list" seems like a good idea to me. But even if that
list is eliminated I think we still need the "kref2" mechanism that was
introduced in my patch. It is not known which function will be called
first - transport_deregister_session() (e.g. if ib_srpt receives a DREQ)
or core_tpg_del_initiator_node_acl(). But the target core session object
has to be kept until both functions have finished.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
