Re: [PATCH v2 1/1] scsi: libiscsi: fix NOP race condition

[Mike Christie]

On 10/8/20 12:11 PM, Mike Christie wrote:
> On 9/25/20 1:41 PM, lduncan@xxxxxxxx wrote:
>> From: Lee Duncan <lduncan@xxxxxxxx>
>>
>> iSCSI NOPs are sometimes "lost", mistakenly sent to the
>> user-land iscsid daemon instead of handled in the kernel,
>> as they should be, resulting in a message from the daemon like:
>>
>>> iscsid: Got nop in, but kernel supports nop handling.
>>
>> This can occur because of the forward- and back-locks
>> in the kernel iSCSI code, and the fact that an iSCSI NOP
>> response can be processed before processing of the NOP send
>> is complete. This can result in "conn->ping_task" being NULL
>> in iscsi_nop_out_rsp(), when the pointer is actually in
>> the process of being set.
>>
>> To work around this, we add a new state to the "ping_task"
>> pointer. In addition to NULL (not assigned) and a pointer
>> (assigned), we add the state "being set", which is signaled
>> with an INVALID pointer (using "-1").
>>
>> Signed-off-by: Lee Duncan <lduncan@xxxxxxxx>
>> ---
>>  drivers/scsi/libiscsi.c | 13 ++++++++++---
>>  include/scsi/libiscsi.h |  3 +++
>>  2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index 1e9c3171fa9f..cade108c33b6 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -738,6 +738,9 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
>>  						   task->conn->session->age);
>>  	}
>>  
>> +	if (unlikely(READ_ONCE(conn->ping_task) == INVALID_SCSI_TASK))
>> +		WRITE_ONCE(conn->ping_task, task);
>> +
>>  	if (!ihost->workq) {
>>  		if (iscsi_prep_mgmt_task(conn, task))
>>  			goto free_task;
> 
> I think the API gets a little weird now where in some cases
> __iscsi_conn_send_pdu checks the opcode to see what type of request
> it is but above we the caller sets the ping_task.
> 
> For login, tmfs and passthrough, we assume the __iscsi_conn_send_pdu
> has sent or cleaned up everything. I think it might be nicer to just
> have __iscsi_conn_send_pdu set the ping_task field before doing the
> xmit/queue call. It would then work similar to the conn->login_task
> case where that function knows about that special task too.
> 
> So in __iscsi_conn_send_pdu add a "if (opcode == ISCSI_OP_NOOP_OUT)",
> and check if it's a nop we need to track. If so set conn->ping_task.
> 
Ignore this. It won't work nicely either. To figure out if the nop is
our internal transport test ping vs a userspace ping that also needs
a reply, we would need to do something like you did above so there is
no point.