Re: scsi: iscsi: Drop suspend calls from ep_disconnect

[Mike Christie]

On 6/3/21 6:25 PM, Mike Christie wrote:
> On 6/3/21 5:25 PM, Colin Ian King wrote:
>> Hi,
>>
>> Static analysis on linux-next with Coverity has found an issue in
>> drivers/scsi/qedi/qedi_iscsi.c with the following commit:
>>
>> commit 27e986289e739d08c1a4861cc3d3ec9b3a60845e
>> Author: Mike Christie <michael.christie@xxxxxxxxxx>
>> Date:   Tue May 25 13:17:56 2021 -0500
>>
>>     scsi: iscsi: Drop suspend calls from ep_disconnect
>>
>> The analysis is as follows:
>>
>> 1662 void qedi_clear_session_ctx(struct iscsi_cls_session *cls_sess)
>> 1663 {
>> 1664        struct iscsi_session *session = cls_sess->dd_data;
>> 1665        struct iscsi_conn *conn = session->leadconn;
>>
>>     deref_ptr: Directly dereferencing pointer conn.
>>
>> 1666        struct qedi_conn *qedi_conn = conn->dd_data;
>> 1667
>> 1668        if (iscsi_is_session_online(cls_sess)) {
>>    Dereference before null check (REVERSE_INULL)
>>    check_after_deref: Null-checking conn suggests that it may be null,
>> but it has already been dereferenced on all paths leading to the check.
>>
>> 1669                if (conn)
>> 1670                        iscsi_suspend_queue(conn);
>> 1671                qedi_ep_disconnect(qedi_conn->iscsi_ep);
>> 1672        }
>>
>> Pointer conn is being checked to see if it is null, but earlier it has
>> been dereferenced on the assignment of qedi_conn.  So either conn will
>> be null at some point and a null ptr dereference occurs when qedi_conn
>> is assigned, or conn can never be null and the conn null check is
>> redundant and can be removed.
> 
> The analysis is correct.
> 
> The bigger problem is that this entire function seems racey with the
> normal conn/ep disconnect or shutdown.
> 
> Manish, when this function is run iscsid or the in-kernel conn error
> cleanup handler can be running right? There is nothing preventing
> those from running at the same time?
> 
> I think you want to call iscsi_host_remove at the beginning of __qedi_remove.
> That will tell userpsace that the host is being removed and libiscsi will

I meant iscsid not libiscsi.

> start the session shutdown and removal process. It then waits for the
> sessions to be removed. We can then proceed with the other host removal
> cleanup, and at the end of __qedi_remove you do the iscsi_host_free
> call.
>