Re: [PATCH 1/2] target: iscs: reject cmd in closed session

From: michael.christie@oracle.com
To: Dmitry Bogdanov <d.bogdanov@yadro.com>,
	Martin Petersen <martin.petersen@oracle.com>,
	target-devel@vger.kernel.org
Cc: Forza <forza@tnonline.net>, linux-scsi@vger.kernel.org, linux@yadro.com
Subject: Re: [PATCH 1/2] target: iscs: reject cmd in closed session
Date: Wed, 25 Jan 2023 11:18:32 -0600	[thread overview]
Message-ID: <478beda7-df98-d925-bfac-2856984c12d7@oracle.com> (raw)
In-Reply-To: <825571ca-4687-4b5c-971f-b9e13efb152d@oracle.com>

On 1/25/23 11:03 AM, Mike Christie wrote:
> On 1/25/23 02:33, Dmitry Bogdanov wrote:
>> Do not handle incoming commands if the session is already closed.
>>
>> That patch fixes the following stacktrace:
>>
>>  Decremented iSCSI connection count to 0 from node: iqn.1996-04.com.local:3
>>  TARGET_CORE[iSCSI]: Deregistered fabric_sess
>>  Moving to TARG_SESS_STATE_FREE.
>>  Released iSCSI session from node: iqn.1996-04.com.local:3
>>  Decremented number of active iSCSI Sessions on iSCSI TPG: 0 to 1
>>  rx_loop: 48, total_rx: 48, data: 48
>>  Got SCSI Command, ITT: 0x2000005d, CmdSN: 0x4a020000, ExpXferLen: 0, Length: 0, CID: 0
>> BUG: Kernel NULL pointer dereference on read at 0x00000000
>>   Faulting instruction address: 0xc008000000a9b574
>>   Oops: Kernel access of bad area, sig: 11 [#1]
>>   NIP [c008000000a9b574] transport_lookup_cmd_lun+0x37c/0x470 [target_core_mod]
>>   LR [c008000001017318] iscsit_setup_scsi_cmd+0x520/0x780 [iscsi_target_mod]
>> Call Trace:
>>  [c000000059e4fae0] [c000000059e4fb70] 0xc000000059e4fb70 (unreliable)
>>  [c000000059e4fb70] [c008000001017318] iscsit_setup_scsi_cmd+0x520/0x780 [iscsi_target_mod]
>>  [c000000059e4fc30] [c00800000101c448] iscsit_get_rx_pdu+0x720/0x11d0 [iscsi_target_mod]
>>  [c000000059e4fd60] [c00800000101ebc8] iscsi_target_rx_thread+0xb0/0x190 [iscsi_target_mod]
>>  [c000000059e4fdb0] [c00000000018c50c] kthread+0x19c/0x1b0
>>
>> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
>> ---
>>  drivers/target/iscsi/iscsi_target.c | 8 ++++++--
>>  include/scsi/iscsi_proto.h          | 1 +
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
>> index baf4da7bb3b4..f6008675dd3f 100644
>> --- a/drivers/target/iscsi/iscsi_target.c
>> +++ b/drivers/target/iscsi/iscsi_target.c
>> @@ -1199,7 +1199,9 @@ int iscsit_setup_scsi_cmd(struct iscsit_conn *conn, struct iscsit_cmd *cmd,
>>  		hdr->cmdsn, be32_to_cpu(hdr->data_length), payload_length,
>>  		conn->cid);
>>  
>> -	target_get_sess_cmd(&cmd->se_cmd, true);
>> +	if (target_get_sess_cmd(&cmd->se_cmd, true) < 0)
>> +		return iscsit_add_reject_cmd(cmd,
>> +				ISCSI_REASON_WAITING_FOR_LOGOUT, buf);
>>
> Did this require target_stop_session somewhere? I think this is a possible

Oh wait, if there is a use after free like below then iscsit_stop_session
won't help since we are trying to stop incoming commands from referencing
the se_session/iscsit_session. We would need to check something on the
iscsit_conn.

> use after free.
> 
> It seems like if we have logged the message:
> 
>>  Moving to TARG_SESS_STATE_FREE.
> 
> then we called:
> 
> transport_deregister_session -> transport_free_session
> 
> and freed the se_session.
> 
> So above if target_get_sess_cmd returns failure then we have run:
> 
> transport_free_session ->transport_uninit_session -> percpu_ref_exit
> 
> and transport_free_session could have done:
> 
> kmem_cache_free(se_sess_cache, se_sess)
> 
> by the time we run the code above and we are now accessing a freed
> se_session and iscsit_session.