Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 3/3] scsi: libiscsi: get ref to conn in
 iscsi_eh_device/target_reset()
To:     Ding Hui <dinghui@sangfor.com.cn>, lduncan@suse.com,
        cleech@redhat.com, jejb@linux.ibm.com, martin.petersen@oracle.com,
        open-iscsi@googlegroups.com, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20210910010220.24073-1-dinghui@sangfor.com.cn>
 <20210910010220.24073-4-dinghui@sangfor.com.cn>
From:   Mike Christie <michael.christie@oracle.com>
Message-ID: <4bc7187d-0461-d42c-3a8a-33c0a0a8c1e0@oracle.com>
Date:   Fri, 10 Sep 2021 11:46:49 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
In-Reply-To: <20210910010220.24073-4-dinghui@sangfor.com.cn>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NWpsc2lSa3NxSkNnTEJUZ2pGaGRldHF2OEdFZWN1WUROVVgrVkdGWWNyN2Zr?=
 =?utf-8?B?UTk2M3hLV0YrZVk3TG9ZNEx6c2hVZ1lCY29mazYwaHN0djdxeThvU1JzMWZ2?=
 =?utf-8?B?ZTQxUTA3YjJNUmNsWXAxcFMxcXI1WU40QnpZd25LK0JGSVRPenp3T1IxM3Q3?=
 =?utf-8?B?NGNPeFJRNW9JL0VaOVowVW5scXZJUEhtQ3B3N0U4L01HRWF0aHJhWGlLN1Bu?=
 =?utf-8?B?UW9adE1najJUS0tWUVNKQzgvS2ZTaXZ5WmxCTWVqVzVrT05WcHI2L1NtbnJD?=
 =?utf-8?B?ZjBxWFZtWVlhWVo2MzdjeVVOTVliRktsVzY0MUxYU2xzck1veGdUMWZPaDZE?=
 =?utf-8?B?K0NDMUtLdEF5Y2xDK2dnS2t1OHNSdTl4TkZZMWRJUEg5Q3RLNkk4Ukl3M0hH?=
 =?utf-8?B?S2Z3RysxRTJKZWh5NkVRZW5EQXphWk5STWNtTDZOQnkreFlWZEdkWDFETWZ4?=
 =?utf-8?B?MkFxWmxjbjkrVkdNbElFNzBncUp1NVJSNWt5RWg4VFlPdlNvT3RuSVhMSXJF?=
 =?utf-8?B?cllaampoSU1DdmlFcXYwT0RGZkxJdGtwQmw1bkNycUlJZUlNTzV3Y0h1c3Zv?=
 =?utf-8?B?UWQzR0JhTmE0OG95WXExSzhXdUJIOGtSdldFWmVBVGpoaGd2eXB6czc2SHpX?=
 =?utf-8?B?NDJnOEtMY01vbW1kTEw1TG8wSmN5Q0s5VzBrOEFWSW5SM3VpNEpOd3diYXVL?=
 =?utf-8?B?RHF5dUw5aEtyTWtoRkNtKzB6eGhWY0JFWXhmWEYvUkVEdnNWT1dmd3Jmb3ls?=
 =?utf-8?B?a0ZJNlFJc3psNnN0NnFacURVTk5Eamx4MEo1eVd2TDhxeEp0UmxTWkZUMVdt?=
 =?utf-8?B?QmdOUmt5TFpHelVVemVGcFg4VHV2akcvbkRtR3ZPQXlGUUVVSE4zaWxTQUVv?=
 =?utf-8?B?V0JsSWVZak5SNHRoMFZ4d0pSYTN4UisyQzlkNHV3SUFxQzlsdG12MEVTRnFE?=
 =?utf-8?B?NjJXODNqKzAvZmJ6UFY3TCtWVkJnN2tpc1F0eEVpQnZwcG54c0lXYTMrMzNz?=
 =?utf-8?B?Rk8wZTE1bENwMFlSbjQzcVlMcUNLL3RjdU5CYjN1Wm5ITnZkbStzYllxTmRK?=
 =?utf-8?B?Y2gxVjdHVytxNGpodkhxanUwUVB3R3NIWFBmVEFoSUdTZEV5TXZxZktRdDQz?=
 =?utf-8?B?Q2U0Rm5qWWV4bjdaenlvUVh2ckZaSzRLYzJxVk9GWEJ3K3A2T3JsTy91a01R?=
 =?utf-8?B?aWR4bWptMUs1aHp0bDlIYWt5YklxR3VibTRjMEwxODFCSndwYmpWMFpOdmpq?=
 =?utf-8?B?ZDg4enV5RGxobkxNTW9hNW9mbG9WZS9YV2tzOUcxVG91UUdOd1RmYmNGRzZ2?=
 =?utf-8?B?a25aTUt0eFJHUDFURzBKa0V5bks2T0lSVG0xSUNiUmNiQk1TVHdkaittZCt0?=
 =?utf-8?B?SXNMTXRMYXhuaS9MdVZIMnR2cjBaZkE4a2U3akR5SWY2R05GdUtiNUVDeER0?=
 =?utf-8?B?bzA4dlltS3FqRGdQVmhhRFZjRGFDcUd3TFQzSWs4dU5oL1hQUi9seERkRnpt?=
 =?utf-8?B?eVFmY29yZVI3TDRVbVhZQStnYllYRFdOTTRjcWVacjRTQlI1NlBqcmd0ei9z?=
 =?utf-8?B?VUdSUlJ2YnJIaXArTWptRjRCbzNua2VqRGd5dVlaVyt6eS9LMWZ3WE53bUNL?=
 =?utf-8?B?ZkZMUCthRW1iZzZoS3k1NGtxdXlkV1dCNUUyd29BMFNEVHRBbXhQWU5tZUl2?=
 =?utf-8?B?WlNGTis5d1NWKzZnVmdBS1g4OSt5TXRPcGpoVHVDeE1xTzFkTHJ0L2R4bGEy?=
 =?utf-8?Q?cYqWDRentazfpQNLv4vPK/dUBBCev/DsE1zNFBy?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a343f22-d732-4c46-c48d-08d9747a9688
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB3573.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Sep 2021 16:46:51.0933
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: aeMb2zzLh87P4hbQ2npaYwI4NfenomUjuEBnwBvqXX/NXU+ZNWGg6hEB9BeaG66sJgE/gYQbRd4YOe7UnW1iooaS8B/ASRqVfOc7P8n0+D4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5487
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10103 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0 bulkscore=0
 suspectscore=0 spamscore=0 mlxlogscore=999 mlxscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109030001
 definitions=main-2109100097
X-Proofpoint-GUID: GvuK2Qm6u7dlr-FtOLfTDclO2dEB5kKt
X-Proofpoint-ORIG-GUID: GvuK2Qm6u7dlr-FtOLfTDclO2dEB5kKt
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 9/9/21 8:02 PM, Ding Hui wrote:
> like commit fda290c5ae98 ("scsi: iscsi: Get ref to conn during reset
> handling"), because in iscsi_exec_task_mgmt_fn(), the eh_mutex and
> frwd_lock will be unlock, the conn also can be released if we not
> hold ref.
> 

This should not happen because we only access the conn if the session state
is ISCSI_STATE_LOGGED_IN. So on entry of the iscsi_eh_* functions we grab the
lock and mutex and check the session state. If logged in then we access the conn.
We then drop the lock/mutex to do the TMF/IO and do a wait. When we wake up, we
retake the lock/mutex and then check the state again.

But yeah, the code sucks (it's my fault) in that its half refcount half locks
and state checks like that. Ideally we should fully convert to the refcounts
and move the teardown/freeing to the release function. I was going to do this
in a patchset to fix up the EH/TMF code after I get my lock removal patches
merged.


> Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
> ---
>  drivers/scsi/libiscsi.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 69b3b2148328..4d3b37c20f8a 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2398,7 +2398,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
>  {
>  	struct iscsi_cls_session *cls_session;
>  	struct iscsi_session *session;
> -	struct iscsi_conn *conn;
> +	struct iscsi_conn *conn = NULL;
>  	struct iscsi_tm *hdr;
>  	int rc = FAILED;
>  
> @@ -2417,6 +2417,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
>  	if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN)
>  		goto unlock;
>  	conn = session->leadconn;
> +	iscsi_get_conn(conn->cls_conn);
>  
>  	/* only have one tmf outstanding at a time */
>  	if (session->tmf_state != TMF_INITIAL)
> @@ -2463,6 +2464,8 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
>  done:
>  	ISCSI_DBG_EH(session, "dev reset result = %s\n",
>  		     rc == SUCCESS ? "SUCCESS" : "FAILED");
> +	if (conn)
> +		iscsi_put_conn(conn->cls_conn);
>  	mutex_unlock(&session->eh_mutex);
>  	return rc;
>  }
> @@ -2560,7 +2563,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
>  {
>  	struct iscsi_cls_session *cls_session;
>  	struct iscsi_session *session;
> -	struct iscsi_conn *conn;
> +	struct iscsi_conn *conn = NULL;
>  	struct iscsi_tm *hdr;
>  	int rc = FAILED;
>  
> @@ -2579,6 +2582,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
>  	if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN)
>  		goto unlock;
>  	conn = session->leadconn;
> +	iscsi_get_conn(conn->cls_conn);
>  
>  	/* only have one tmf outstanding at a time */
>  	if (session->tmf_state != TMF_INITIAL)
> @@ -2625,6 +2629,8 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
>  done:
>  	ISCSI_DBG_EH(session, "tgt %s reset result = %s\n", session->targetname,
>  		     rc == SUCCESS ? "SUCCESS" : "FAILED");
> +	if (conn)
> +		iscsi_put_conn(conn->cls_conn);
>  	mutex_unlock(&session->eh_mutex);
>  	return rc;
>  }
>