RE: isert patch leaving resources behind

[Saravanan Vajravel <saravanan.vajravel@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]

>> You should look at the DEVICE_REMOVAL cm handler, it should call
>> iscsit_cause_connection_reinstatement. I do think that it needs to pass
>> it sleep=true (for device removal) so that it will wait for
>> conn_wait_comp...
Ok. So it should take care of current resource leak issue. Right?

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

-----Original Message-----
From: Sagi Grimberg <sagi@grimberg.me>
Sent: Monday, August 14, 2023 6:07 PM
To: Saravanan Vajravel <saravanan.vajravel@broadcom.com>; Dennis Dalessandro
<dennis.dalessandro@cornelisnetworks.com>; OFED mailing list
<linux-rdma@vger.kernel.org>
Cc: Ehab Ababneh <ehab.ababneh@cornelisnetworks.com>; Selvin Xavier
<selvin.xavier@broadcom.com>; leon@kernel.org
Subject: Re: isert patch leaving resources behind



On 8/14/23 14:20, Saravanan Vajravel wrote:
> 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.

You should look at the DEVICE_REMOVAL cm handler, it should call
iscsit_cause_connection_reinstatement. I do think that it needs to pass it
sleep=true (for device removal) so that it will wait for conn_wait_comp...

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4227 bytes --]