Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <85a64450-99c8-268d-1ac7-86e70cbb3562@oracle.com>
Date:   Thu, 3 Mar 2022 08:59:06 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [PATCH 2/2] iscsi_tcp: Check if tcp_conn is valid in
Content-Language: en-US
To:     Wenchao Hao <haowenchao@huawei.com>, 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>,
        open-iscsi@googlegroups.com, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     Wu Bo <wubo40@huawei.com>, Zhiqiang Liu <liuzhiqiang26@huawei.com>,
        linfeilong@huawei.com
References: <20220304025608.1874516-1-haowenchao@huawei.com>
 <20220304025608.1874516-2-haowenchao@huawei.com>
From:   Mike Christie <michael.christie@oracle.com>
In-Reply-To: <20220304025608.1874516-2-haowenchao@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?M2V3S2l6eTJXKzZHMXBuVTdqcWw5NGVIWFBwSWhkb2ptdWwrVjdFZERrWk1I?=
 =?utf-8?B?NE9KOGI2bDNtOFVsRzVJU3VESGRoWDhpZW9OZ093WDl1b2tRcStNVmVUTXQz?=
 =?utf-8?B?d2FmVmVFS2cxUnhuejd2WVFBL3FjYTNEWkVyVkVkbTd3cUg3Nng0VDB6M05p?=
 =?utf-8?B?OWV1RWpxVXRZWC9wK21JTC9vSzhlNlBUeXZOUUdsd0VVQXF4cXdDdS81L2ZM?=
 =?utf-8?B?TGdBUXNsbmJ5M1V2RU9qb0E1OUprQnN6NUtocWlIR2xrK2NxVm9yQkd4R0cv?=
 =?utf-8?B?eldQUS9SV2huSjBRMTZkZG1obTlGeUgwOUJqTllPOWwwcGZSeW1OMnRCb3My?=
 =?utf-8?B?VHhTTGlwcGF6dVNzV1l6S1o0cklYKzlRaENMaU5XZFkyV1JsOFlhWDEyMUd1?=
 =?utf-8?B?d3pXWjVLWkVOczJaNDI5dUpXL29BaEgyZGJ1RVdOZFFsQmpxbWo4T1hXb3lx?=
 =?utf-8?B?Y3J1N1c4Ri9odkFjdy95YzlRQVhDVHFBQUVFdWN6bXpYbDU4ZE5jcEsybHBa?=
 =?utf-8?B?clVIOWViVm1iVU9mNnRxL1o2NHdTL00vd29sNHZjSzUvSEdtT2tIbGFnTmti?=
 =?utf-8?B?UWR6UDV0SzBCcnE4T1JYOWZHSXNYUjFaSWNLWk90Q1N5RUpCTnZnVGVEL1VI?=
 =?utf-8?B?QmswUUJacTZRMm9YM2locStZMWsyaFVQWmtLbStyVjRIczJrV2pCdGx3cUt5?=
 =?utf-8?B?dFk3bXVLN1RrdkFoR1IvQTZ3T2lhRk1RaUpCRmZsMTdjZ3pScldraVhreDdz?=
 =?utf-8?B?TkVxL1pZeVJ0OHo1Wkt2RVRLbG42Q0dESTJ4V3MydU1sbzZCNTZrRUNZQlVx?=
 =?utf-8?B?OFJQOVFjZWpRWG9MNnVTRzZ2WWhBWExwdFFaVlhvUmdsYjhEUHY4TUZ4YUVk?=
 =?utf-8?B?bzZndVJ2M0ZPb2pZQzZRcVQrWXpNK2pzVWpJeEFXNWt4b2dpSHZidWt1M1lD?=
 =?utf-8?B?dkQzdnVGTEhFQ0JzaWJITmlOSzk5bTRidGxXRVp1VUllOU1KbzhabUJOOGFJ?=
 =?utf-8?B?RVV2dHpDY0FaeEVNZFVxVHMyK1lyZlp0Nm4wQnpNTHMxZjVRWnhkWTczT2kx?=
 =?utf-8?B?SWdLbVpqcTY1bVVVRFZKbWp1YVY4a2Jpd0VmaXN4RzFqcVFGWEhGTXZ1dVNI?=
 =?utf-8?B?eWJMZ1JaSGdmMHVvcWtXUExSYUhERHR1bm95K2FSb2d2aE4yY1hXdGd1elV2?=
 =?utf-8?B?TUR4bnJNa1crUkZ3WTJSekFzbG5TRU1lR3ZpTy9OVklQQUozZEkveXpZUFl5?=
 =?utf-8?B?b2QvaUhyQi9uVlkvQXU3MzZjMUdybWlwS0VCUVh4aGF3UVh5L25VTWZFY2lH?=
 =?utf-8?B?UG5jd3JRRlJ2Z1QwZXlsUzZSQkIxOG1TMGhoMGNBOGZEUnVxOEtiMjhOYUth?=
 =?utf-8?B?VW1kN2N5QUd2Wm55aGxCOXdTVC9qWnVZdlF4UEFNWjJROHMyYWVSNWNHcm9y?=
 =?utf-8?B?MnJXOEhEOHNBdE5kc3RJTndHUEdtdUFCdFRvVEJ2WElzN0F3TktJY0xUY1lC?=
 =?utf-8?B?OThXZXlVNlRTbG9rYVVMUEYxWFR6Qkcra1JZQUtVWUtBTnNoWEFzL2tBeG9U?=
 =?utf-8?B?ekNxVWIrbXFjcGtZYkJaSy93QXNQNmhCR1Q3VzdMM3NHTnpBZFp0YkFyWSty?=
 =?utf-8?B?OWhBOXZPRGhDN0lTdE9odG9nd1VKbENZYk13UE5KclQ4WjF2Q1FueExNTEJX?=
 =?utf-8?B?RTB3V0QvaHFjdU1QeFo1YkxwRnlxa1VlcGQ1QVlRdVlWWUY2ZlhHL0ltaUxW?=
 =?utf-8?B?UGhmUjh6a01FUmtUdGhBbGJ0R3Z3UGRXNW13Z0l6OG5GQzJ4RUN1TEpveDlY?=
 =?utf-8?B?MHFFc25ZT1Z3RS9oemFGd0pUOFMxczEwUVZlQU5MZEREM2h0dEJLdDdFU1FB?=
 =?utf-8?B?YjJ0TjBYdlZ2QTdpeFBiekJXYStkRmlxdGl2UCtlM2UrMDZqaitYbTdHamxM?=
 =?utf-8?B?MnI0K0NIWHZ0TUw1d2ltOUpSOTlxOEdreUYxQjRaUVFnUXBBa01PajUvQjQx?=
 =?utf-8?B?alNRRnE5dFV2aWJwV1BMQ3Z6cnZ6UkROQzBBVmdwOHowRW83LzBQb05WY0Y1?=
 =?utf-8?B?MU0wRnhSRDJ3dmF0SDRXQ2IwdGhZWEpXbFBwcERYQnpaSm9qSHg1VjhIUEZy?=
 =?utf-8?B?TC83L0pFY1B0YlY1MVZSU3BYdjdFYWhVL2pMcG8zMkRGK2swdklEaTlObVBJ?=
 =?utf-8?B?Nlo5YSt5VUpnTHpOY0ZSMTd6djJNTHhQWVRDMmU4S2FNSUlaMjZPMW9IRm1z?=
 =?utf-8?B?dUZTNUwvdTV0cXpJdk42VnlQS093PT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0d8a0adc-e33c-4519-dbf6-08d9fd265e69
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Mar 2022 14:59:08.6560
 (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: pa8ucy6G4b2A+xAutUytPd3Beoc7AdcHmMbyeEg5NZPrY9v84EM70u+Ms1s89B17Tj/JkOZq5/sM+vIdQMa9Cj7Pdjxo1jtiOaHhBbm36L4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB2811
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10274 signatures=686787
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 bulkscore=0 malwarescore=0
 mlxscore=0 phishscore=0 suspectscore=0 adultscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2203030072
X-Proofpoint-GUID: QBmCLm0Op-ixg7FgvjCo84tk_Bq9UHIf
X-Proofpoint-ORIG-GUID: QBmCLm0Op-ixg7FgvjCo84tk_Bq9UHIf
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 3/3/22 8:56 PM, Wenchao Hao wrote:
> iscsi_create_conn() would add newly alloced iscsi_cls_conn to connlist,
> it means when userspace sends ISCSI_UEVENT_SET_PARAM, iscsi_conn_lookup()
> would found this iscsi_cls_conn and call the set_param callback which is
> iscsi_sw_tcp_conn_set_param(). While the iscsi_conn's dd_data might not
> been initialized.
> 
> Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
> Signed-off-by: Wu Bo <wubo40@huawei.com>
> ---
>  drivers/scsi/iscsi_tcp.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 14db224486be..a42449df6156 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -716,13 +716,17 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
>  {
>  	struct iscsi_conn *conn = cls_conn->dd_data;
>  	struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
> -	struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
> +	struct iscsi_sw_tcp_conn *tcp_sw_conn;
>  
>  	switch(param) {
>  	case ISCSI_PARAM_HDRDGST_EN:
>  		iscsi_set_param(cls_conn, param, buf, buflen);
>  		break;
>  	case ISCSI_PARAM_DATADGST_EN:
> +		if (!tcp_conn || !tcp_conn->dd_data)
> +			return -ENOTCONN;
> +
> +		tcp_sw_conn = tcp_conn->dd_data;
>  		iscsi_set_param(cls_conn, param, buf, buflen);
>  		tcp_sw_conn->sendpage = conn->datadgst_en ?
>  			sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;

Is this something you hit or from code review?

We have those state checks:

if ((conn->state == ISCSI_CONN_BOUND) ||
    (conn->state == ISCSI_CONN_UP)) {
	err = transport->set_param(conn, ev->u.set_param.param,

so we don't call set_param until after we have bound the
connection which will be after ISCSI_UEVENT_CREATE_CONN has returned.

Also for this specific bug iscsi_if_recv_msg is called with the
rx_queue_mutex, so set_param can only be called after the
ISCSI_UEVENT_CREATE_CONN cmd has returned.