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: <a3db8aab-8d30-d470-5eeb-a2cb4195a122@oracle.com>
Date:   Mon, 24 Apr 2023 10:22:42 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH] scsi: qedi: qedi_main.c: modify the code flow in
 'qedi_alloc_global_queues'
To:     Peizhi Li <meetlpz@hust.edu.cn>,
        Nilesh Javali <njavali@marvell.com>,
        Manish Rangankar <mrangankar@marvell.com>,
        GR-QLogic-Storage-Upstream@marvell.com,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>
Cc:     hust-os-kernel-patches@googlegroups.com,
        Dongliang Mu <dzm91@hust.edu.cn>,
        Dan Carpenter <dan.carpenter@linaro.org>,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230422101626.1722235-1-meetlpz@hust.edu.cn>
Content-Language: en-US
From:   Mike Christie <michael.christie@oracle.com>
In-Reply-To: <20230422101626.1722235-1-meetlpz@hust.edu.cn>
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?SkJacEtpMDFET24zV3FWNlVMMEpUNW9sNTI0NW5xYjFzalJVY3RYYjBRU1pn?=
 =?utf-8?B?cC9kUUt4V3QrZ0xhQUQvaVppRnp6V3Y4czRSZm1mZWN5cWdaVTA0aUpYSHR3?=
 =?utf-8?B?c2N3c3RJZE53a3EvWXNuUC9jVUg4M2d2NldlNnBma0F5TEI5K2hlRk5sbzNS?=
 =?utf-8?B?MHJmMFVnUTd6d3dhTVh3RGYwcXNobXU5QTVzVHNuWGgwcFhpdDhkTmsvY2NV?=
 =?utf-8?B?MlVQWVF2eWZQK1pVZzNVelVBM3dVbE9RWndoNEhhaDVPTmV0eUNEaFRuUHRM?=
 =?utf-8?B?WFlRL2FtTk5VT3ZPRUM4VzdJakxwOWdBMVYwb0paSldnRXpRWDBQeDlqQzdY?=
 =?utf-8?B?V2NJeUlXaXRTRWZIL0hqOU1RNVV5VmJCN3JuaW43ckY3SUd2cHZTM2hsUVZm?=
 =?utf-8?B?dFFORFhZYUNGQkJheGNPVXZOMjVwcll2Y3ZKcVVLZHlVWlE5K0F2dUJFSVd2?=
 =?utf-8?B?QWZ6WGZ4TU1HYzl2ZTljand6VFdnbit0a0FKVUFobHdPKzh5SVowRDZ5OG40?=
 =?utf-8?B?TDNrK0IxOGJFTm9vK3NlczJLMlVocTRocndyTzFyTkhBRUlEVTlHM3pIMzVw?=
 =?utf-8?B?UjZkZFFKV0MwNm1kTXJGTHVkZGZnZE95a284S1dNckg3d1hWR3dxZ2hDS3pF?=
 =?utf-8?B?Mnc0dnRkNlZySEkydlM5ZTdZVFltMkV1SEhGeDRCQ25MWXpldWdUTTh4WGxY?=
 =?utf-8?B?RHpvZWRiOVI2bFhxMjVFRmpFaThGbmJ1WXdCd1lLL2RGRENNRXEweEVQeFZL?=
 =?utf-8?B?QUgyTWdUbmxaTTBoL21EUzd3T2o1bUNiVFJtb0lPWDhtaVN5K1pFOEl6ODFZ?=
 =?utf-8?B?REx1OFdMNzVmU2daV2VkRkFub3dCWmQvS2RaTFpPeFVkNnBjRnNPT1FaUjJS?=
 =?utf-8?B?YXpVRDIwaFNXVjlRN1pGbUNKSmxQWkFFcTNRMGNzd0JKdVQ4Y1dNV0YrS3dE?=
 =?utf-8?B?MVlwN2RUUmFWTXFhckJjNXJRT2luV1d4bUp0TXkvU3B6RUh3VklPNW5mMzBN?=
 =?utf-8?B?ell2RXRYNVBGVmxRU29tTnpVb1JUdy93QXovNVd0QW1XcVJPTytOYSttMG92?=
 =?utf-8?B?cWtQWC9NVHk1eVpXVEdsZkVURlRtNzh5QXV0b2liWkJHNjg3UVN5TmpxaUJr?=
 =?utf-8?B?SkFiYjlvWW1Wd1JNQTZvSkdFQ0M5MC96SFFnZFRmT0tVZ0JkblBqTndCckFE?=
 =?utf-8?B?YnNhZThPNlpFL1Q0dVFkSW1DWW90L1hIMS9DSzZkNmxJNUhDQmJDTTJVL1pM?=
 =?utf-8?B?T2I1UFdZbGRFS2Y1NTZBcER0bFc3VkgwbDdGWENWOWE1M0w2WU12RmZtRUkz?=
 =?utf-8?B?ZnJ1UTlTOFdUS1E3bi9TVVpCMUJ5WjV5QitUMFNqb2UxN1d0SUNXNFFCV2xv?=
 =?utf-8?B?SUVHVnFXUjloQ3FxWVFHU2d0T1JXRDhkcEVWS1A2Rk1oWjl0cTg5UWhkUk1M?=
 =?utf-8?B?RU91WGp1Tk82TzRFTDBSVUhJMmJLOTFKTU5qOWNyUmt0NkkzN2J2OUFCYXd0?=
 =?utf-8?B?STlhTUJ0OHZQUDZ2b1ZKYkhqNEN3SmxEV2Y5N0lOcy9yM1pzQ0dlMGlvcUxO?=
 =?utf-8?B?UDMwUEVKK2h2WnA5bVJSRDZnN3VTcFVaWjdFWFgrc3hKa1A4VXlSZlFpaVll?=
 =?utf-8?B?SVUva3VJcVgxd1lKdlYrQmFkRjQyb0NUN084MWFCdktBSURRSU5DQnV1dkV6?=
 =?utf-8?B?T2JEV2RzREc3bjBYcDljeXRURy9hRlRCbWx1UkR3VFhwSzNoc0RKemRNczZQ?=
 =?utf-8?B?dXMyYVVnbFpiSEJGMzNwaEZlYkk3U3BqUWJXMGYzRHJ1cHQzWDhWNSthWUhO?=
 =?utf-8?B?QUI5Nld1d3hUTkMzTE42ajhTM1M2Sk9tdzdQVXZDSU5qUzRGY2RkVzBQQ0VD?=
 =?utf-8?B?WDJSRlAzRm1iUzZ5V2J6Vm9IdnRvNURoWk5MQzNhVHc1TytDSzNwN0NPSVND?=
 =?utf-8?B?UTNRQTBpUFk5WWIwZWxGUE96MnhHZWNuOGxJT3dLc1MxTmFZcW9qdnRXcDRT?=
 =?utf-8?B?elF4cisvUFV6dDBWZFg0b3ZrOWc0andZUnVpTUZyNTRVTkpnQXFlZ0ZoaU9P?=
 =?utf-8?B?RHp1MUg1U0xkb1ZFUmFWWTFKMmZpSFdMY3U4eVpEOG5GWHh5K1oxbGdieHpj?=
 =?utf-8?B?Q1hnU3Y5bVl4OXlZdmlHSlErZHN3NENBSG95UEppVXZHcEtsSWoxM0FKSURq?=
 =?utf-8?B?bmc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?WVNiYmVUQ28vVkVIV2JMSmM1R0pDSitERTlqRUdPNUdzVkk3ZG5lTTdibTZx?=
 =?utf-8?B?aVM4YlpKeGRES1RnUytLRE1RQmtGYVdaeWFLa0QwNEcyNE0zQ3hIMXFJMHpY?=
 =?utf-8?B?QnN4a1NJOFdyU0lWNWlPS0lxeVc5VnVCWkwzMXQ3L3gxTnNxSkZScU5Wczk4?=
 =?utf-8?B?YjlURDk2SmpGYXNnTElORi8yMmdhd0gwak1YNDhiVWNZRDlKclBTbE1jR1Np?=
 =?utf-8?B?ZVUrWmpzTUoydnpYK0Y0LzFyUXlWczNkMml4cTc0ZXl5alVuWitnU3BjTWZE?=
 =?utf-8?B?dFFQaFI5aEE4NlFpS0pkbUt1Z0JXVDd1RWJKWEtPOWZ0amJlRFVCcVkwbHBG?=
 =?utf-8?B?VlNHcEtoM0ZmT3ZoaVhsZFZSSkR6b0ZpdVhhdDduRzFXSkJFWEdqK3hwTGlE?=
 =?utf-8?B?TXFueXpmbklsQVJlTmQzbFJScElGZWtuYmVmNzZkMEhRbUdwQ0dkTksrTGVR?=
 =?utf-8?B?NGRsQzhja010aU9vQlo4bVpHb3BjNlp4RHBJM3Vtd1JuUHBRM3B2MnhJTkVh?=
 =?utf-8?B?ZnlrSkw5YzlCS0FLR3ZXVmR4UEtHZEYyb1FqRFZjMDJQaWpCRjI2a0htVVRx?=
 =?utf-8?B?aVZVNVk4UG01L3hlMkJlU0l2NGdvK2JwZDgrZnRuWmpqa1Q0QWVJL0ZPckk2?=
 =?utf-8?B?b24vNm82WHZzd0w2cmMxUWd0QVlhV2xmRnpGRUNIb2F6VGNPK0Z1bVpUZzRN?=
 =?utf-8?B?Uk1oR3MzRXZuQ1cyN05FM3lRd3c4eSszdFRSMkdzS1ZxOC9nd1A3VFE0c1lY?=
 =?utf-8?B?MDdObiswd1hGeE9yLzFxZTJTMXhORjl4cUJaYkQ4REh2Z252OG9tb1l1NGU0?=
 =?utf-8?B?cTRTTkxEc1h6d0IySjhIalpQNkdSSUx1QWtlKy9XS0VFUWpYTHJuWnAyREZT?=
 =?utf-8?B?QkZDMmZFMGc5MWhPeXZhMndjK3dVeUNnQ3VFb3I0c3U1MTVTMHYrZkJtd2tj?=
 =?utf-8?B?dlM3eExmWFN5QkgxWDJVbGtHeW5WM1dGa1ZLZXNES3V0TERNVU9QM2dJeWxs?=
 =?utf-8?B?bEdpOE4xMllpbnBuUEdwL1VSSSsyVEh3N2JJV2VZWXh1OGErM3BRQVY0VGsx?=
 =?utf-8?B?bGNUeGJuN2tkT2hQQ2dCUHEvTjZKUC9OK3ZGTUQzdm15V0dWNGZDUjJNVmgx?=
 =?utf-8?B?a25EdWhkQm11cHhYUEF6dkxRTWIrY29iSlNoK2tNcjlOTmIxSnE4M2JqR3F0?=
 =?utf-8?B?aEdoM0doSGpnQ1hwVU1xTXFvWWh2RTY0b1FDcjFhd0g5RmYxby9ZMnovN0Zu?=
 =?utf-8?Q?LjtSeC9cCTBqfJy?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a6beb0d5-1ede-43d6-c925-08db44d7c04e
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Apr 2023 15:22:44.0792
 (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: fPBwbnhyODx7yohKrPpR4KGNAK4y4VyiKJyXJI87JkfvHRxVLA+HORxHuVxBmSoTMXGqpoIckVtwSIjUi3RWY3Ou+Nss64kweH3nM5j/2xk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB6509
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-04-24_09,2023-04-21_01,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 phishscore=0 bulkscore=0
 mlxlogscore=999 malwarescore=0 mlxscore=0 spamscore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303200000
 definitions=main-2304240139
X-Proofpoint-ORIG-GUID: QVWa34NKLYUaMIkbl_2B_qj65RVfE3yF
X-Proofpoint-GUID: QVWa34NKLYUaMIkbl_2B_qj65RVfE3yF
X-Spam-Status: No, score=-4.0 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_H2,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 4/22/23 5:16 AM, Peizhi Li wrote:
> When goto the mem_alloc_failure, it will result in a null pointer 
> reference to variable 'gl' in function 'qedi_free_global_queues', 
> due to the 'qedi->global_queues' not being allocated.
> 
> Fix this by returning -EINVAL directly.
> 
> Signed-off-by: Peizhi Li <meetlpz@hust.edu.cn>
> Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> The issue is found by static analysis and remains untested. Meanwhile, 
> this patches is similar with qedf which Jinhong Zhu fixed already. 
> 
> https://lore.kernel.org/all/20230417135518.184595-1-jinhongzhu@hust.edu.cn/
>  drivers/scsi/qedi/qedi_main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index f2ee49756df8..34f38e0f1c7d 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -1637,8 +1637,7 @@ static int qedi_alloc_global_queues(struct qedi_ctx *qedi)
>  	 * addresses of our queues
>  	 */
>  	if (!qedi->p_cpuq) {
> -		status = -EINVAL;
> -		goto mem_alloc_failure;
> +		return -EINVAL;
>  	}
>  

The patch looks ok:

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

Looks like there is another issue in this code path though. If
qedi_alloc_global_queues fails in qedi_set_iscsi_pf_param the err_alloc_mem
goto just ends up returning, so the p_cpuq allocation a couple lines before
the qedi_set_iscsi_pf_param call will be leaked.

The __qedi_probe call to qedi_set_iscsi_pf_param just frees the iscsi host,
so it won't be freed via a qedi_free_iscsi_pf_param later. So, I think
qedi_set_iscsi_pf_param's err_alloc_mem goto handler should free the p_cpuq.