Received-SPF: pass (google.com: domain of linux-kernel+bounces-108058-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <3abd7f0b-dc88-4183-8762-f2f101402edd@oracle.com>
Date: Tue, 19 Mar 2024 14:28:52 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] iscsi_tcp: do not bind sockets that already have
 extra callbacks
Content-Language: en-US
To: Khazhismel Kumykov <khazhy@chromium.org>, Lee Duncan <lduncan@suse.com>,
        Chris Leech <cleech@redhat.com>,
        "James E . J . Bottomley"
 <jejb@linux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>
Cc: open-iscsi@googlegroups.com, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org, Khazhismel Kumykov <khazhy@google.com>
References: <20240318194902.3290795-1-khazhy@google.com>
From: Mike Christie <michael.christie@oracle.com>
In-Reply-To: <20240318194902.3290795-1-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?VkV5alNNbldqQTVpWTdJVm9YL2tsMzIvZy8zaGJ2MG1udmIrNHJkMGVkelAr?=
 =?utf-8?B?a2JUTzdSeU9DWXE1M1BXM2J4Y3pnVHVDdEtNVm1kM1B6bFBlYzE3ZlpYQjg3?=
 =?utf-8?B?M3VmMmxJNlZ0OFF2aVB1T3RHaC9nYlB5TEZQR2I0ME40eUZjcmZWcW9TRXY3?=
 =?utf-8?B?WkJpLzh2dDhwaUtFN2RGbFdHM3A0MzMxc3orNk5EK2ZWOWVaazZQeCtmc3dM?=
 =?utf-8?B?cWhTWDJ3NTlNU3haQ1Q0aldXL1BNSzdKanlDc0I3ajYwRy8zWE5sN1I3aVJN?=
 =?utf-8?B?TzRHWGUvQVZBaEVKck5mYzJQWVhJQVR6S1BmU091SHdleHVCUUJDSUpWR2hK?=
 =?utf-8?B?MlpqS0xTSXk4aW4vN2RLcUw5V01NdlUwYzBGcU8wQTRmakI5bUg2NlA5Njdm?=
 =?utf-8?B?bzA1aUpIWmthNzBQZHhDclVkU2s4UGtMQXE2aWVmL3JWYUhXbGFQZ3hoRWFN?=
 =?utf-8?B?aFVRR1orWC82emE0M2pMWTZmZEw3RGxQVnZjcEp3N2ZXWkZmUVJYU2lMdlB3?=
 =?utf-8?B?T2tzdFgyZzhOOXBGNDgrWHEyL1haTENLM01qUVM3TzMycERDMnF0NnJ5dm5v?=
 =?utf-8?B?YlhXMW1EeXZCZElVVjI5ODVaNnNXbXBlL05xNEdLZ2Q3TXRsbG14dmF1dVFU?=
 =?utf-8?B?TVFDVG8za0pTSVR3R0d2SEFTaERxcDIrNEJqQXhaRjFzK1NldlE3NTFLZTZw?=
 =?utf-8?B?OUFTalZHL0xhbGNERXlSVGFKWDltS3laOElWT1BuUmtRY3BlVkkrWS9XS0ZY?=
 =?utf-8?B?RGprVlNBbFVVS3V3NUI5Rm9pSmVaQ245YTY1RElVQ1h5L1hqWkptcHFXUExC?=
 =?utf-8?B?dHpUa1k2OFY4djJKQ2xMTGp0dVFUTlhWUzRuSHRybS9ER1lkQzNxd003eml2?=
 =?utf-8?B?Ylh1WFU5Y0s4Sk9McFo3cFJ2RHYvZllGVjlYNC9pMFNXdjBOZU5lSm92MmhC?=
 =?utf-8?B?b2RyT1JVd1RKcGRKaGZPYTQvNkF4Wjdnb2hxNFJPa1ZEdmpzSFRrQVBGVFVj?=
 =?utf-8?B?VXZ6WGlYZ2NIZVRSRngzSDdINk9Vc1IxL1R2cGx6cmQ1YTZIZ0VkZWxsUXFT?=
 =?utf-8?B?dFBoZjRmZFUvMG9BSkFLYi85QTdMT1NiNmd1R2xMczlIVU50S2g1UjI2WUM5?=
 =?utf-8?B?eCtlWmFSamxPZjlUQ3VWVkpkSnFNTXRQVXBCZzRvQjJqQnV3K0VUSzNEY0di?=
 =?utf-8?B?K1dmVnRkM2pwR0FsUUk3emY3UUxZVnl6YmFEK05tOUNLOGxNVXFjVE9BMGV2?=
 =?utf-8?B?dWdRN2V1OWRrVFAvNWtaUnUxQ1pMV1djeTJUbHIyQTg3cFBnNjhUTHpOS1lr?=
 =?utf-8?B?SEhXL2RuekwwRzhHWjk2cjJEcEZTTzhzdW5vY200T2Vnc2RVemF1UzhpNjZS?=
 =?utf-8?B?Y2JCdEl1TmRuSm1IUXNqSWQydStYY2o0cnZwcmRhVGJOYlVLK2pyNUxNUmlJ?=
 =?utf-8?B?K2MrNXIzU2tuWVFMMVBUcnZULzF3eEVWeGtwanlPcTJYamRpbThORXZuSS9R?=
 =?utf-8?B?NFJEM2RESGJoUGNVQ0xSMG5yWkJxVWlwUHlJUjJQMGVvY3ArWlJ4S3B4clVZ?=
 =?utf-8?B?b1NGcDcxZ1QxVi93Y3p4bWpJRFBpSDI0SUdQRjFOM2F2RnBiNFFoWWcyekdQ?=
 =?utf-8?B?L2hnVTVYdWhoejhNL2lleFZuektrUERCNFVhdlJUaHNsZEdURUhqTWJqaVZw?=
 =?utf-8?B?QUJNS0lRejN1MmNlVDdLV2Rzc1IrMkNxUWtMMW5CQlplSkdaak9kNlJFU2I5?=
 =?utf-8?B?dWZIQjBpN29RVUI5TGVRMlJCK25mZmo0T2F2c2xRR2N2RDZ1OXA5ZkN0MFRh?=
 =?utf-8?B?ZnBLSm4weHV0WWp6czBNSkxlRW1IVWcwM3Mvb2dkVFcvMVY4N1BlOVRlc2hs?=
 =?utf-8?B?ZkhWdUFCM2dHY1kxRHQ3T0g1MGJxaEl4MEdJN2Z4VXFmVDdZYkZBYlFIRita?=
 =?utf-8?B?M0d4QWJTNVlxYkYxcEQ1RmYyVDhEM1VqMFFzdTUvSmtlUkNhRnRIa3JUbllt?=
 =?utf-8?B?NExwM0RxQWhtc0VmbU9FZms0S1MvaS8xZS9kQ0hpTGIvWHh3NWJCNzYwZEJr?=
 =?utf-8?B?eWdTWWdodnozazFSTkxJU2dhWGRwZlFUcnlYWHhCUDY1cXNieE1kSDBXYlcr?=
 =?utf-8?B?VDc2dFZ2R1N5YjJuS25wSzFsWjJCTGh3Q0ZLd0MrOEU5YkdzMjZyMTRqb3kx?=
 =?utf-8?B?K1E9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	zbEpqD0s75KvQtVzuRNBndeAuWYJfNcjMkeCiD7lUv6sJzdTxVmzSEG12/UYebCbCvsA+E5W1kMWf2i2PGWLNKUEtb4BPRJKt3c0n1Rg/+QU19jk65qvPJJDp8lCjR66z9jNWhb3x8JP4BJl7n8iDjUP0hL9EBXNv2GLJoSaNmdJeyMFrhjur3PTXu6Rs+sTtKxH/BrJQty5Sz5qGxcW48hq7NuixLOWEs97SQAgVBkN6zZio7z9MeoR3QGA0m8i0hGqtODBB8Pti83usoropBt1HDceJAY2t/aSZwR9XAuCgskqLOIq3VEua3m+4iFTv6htKaPjrZnlYGAOpfum5qcl3q1nIS4x33KijgoBa8NRLGC3Sy7R6YdRdprtSUjkGvqZ7oW2yycMyrzPyZjcZaJczsHQULe7BDy9t9Fcw9OU85FobUZ0Ha1CfjcUV9jxoqcXTI/uSQiox+Kc0UcuISQfPYO7xqdoHOJNLwfSljsL3aEaPMQnDK7AOa5Esfe9szxlCS2gIBYm9q8uadoaru2P0JReIS5a/XYhegRhl2ObBBwTLBwjLEly9NRcwkUhaWvW8etHDixzNIulAhgMcq4czPpNc7yA4Rya+uqJDuM=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2bd289e4-7d93-478f-05b5-08dc484ad06b
X-MS-Exchange-CrossTenant-AuthSource: CY8PR10MB7243.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Mar 2024 19:28:54.3270
 (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: ix4puzSB3VFfiKqFKw3/l0cyM345SJ0bgEMxTdfmhb3mgpM3o9aNcR+m8dI38W341pEttB4PHEPNxECV7c+AWSrW4sMV4+N2d6OdD2hjrVo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB7010
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-03-19_08,2024-03-18_03,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxlogscore=999
 phishscore=0 malwarescore=0 mlxscore=0 spamscore=0 adultscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2403140000 definitions=main-2403190149
X-Proofpoint-ORIG-GUID: zzp9Yzf8E5iGrpcd2twIHuplbdxIR5gP
X-Proofpoint-GUID: zzp9Yzf8E5iGrpcd2twIHuplbdxIR5gP

On 3/18/24 2:49 PM, Khazhismel Kumykov wrote:
> This attempts to avoid a situation where a misbehaving iscsi daemon
> passes a socket for a different iSCSI connection to BIND_CONN - which
> would result in infinite recursion and stack overflow. This will
> also prevent passing *other* sockets which had sk_user_data overridden,
> but that wouldn't have been safe anyways - since we throw away that
> pointer anyways. This does not cover all hypothetical scenarios where we
> pass bad sockets to BIND_CONN.
> 
> This also papers over a different bug - we allow a daemon to call
> BIND_CONN twice for the same connection - which would result in, at the
> least, failing to uninitialize/teardown the previous socket, which will
> be addressed separately.
> 
> Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
> ---
>  drivers/scsi/iscsi_tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 8e14cea15f98..e8ed60b777c6 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -725,7 +725,7 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session *cls_session,
>  	}
>  
>  	err = -EINVAL;
> -	if (!sk_is_tcp(sock->sk))
> +	if (!sk_is_tcp(sock->sk) || sock->sk->sk_user_data)
>  		goto free_socket;
>  
>  	err = iscsi_conn_bind(cls_session, cls_conn, is_leading);


Reviewed-by: Mike Christie <michael.christie@oracle.com>