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: <4c6939b5-05b9-1346-5376-82011b4cc093@cornelisnetworks.com>
Date:   Tue, 18 Oct 2022 10:24:18 -0400
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.3.2
Subject: Re: [PATCH] rdmavt: avoid NULL pointer dereference in rvt_qp_exit()
Content-Language: en-US
To:     Leon Romanovsky <leon@kernel.org>,
        Natalia Petrova <n.petrova@fintech.ru>
Cc:     Jason Gunthorpe <jgg@ziepe.ca>, linux-rdma@vger.kernel.org,
        linux-kernel@vger.kernel.org, ldv-project@linuxtesting.org,
        Alexey Khoroshilov <khoroshilov@ispras.ru>
References: <20221017142652.13906-1-n.petrova@fintech.ru>
 <Y05msUNgnQTWmXyh@unreal>
From:   Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
In-Reply-To: <Y05msUNgnQTWmXyh@unreal>
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?dkV5ZDRQKzg5Y24rd3NKNDRhN1p3dHhnRVh1TGxJbFZwT0xoQ0lldHpQWEVm?=
 =?utf-8?B?RVh4TlUvWFhDRXdEUGVPMUJoVXN5ajkxTDAvS3VDMk8zWUdlYy9Dblk0Smd5?=
 =?utf-8?B?TGFNYUx2b0R1bFVlNmNOdGtYcmVBeHZLTFFxUG1KY3lnWDZoSWdYUmJlZUxM?=
 =?utf-8?B?eTc5VUNwYXFaRktKWW9LZjBJcHRHRDVLYUN1Z1gva2l3TVZoSFpoTmhLdUxj?=
 =?utf-8?B?WDByZlBDb3k5Wkw1bXg3NDlmL1ZMdXM0cmhXQjBFMUVsOFRlRHRINE9FZngr?=
 =?utf-8?B?WUtkTFhTNHpZajJTMmRXanlyOGp1R2QzMitxS3dhbEVuUzM4VDh4RVo1ZFl2?=
 =?utf-8?B?Mng0QWxwWEgyTGpFSTVpWjVyUXBrWXFjWDNPTGQvUUFmYi95dDVHYy9vb0Nm?=
 =?utf-8?B?Nm5hemlmc0Y1ZEZTWGpwTXB0aDYvQXBXZGk1TmMxaXEvK2JSQkdqOEFRTjNI?=
 =?utf-8?B?VFg4c093dm01cFpXQ241WXZwNEVPKzZrOFpPdmExMGUwdzh3TkMraExEcjEr?=
 =?utf-8?B?RGNhZ1dtZEtpQlNSOHZiMnhYTVhXSXdKeHBkN0dYRGxoSDFMMHlVU1c3ay90?=
 =?utf-8?B?R0VpOFR6QnkwbHIxYmhGaVNENUMwZFpHNW02U25WQUx3cGpCdkdaV1lDMUox?=
 =?utf-8?B?WGZ6bEYzcmRFRkgxK2xGd2NiQXgxRnBTSDJkdkNzTmNsajB1MGZPTEJqUlV2?=
 =?utf-8?B?bHdLeURqV2tZTStpREtKUk5MWVNJSW9KdDVsL3V4bktYVmxJU0lkeU5TNEc5?=
 =?utf-8?B?WXdJYThMNloxZ3ZvSUV6a3BPZ1FPWjVtTzQwOTg0STlwMnpFZXVFTTh3SjI0?=
 =?utf-8?B?K0M0M1N2SEF0SGt4Qk5aU1F3Sy9sQU9yTEhpOGthZlpYMmw2OFh6SThHeHZU?=
 =?utf-8?B?VlhWU3pFc3hyWGMyd3BsUUtzUFVIeEVaQzVTNGpnYUxPMC9VMHJZQ3dFU2Qv?=
 =?utf-8?B?MExRc0lCMnNvNWZYa1I1MTNzdktEamY3WVBxTk9WQytjWlVDeXkwaVdkUFQ0?=
 =?utf-8?B?OVArZHRVOEQrVkYzTmZ2NENzRHQyNWpLTElRNnp6TkZ0RWl2M2FwamR1N2xF?=
 =?utf-8?B?bFJrRTJZcGVBRGZ0Ykdpd2tNK0FwZHIzSHBJbWd3ZnBZMW1IeVUweGtmZjBn?=
 =?utf-8?B?MFJ0Qk5QcEo2cG1GSU9YemVpVHBLWVJaaDROUGdjZys4Mkl4MjRVNkNKc1RP?=
 =?utf-8?B?d0xlWGU4MDJDVytYN0Y4dUZHZTFBSHVuNHpjRTV1OXliOHFhNFZyMndvRjZq?=
 =?utf-8?B?dUlTN1FhSzZBVFQwd09HZHcwNEpiRmUzeDVUK05CblpDdUlIWWhGbnVHNm81?=
 =?utf-8?B?RlhjcTNoKzJzd0o1NU5GaUQ4SVdKekxqMlBUQnFoZkkySkJtck1QZDUxVzZo?=
 =?utf-8?B?aE5selJmNXlWNytsQjYzaldJYXVjUldJUmpReVZ3N3Z2cmQ2TzFtcDEvYXJ1?=
 =?utf-8?B?ckhNRkZkUWZGVElJWWdia29hdzdaNlYxNUdsT1U1dzVGQ1ZJL0J0MFVadHJ1?=
 =?utf-8?B?L0E0Ry9FWUhSOTJMZlJsSEJVcVpTUG5zVWFVU05PalJ0WkJmNnllTHJ4Vk8z?=
 =?utf-8?B?cTQwR1JqVHpVY2w2NHJnK2xUTllsNXAwYUFwOHIzQXJISERpVGN0ZXFoL3pi?=
 =?utf-8?B?aW9FTERDcmNSQ1hibG02NEVkQ0lvUVdPRmw1bW9yQWhoekpQeitPUkw3Lyt4?=
 =?utf-8?B?SDZ0ZzV5U0l5T1RYQnRtUE5XV3gyWnlTam1NcHNrWGp0dHlaVFYvTnpNd3Nx?=
 =?utf-8?B?d0RUMmRtREJkckRIaDdhNnNoa1Z2NzZtblBOcmVKRFhNcy9nTjhUTFdMZ2xC?=
 =?utf-8?B?Z1BGY01TVGhVSkovZkdGNmR2UGptZ3hZb0dnYUhLb25YUWJHL2FRTHQ3ZVVm?=
 =?utf-8?B?aU9yS00ya3NhakJ4SUx6enRSU3NQZGQySWtyQjBKQW9jazV6dWdsbTQ3blpJ?=
 =?utf-8?B?SW90NS8wMFZpVjhrdSsrU3V5SDRKdVJCOW1OVzVmcGhjWWR3eTJyWElIMmd2?=
 =?utf-8?B?aFEwRTJNRWljbERTTm51anlNdDd1Tlg0RmQvL0xPY2Rld0NuRFZMU3pUNk9i?=
 =?utf-8?B?VW9KeDNIZVdpbnRwUGRFOGptczBuUHcyK3RhRHA3ZUdzKzVLTDBHdVM1cnI1?=
 =?utf-8?B?SURqYWMyNnVSR1NLZ2g2N3RFTlhBbktxM21GKzhQbGZUSmZ3TTVlWG1wUHcz?=
 =?utf-8?Q?pGFm1X2n5WQVyqzUfou3LO8=3D?=
X-OriginatorOrg: cornelisnetworks.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2e14b6d4-af0d-41de-9d7e-08dab11472b5
X-MS-Exchange-CrossTenant-AuthSource: BN6PR01MB2610.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Oct 2022 14:24:21.0347
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4dbdb7da-74ee-4b45-8747-ef5ce5ebe68a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RhmnzzaYZTpVVXho+sf/chgOnxxwDZBZk8Lqr1ZAsWqUmmzO8b0jeiEhefNeQr26XmOOA7ccHCU0sl6nagG+PZ2RP0LLATd4QpHiZ86Lcq4R3CPLDOPO/AsyiU82ezcE
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR01MB2154
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS 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 10/18/22 4:41 AM, Leon Romanovsky wrote:
> On Mon, Oct 17, 2022 at 05:26:52PM +0300, Natalia Petrova wrote:
>> rvt_qp_exit() checks 'rdi->qp_dev' for NULL, but the pointer is
>> dereferenced before that in rvt_free_all_qps().
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Fixes: f92e48718889 ("IB/rdmavt: Reset all QPs when the device is shut
>> down")
> 
> Please never break fixes line.
> 
>> Signed-off-by: Natalia Petrova <n.petrova@fintech.ru>
>> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
>> ---
>>  drivers/infiniband/sw/rdmavt/qp.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c
>> index 3acab569fbb9..06e755975f61 100644
>> --- a/drivers/infiniband/sw/rdmavt/qp.c
>> +++ b/drivers/infiniband/sw/rdmavt/qp.c
>> @@ -459,13 +459,16 @@ static unsigned rvt_free_all_qps(struct rvt_dev_info *rdi)
>>   */
>>  void rvt_qp_exit(struct rvt_dev_info *rdi)
>>  {
>> -	u32 qps_inuse = rvt_free_all_qps(rdi);
>> +	u32 qps_inuse = 0;
>> +
>> +	if (!rdi->qp_dev)
>> +		return;
>> +
>> +	qps_inuse = rvt_free_all_qps(rdi);
> 
> These lines are not needed.
> 
>>  
>>  	if (qps_inuse)
>>  		rvt_pr_err(rdi, "QP memory leak! %u still in use\n",
>>  			   qps_inuse);
>> -	if (!rdi->qp_dev)
>> -		return;
> 
> It is enough to delete these two lines. At this stage, rdi->qp_dev always
> exists as it was created in rvt_register_device().
> 

Agree with Leon here. qp_dev is created in rvt_register_device which will fail
if the qp dev allocation fails in rvt_driver_qp_init().

-Denny