RE: isert patch leaving resources behind

[Saravanan Vajravel <saravanan.vajravel@xxxxxxxxxxxx>]

Hi Sagi,

In surprise removal case as well, isert_free_conn() should get invoked by
iSCSI target module. Right? I am trying to understand how the kernel object
leak is possible.  In your proposed patch, isert_conn is released in
isert_wait_conn() handler. Again, isert module tries to release the
connection in isert_free_conn() handler as well. Hence it will lead
use-after-free issue.

Following are the snippet of iSCSI functions where iscsit_wait_conn()  and
iscsit_free_conn() handlers are invoked in files iscsi_target_login.c &
iscsi_target.c. We need to review if there is a possibility that
iscsit_free_conn() is not invoked in any case. If yes, we may have to fix
that.

void  iscsi_target_login_sess_out
{
.
.
.
	if (conn->conn_transport->iscsit_wait_conn)
		conn->conn_transport->iscsit_wait_conn(conn);

	if (conn->conn_transport->iscsit_free_conn)
		conn->conn_transport->iscsit_free_conn(conn);
.
}

int iscsit_close_connection(
	struct iscsit_conn *conn)
{
.
.
.
	if (conn->conn_transport->iscsit_wait_conn)
		conn->conn_transport->iscsit_wait_conn(conn);
.
.
.
	if (conn->conn_transport->iscsit_free_conn)
		conn->conn_transport->iscsit_free_conn(conn);
.
.
}

@Leon,

I don't have the kernel logs in hand now. We can reproduce the issue and
share.

Thanks & Regards,
Saravanan Vajravel
+91-80-46116256

-----Original Message-----
From: Sagi Grimberg <sagi@xxxxxxxxxxx>
Sent: Sunday, August 13, 2023 7:49 PM
To: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxxxxxxxxxxxxx>; OFED
mailing list <linux-rdma@xxxxxxxxxxxxxxx>
Cc: Ehab Ababneh <ehab.ababneh@xxxxxxxxxxxxxxxxxxxx>;
saravanan.vajravel@xxxxxxxxxxxx; Selvin Xavier <selvin.xavier@xxxxxxxxxxxx>
Subject: Re: isert patch leaving resources behind



On 8/10/23 17:44, Dennis Dalessandro wrote:
> Commit: 699826f4e30a ("IB/isert: Fix incorrect release of isert
> connection") is causing problems on OPA when we try to unload the
> driver after doing iSCI testing. Reverting this commit causes the
> problem to go away. Any ideas? Was testing done on this patch with
> removing/hotplugging drivers?

You are correct, the patch is wrong because it doesn't fully release the
connection in ib device surprise removals.

Perhaps this should address this issue?
--
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
b/drivers/infiniband/ulp/isert/ib_isert.c
index 92e1e7587af8..274ac9361fe7 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -2570,6 +2570,9 @@ static void isert_wait_conn(struct iscsit_conn *conn)
         isert_put_unsol_pending_cmds(conn);
         isert_wait4cmds(conn);
         isert_wait4logout(isert_conn);
+
+       isert_put_conn(isert_conn);
+       conn->context = NULL;
  }

  static void isert_free_conn(struct iscsit_conn *conn)
--

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature