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: <86d0225c-ca6e-c43e-835c-322770fd68c2@amd.com>
Date:   Fri, 18 Feb 2022 22:03:18 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH] drm/amdkfd: rework criu_restore_bos error handling
Content-Language: en-US
To:     Tom Rix <trix@redhat.com>, alexander.deucher@amd.com,
        christian.koenig@amd.com, Xinhui.Pan@amd.com, airlied@linux.ie,
        daniel@ffwll.ch, nathan@kernel.org, ndesaulniers@google.com,
        david.yatsin@amd.com, rajneesh.bhardwaj@amd.com
Cc:     amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, llvm@lists.linux.dev
References: <20220218173913.3376948-1-trix@redhat.com>
 <b98981f4-358b-ebdd-dade-4027c57a1a7e@amd.com>
 <4ca24c08-9014-3380-b8cc-dc0ed9a01a73@redhat.com>
From:   Felix Kuehling <felix.kuehling@amd.com>
In-Reply-To: <4ca24c08-9014-3380-b8cc-dc0ed9a01a73@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eGV6WnovMDQ2aDFaUjc5aS9NeWZwSWhSaU1TL3MrSFFSQTdSOEdoaGpFT2FP?=
 =?utf-8?B?SVkxaXJacWdVcW5mVDd0Z3NnS3IrbHROTE5LMENSa05ucEorcm4yY05Sb0Y0?=
 =?utf-8?B?SlE5VjZOdXJYTVpvU0x3OTdCaEpTeitoQ1pMTU10cHZ3Z0tEaXZ6NFkzeXky?=
 =?utf-8?B?cE0xZUU1M3dvcTh0VDJtVHRvNzRZL2V4emltajd1b1VmbkhSYmk3UzF1cWRw?=
 =?utf-8?B?UEovaDJDRTg4bU1YNUd2bDh6R2JXZmpFSzVjMEMvSEt2V3hGU21QNnZKZjJ6?=
 =?utf-8?B?cmptWWpka29pZSsySFJJeW96NTVNUXV1Vjg3UFN2WnVQTUhLSkhmTXZZYStF?=
 =?utf-8?B?VkhscUIrdTE1aUNEUkxUcE1lOUZ4a1dZRVZvbm82OHlzRUVoL0s4OG92Tm5r?=
 =?utf-8?B?eUk1VTJoMlI4aHhBN0c2YjMrVE81ZzZicitkc0FKNFE5SThma0pPNStZWjla?=
 =?utf-8?B?S2FieXlkeHQwL1BwczgxQURORWEvMkFoYTVkV1BOTDBsMDk1RXFSaGhZTVc1?=
 =?utf-8?B?ZGMzV0V5bGUrTi9LMWdGQWQ4NnRGN2c1QmliR2tDQit6K0tPUW4xNGIzOUw3?=
 =?utf-8?B?Rk1pby9EWGtqMDRXa0VkeFpvTmVjK1ZNalJlRC8rbDFEWXAvYkh1VDkyZ1Jn?=
 =?utf-8?B?QUJEZTBZV2k2eFRsWTdTR0F5dVJwR0drRHpoQSt5Q3E2dzdlclc5aDlINFpY?=
 =?utf-8?B?cDFPZkczMERscEpTSjdLQlp4K1prektMQWRFa3Yvbk0wQ3JFSkVUOXp3ODN2?=
 =?utf-8?B?T3lUSlBWNFhjalhyUWI0Z0ZzUitrbFp0bksvZmJiUXZjeHBQcXhGNmNVbWNk?=
 =?utf-8?B?TnhWVWd1RnU3SGFINTJKWU5FZHRHN0RmZ3JGS3JEWURBamw5cEJSMkVKRUJB?=
 =?utf-8?B?Y045RXgrbml0aWJ3RGtmcXdCb2ROQTI4S285VlZ5R3dzbDZCbnpHejZxaXha?=
 =?utf-8?B?MmQxMDVRdU1tMlFsZ3BkbURSTVlSNExXSUJHVEwyNGl6NlpoSEZZamd2Z3JU?=
 =?utf-8?B?TlA1Q0tGR2JwenQrRDFNWWM3UUs5YlFyd1FTT3M2WVVVaFQ3QnNIMjFJdkFL?=
 =?utf-8?B?YVRvZ2Q0MFdIYk9PSmJaNkpjMHNDdUZrZENGRjI5dDlLeHZORlZ1VWVROXRi?=
 =?utf-8?B?VGt6bnlQbEZRdUxGdHhnSEJiQjdmRFZ2WW5rUTNNZkREZXNjMCszd2JFczNz?=
 =?utf-8?B?WVZEQkJjS0Evc0t5VE5qMmpaYkZQZmk3QjVZTkpEOGpUUllKcEI1NTJzeGxx?=
 =?utf-8?B?K2FhZkVXaGRhdkJwTjJkbjRORjljWFRNc2o5SWxnazdxbEwrUGlYZW1Vc3p4?=
 =?utf-8?B?a3BWQXhhVGkxMlZKMFNuNDNncWlRZ3Jqa295SXJJZWZyU2xxemlQTVNwRVJC?=
 =?utf-8?B?aWhCZHpMQmduRVdSb2RLdUVMZWlYZXpWck9yd3RGanc5OVoyaWtnSk45bXN0?=
 =?utf-8?B?OXpHSU1WZFdseWFFZGVHTjVhMjJOSCtTTWpGUk1OclFXcGpxVXkzd0ZsR2Vm?=
 =?utf-8?B?ZXZQZzNELzR2bFJDekNYajRkR0RSSkxMRFpLS2xvUUY2RWFWZzBwRE51Mkxk?=
 =?utf-8?B?dnhmdDczcCtNZ1hZeCs0eHdnQmc0N25ZY2RUZUFGdnpRUnFnZm9VeEtBZEVq?=
 =?utf-8?B?WUx2RHNMKzVmYkdKdGVyWVpsckluVGg5NU5TNkJsNE9HNnI1MUU1aGdEbW91?=
 =?utf-8?B?WFJrVDJuUXcxbEM2Yi9LbnNrTFYycVdaelM4N0EvekpIQXppMEl3Nmh4ZlNT?=
 =?utf-8?B?VEV2TFk2T3RncVg5WkRTbFp2NU1mUWRmRmRobGY2b1o0ZFZXNFY3a0ZBYnJG?=
 =?utf-8?B?eUpoN1EzaDZBT3VUbGM1bXhTSURNMFhZVlhxWmhycEtHN0MwZkllVE5wRXJU?=
 =?utf-8?B?eWYzckRldThZdks4b2ZVcG0zdzAwWlFHWmVVbUxseWlGL0NMQVJyTVkydmp1?=
 =?utf-8?B?QW1RK3NOVDUzUkExc2ZHcTg3Mzh4VWEzN2NiRTl4empsSkUvOG0wSDE3K0lV?=
 =?utf-8?B?bThUZmFMclBzdWQwWGovaTBPU3Vwa09uZEVhWDlkNXBJMnpmZDg5bUVXVXRi?=
 =?utf-8?B?K29Qd0tkdUlOSnJjWXBvOEl1VTZJM1hNS1ZhcDRnL1hCWERNNWlnaFhVM2NP?=
 =?utf-8?B?N2owUDRmbXJXQmt0V085U2pGQzREdzBvRjV5UW42ZFp4Q2JZSHFZUDN1Smw0?=
 =?utf-8?Q?t95fXH0eVNTXRAeFmCyWkp4=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1f32ef96-4bd4-4b6a-9406-08d9f35462ae
X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5115.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Feb 2022 03:03:20.9296
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 4PFFOxTHVa0mcvy0SqqsfgcCTeVtOkThsWKH0dfoDDt3NXdwD1SqkIH3wv7XzX0tYHCGLTPnk7WizPidhjklng==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4887
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,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


Am 2022-02-18 um 21:34 schrieb Tom Rix:
>
> On 2/18/22 10:35 AM, Felix Kuehling wrote:
>> Am 2022-02-18 um 12:39 schrieb trix@redhat.com:
>>> From: Tom Rix <trix@redhat.com>
>>>
>>> Clang static analysis reports this problem
>>> kfd_chardev.c:2327:2: warning: 1st function call argument
>>>    is an uninitialized value
>>>    kvfree(bo_privs);
>>>    ^~~~~~~~~~~~~~~~
>>>
>>> If the copy_from_users(bo_buckets, ...) fails, there is a jump to
>>> the generic error handler at exit:.  The freeing of bo_privs and
>>> unwinding of the dmabuf_fd loop do not need to be done.
>>>
>>> Add some specific labels for the early failures.
>>> Reorder the frees to be the reverse of their allocs.
>>>
>>> Move the initialize of 'i' back to the loop.
>>> The problem with the early frees predates the loop
>>> unwinding problem.
>>
>> I think the existing error handling strategy in this function is 
>> fine. Having only one exit label avoids potential issues when using 
>> the wrong label. Freeing NULL pointers is not a problem. The loop 
>> becomes a noop if i==0 (this was fixed by you in a previous patch). 
>> The only real problem I see is that bo_privs is not initialized. So 
>> this should really be a one-line or maybe two-line fix:
>>
>>     struct kfd_criu_bo_bucket *bo_buckets = NULL;
>>     struct kfd_criu_bo_priv_data *bo_privs = NULL;
>
> This is the other way I considered to fix the problem. So it will work.

OK. I have already submitted this version to amd-staging-drm-next. Thank 
you for reporting the problem.

Regards,
   Felix


>
> Tom
>
>>
>> Regards,
>>   Felix
>>
>>
>>>
>>> Fixes: 73fa13b6a511 ("drm/amdkfd: CRIU Implement KFD restore ioctl")
>>> Signed-off-by: Tom Rix <trix@redhat.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++++------
>>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> index 965af2a08bc0..1d5f41ac3832 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> @@ -2102,7 +2102,7 @@ static int criu_restore_bos(struct kfd_process 
>>> *p,
>>>       const bool criu_resume = true;
>>>       bool flush_tlbs = false;
>>>       int ret = 0, j = 0;
>>> -    uint32_t i = 0;
>>> +    uint32_t i;
>>>         if (*priv_offset + (args->num_bos * sizeof(*bo_privs)) > 
>>> max_priv_data_size)
>>>           return -EINVAL;
>>> @@ -2119,13 +2119,13 @@ static int criu_restore_bos(struct 
>>> kfd_process *p,
>>>       if (ret) {
>>>           pr_err("Failed to copy BOs information from user\n");
>>>           ret = -EFAULT;
>>> -        goto exit;
>>> +        goto free_buckets;
>>>       }
>>>         bo_privs = kvmalloc_array(args->num_bos, sizeof(*bo_privs), 
>>> GFP_KERNEL);
>>>       if (!bo_privs) {
>>>           ret = -ENOMEM;
>>> -        goto exit;
>>> +        goto free_buckets;
>>>       }
>>>         ret = copy_from_user(bo_privs, (void __user 
>>> *)args->priv_data + *priv_offset,
>>> @@ -2133,12 +2133,12 @@ static int criu_restore_bos(struct 
>>> kfd_process *p,
>>>       if (ret) {
>>>           pr_err("Failed to copy BOs information from user\n");
>>>           ret = -EFAULT;
>>> -        goto exit;
>>> +        goto free_privs;
>>>       }
>>>       *priv_offset += args->num_bos * sizeof(*bo_privs);
>>>         /* Create and map new BOs */
>>> -    for (; i < args->num_bos; i++) {
>>> +    for (i = 0; i < args->num_bos; i++) {
>>>           struct kfd_criu_bo_bucket *bo_bucket;
>>>           struct kfd_criu_bo_priv_data *bo_priv;
>>>           struct kfd_dev *dev;
>>> @@ -2323,8 +2323,11 @@ static int criu_restore_bos(struct 
>>> kfd_process *p,
>>>           if (bo_buckets[i].alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_VRAM)
>>>               close_fd(bo_buckets[i].dmabuf_fd);
>>>       }
>>> -    kvfree(bo_buckets);
>>> +free_privs:
>>>       kvfree(bo_privs);
>>> +free_buckets:
>>> +    kvfree(bo_buckets);
>>> +
>>>       return ret;
>>>   }
>>
>