Received-SPF: pass (google.com: domain of linux-kernel+bounces-188921-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <d28c71c6-6f39-40c7-9ded-c0bcd11f89fb@oracle.com>
Date: Fri, 24 May 2024 11:14:07 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/2] libiscsi: disallow binding an already-bound
 connection
To: Khazhismel Kumykov <khazhy@chromium.org>, Lee Duncan <lduncan@suse.com>,
        Chris Leech <cleech@redhat.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        open-iscsi@googlegroups.com, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org, Khazhismel Kumykov <khazhy@google.com>
References: <20240523222128.786137-1-khazhy@google.com>
 <20240523222128.786137-2-khazhy@google.com>
Content-Language: en-US
From: Mike Christie <michael.christie@oracle.com>
In-Reply-To: <20240523222128.786137-2-khazhy@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?djczUEJRNE5XdE8yR3d5SE13TDk4ZWF3SVJHdkpsZUIzSEJ4bElWb0d2SVJI?=
 =?utf-8?B?QUJvZm1La3MyZWkrVEdqZ0JwNmNZZGhYN1RSZGhrYmJXSVcrMDQ1ZnRUd2hr?=
 =?utf-8?B?Mk1pLzlQVVlVN0c1eUNHZHRQY29hc1huc0p0RG5abm5IWVErK2N3ZWZWSEt4?=
 =?utf-8?B?SmQ4UWlpNnd0djljTHF0U3FuOFN2ODVQTFl4M2VwTWYycXN3NEdvbnh6Umk3?=
 =?utf-8?B?N3FydEpqdjV2UFRJSHgyZjBFbHBtN1NLSUx6WTZZaE1KZm1Wa2xMV0VpWGd3?=
 =?utf-8?B?bnVHQnZ5N29vWTVVWG1KVmRIdHlOMi80dGJ1VlduYnZPcFIwaTFPZG50U3Bp?=
 =?utf-8?B?bWJweGZmOEt4V094WC9OMVUzOXp4a1JNMFBhaVlvRGFNN0dlTmF0L3htY1p4?=
 =?utf-8?B?N1gzNExkUkdlQ2dJc0RkTkJ3ZFl0Q1BVWDVINmZ2V1JHZVZpUmo5U043UVlP?=
 =?utf-8?B?VE10eG96VnNSaXdPT2tsYnBiSWFGUDRRT282b0JsRDhQc0JDLzhRL3RwaG50?=
 =?utf-8?B?KzVheVBMeEJndXE3bmdpcDJkK2FtY2QxYnR0YTVUanV3ZE1QWmpmUzhFZU5V?=
 =?utf-8?B?cDJNOTJvNzZIL1FCVWQ2ZzFuc2hiZTNXUElabDAxelExa3NOMGlUUUJRQ1N6?=
 =?utf-8?B?YTlWcll4ckp1MW9paTFHV1VuSW5BNjkwRTZBaTBqVTdBY0tTUUNISWxGcW9D?=
 =?utf-8?B?WGNiL1p0blNGUDFOblVTalVYOXFINkxTRUx2dGdHT2hwaUduSjkydVN3MWVt?=
 =?utf-8?B?OE1QZFNsM0RSNGhGbUZFdCtqUFg1M2NVUHVRVm82YmZlSnhVazY5bHEzSDNF?=
 =?utf-8?B?SlE1WjE1MFo4VmNzc29ncXc3a3NWWS9qeEhnOC95MWtzMVFPR0RYQk1UZGVp?=
 =?utf-8?B?MnFiVmxWVi80YzdyZEhEeTdYUFZkaGkzanIrUGhsTUYxZE4yL25aWTVRTUxG?=
 =?utf-8?B?dFZZYzg1NUZFdVl3QkRrcVVnN1VtUTF0TDZIaENaVlg0anZibkdWcWZhc0pC?=
 =?utf-8?B?TUROQTNJWFFiNHQ3YmFjU1BXSkp0OVR0NVphSjBIU0c5Z0VVdlJXVDFZUmFt?=
 =?utf-8?B?dldZK3VCSmlpMUpvUTNHTXNaSXRmM0xTWG8wMmpGV3Y4ZTJTY0hiODRIWkVP?=
 =?utf-8?B?NGJSWk9CMEhXSnM1MGxGeUZpb1hJOUxoaTlEaXNhb0xMNm9JYVZ6U09IWHFZ?=
 =?utf-8?B?VkZjOEllVmZWeHBNZWpLUVFvNzFUd21JM0I3ci96dXVTaVpLaG5vS0J0QkVh?=
 =?utf-8?B?MDJkUTdpWDZQMGI4dWZ0eHhFbkR5RTFzWEswVCsrelhDdDZSWFRxVmczdEdR?=
 =?utf-8?B?ellIa0dtVHhBaW1Oa01MaFRsMEJiUU8xK0VBdG9USC9kYVBGWGhkRmhJUy9h?=
 =?utf-8?B?b3k5RWRUd0FGM2VVcWMxMVV2Sk9aUzBPbCtCZFFGcUZOMEdxRHB4cUNPQXFv?=
 =?utf-8?B?TytwcmhmVzl1OXZ5SmsxUEUxUUtTV2pyaTE1NmowMkhrNHo4SklIbUdjYkFC?=
 =?utf-8?B?VEJKZjFsT1lEaVRDWTZJZjRYOHc3YmZoQUxNM05RNzNlall3WklVVW1XdTNX?=
 =?utf-8?B?WFlxMHNsR1VsSk9FRUlyQ0RScFBYUVNyaEljYlRKOURQMi81cEFUREdrYnV2?=
 =?utf-8?B?QmxtTlU0R1YrODE0WEFDdHpaR3FUc05KSVpxaGtkMzZnNncxbU9Eck5ndzBy?=
 =?utf-8?B?L3ZidUlEdi9iMFJ3ZTJVL2hXUmVWL011TEFsT0oxaXh0MjlhUm5pOTMzZGVD?=
 =?utf-8?B?Qk9VS2hHYmhIVHhNVXNUMUVsMFJBR0hScmFOamJ6YmRhdmRqYjROSzh2S09j?=
 =?utf-8?B?RmFnczJYaEpnR3gzcTdVQjlpTW0yd09LN04xYVc5MkJzT0g5eDFnNWZnRDV0?=
 =?utf-8?B?OVlkeFFOSWhYcUlpMU95clN5cFdyZFlrRnNqMTBjUkdyeVhnSVdMVHc4b2RT?=
 =?utf-8?B?aUpzS1Zzd1JTVDR6VnhibHY1SXBzQW1NUnErbDE4UERZdkl4NUI1WEJ1VkI2?=
 =?utf-8?B?NVpWYUliNVFpOFUrYmhZL0o5ZnZIUmpld2pyRjBjVzB3OG5uWGk4aUdXYm85?=
 =?utf-8?B?cS9udmw0NlVXZnpaMUorYVlvMmFTQnV3WGtQMndDemhIS0xmOUM4Ky9lcE43?=
 =?utf-8?B?S1NiV3U1ci9KZUdjQ3ZVWHBjNGwram8zbDlhODNPY0VXSFpDRWgwZFVyU1k2?=
 =?utf-8?B?Smc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	R0fZjTuiSw2TmwCAEa8p9wKdN3firqRZebdpGtRb4kVlc5FxGzoMkSdVCA76KQq9dH4YPif5qqpDXO2DbQOeDPlEOadPCZE6SGPwvkkQi/dq8NhAzJ23/tn1hadsLVtHr1V36mmW0o/649dFrcZ7LuUVyAljRhekMNCpls9AW12yElB3Tn+Lp33G2JUTHp7ZzqBRHAslGGIOxqb/CvyFg6mFeyxIsKet4FaPVwIbrnFsgxATSPPS/HOH4eYLdZF4RTv+YjkAV5itvlbhvgFwcxtgEsS4K6VKhGDaP8YsT50If1tD9eooGcBHwrWdhoVyEX5sqMXhVDuTEcPF1IEv6lOswmYjpx1peCorFI1hxywo41bAIzz6auLWzqPZpRluKclV0MQ0hHG0VY2N7kIZykieUdaX2Kc281kxjQyhoh5RLkeXpaRIdudl+AmE3fN4os4pRi9R/oCk+ktDOAMMC36/zye1nE3BWn2eli+GPnsSO0TOCjPOoSV7gdNH+AGUNHfNOxlFkwMKCJYLjop241OVWA+8otoRigBOM0fZUXkHEZnMHVLfnjOllfSQFuSGu13FwHzLDm1Qzd1TWr6PqnFKlvwuNF8b25lYMScxZqM=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 14c4a04e-607a-4403-add6-08dc7c0c8a9d
X-MS-Exchange-CrossTenant-AuthSource: CY8PR10MB7243.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 May 2024 16:14:08.8133
 (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: gP4y9riW2khLhwhDDAchiOjItcsv+xxAPDRsF03nRK54S0nsGnBwsrJW8pwLwCKci2dbytlzRGNty+WMUUjlTdk2SKSJo/JWz8nAH/jTaC4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB6400
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.12.28.16
 definitions=2024-05-24_04,2024-05-24_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxscore=0 spamscore=0
 mlxlogscore=999 adultscore=0 bulkscore=0 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2405010000
 definitions=main-2405240113
X-Proofpoint-GUID: ak9OaVYwnilBcICZdmdPa_hRxypRhQBq
X-Proofpoint-ORIG-GUID: ak9OaVYwnilBcICZdmdPa_hRxypRhQBq

On 5/23/24 5:21 PM, Khazhismel Kumykov wrote:
> This fixes issue where misbehaving userspace initiator could bind the
> same connection multiple times, which would leak the old connection
> socket without cleaning it up.
> 
> For iscsi_tcp, it calls iscsi_suspend_tx directly in stop_conn. Update
> this to iscsi_conn_unbind, which matches the lifecycle of other drivers,
> and clears the CONN_FLAG_BOUND.
> 
> Suggested-by: Mike Christie <michael.christie@oracle.com>
> Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
> ---
>  drivers/scsi/iscsi_tcp.c | 2 +-
>  drivers/scsi/libiscsi.c  | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index deb9252e02e6..1d93404515ae 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -696,7 +696,7 @@ static void iscsi_sw_tcp_conn_stop(struct iscsi_cls_conn *cls_conn, int flag)
>  	wake_up_interruptible(sk_sleep(sock->sk));
>  
>  	/* stop xmit side */
> -	iscsi_suspend_tx(conn);
> +	iscsi_conn_unbind(cls_conn, true);
>  
>  	/* stop recv side and release socket */
>  	iscsi_sw_tcp_release_conn(conn);
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 0fda8905eabd..0fb98eb53584 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -3453,6 +3453,12 @@ int iscsi_conn_bind(struct iscsi_cls_session *cls_session,
>  	struct iscsi_conn *conn = cls_conn->dd_data;
>  
>  	spin_lock_bh(&session->frwd_lock);
> +	if (test_bit(ISCSI_CONN_FLAG_BOUND, &conn->flags)) {
> +		spin_unlock_bh(&session->frwd_lock);
> +		return -EBUSY;
> +	}
> +

If some of the driver's bind_conn callout fails then it could leave the
ISCSI_CONN_FLAG_BOUND bit set, and this will fail on a retry.

It's ok to call iscsi_conn_bind at the end of the bind_conn callout
like how iscsi_tcp and cxgbi* do. So for iscsi_iser.c, be_iscsi.c,
bnx2i_iscsi.c, and qedi_iscsi.c you need to move their iscsi_conn_bind
to the end of their bind_conn callout.

You could also keep the the existing flow but add unwind/goto error
handling to those drivers, but I think just moving the iscsi_conn_bind
call to the end of the function is easier and it syncs all the drivers
behavior which is nice.