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: <b98981f4-358b-ebdd-dade-4027c57a1a7e@amd.com>
Date:   Fri, 18 Feb 2022 13:35:41 -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:     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>
From:   Felix Kuehling <felix.kuehling@amd.com>
In-Reply-To: <20220218173913.3376948-1-trix@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?OXdpeUJ1eWE3SmlFMDkzVnVZZUtxRFA2bzRFTjFkaW9vejFWWERIb1FGaUQ0?=
 =?utf-8?B?eVk4ZGN6K2FlWGJIUStpeDJTektoWjVxWTFiS1ZyZnhMNHZpMjM0bUc1NjJM?=
 =?utf-8?B?aDdaUXRtOTVJRThmK2Y0cUcycmZhZWpqcjBJMzAxWmd2YW05ZllwdjV4NDRY?=
 =?utf-8?B?WWgrS3JGM094Si9rT0toZjMyQmNhRWJnaHJjekgxbm82dXFGQzVKandsVWRm?=
 =?utf-8?B?Q1dDajBsUmxQMTFGV2xXOVpQM2E3bStRSGU5dEhrWmRmL3h4VzdTTExjdDNJ?=
 =?utf-8?B?QVEvTGVWSTN3ZWt4aXF5SWE2c1k1S1BiWTlPQTBidjdUdlBkTjlnSEpIT0RZ?=
 =?utf-8?B?U0drNjlJSVZKdUs2OTREcVZZOWRyRnJDZ0x6TnZ6NjhMWUFjRVBIRVkyeStF?=
 =?utf-8?B?aERKMGlQZ3lSSkNHa1AvaWdHSXExejVxUkloaGZjeFJoS05FMVI1eENtSXVM?=
 =?utf-8?B?cFlDRGtCb3RlSXluaXRhQ2ZiUE9WUWZWdEEzaWt0LzFyRFRjVTdkdHRpTGp2?=
 =?utf-8?B?WExmQ0l6OWRUMk5mYXJqWmI2Ykp4Y1htazc1aGRVMkx0YWdKTzZvVTlieTNH?=
 =?utf-8?B?Q01zbmJJcDZuREFlZ2I0WXN2Z09tVDZWMmo4b3ErYUVzd2xGOGt3NjN6MHJB?=
 =?utf-8?B?SC83YmJXZHRJS0kwS1ZHV2d2V2ZtU2FiM2NycHJiYkVqenhtSUVpWEswVTdt?=
 =?utf-8?B?VWtScUpqMStiTWxmQmh6RHNrbmtGTGhtQlA4UFU0dkJNVGFDYXk2cXpxcHdv?=
 =?utf-8?B?RlEwQ09kOXBhM3dINkY2eGlXUXloUmVvbVVQTlJ1aEZlRHpIa1FTa21hRWh5?=
 =?utf-8?B?bVdVemVDUm5VSzA2TjFlakRRY25LL1ZtTkZLNk96SXpVZnhOOUwwdXVad1Nu?=
 =?utf-8?B?dHVMSHBlL2xZUUZ3SHk5VVM1ZmhLd0tTZERHUk8zOGRRaEZhUFhyaXVYUTRs?=
 =?utf-8?B?S2o5aTduMUJndkZiMlpIWVZYRm1ZWGpWaFBINTdMNEZUa1N2V2xFVU0zU3Iy?=
 =?utf-8?B?aHlyYzVVVExwSUNjb1l1alN4aDVWZituYmlUdVpsTjBnb1p0T2pxbFlXcGpT?=
 =?utf-8?B?SUtUMjRGN2ZHVVU3YU4xVmRDYlRTN2Vua2t0OHYydDhlVnhVMW1KUGFnV2JQ?=
 =?utf-8?B?TlVGSkpkd0NUc0RSMGhDckYyOE1lbHJkS1Z2Zk1HVnBJVHN6TTRpUnN4VlF3?=
 =?utf-8?B?aFBPdXJYM1owUjhJS3dWRjRSTEtmYktWYkowQng0aldlYWNjVUUyeVIxQWRt?=
 =?utf-8?B?YkJENUNFQUg1cWw1TnhSNWRMenUzdkpHY0pLdDJYeTIzOUJtK3dCczBIc3Y3?=
 =?utf-8?B?aThidEhhWmF0eVllU0haVGpEUVZjK2RtNS8xbzluK0RmMm84RVBDRGVHczI1?=
 =?utf-8?B?T21oekFKbTNUUllpandhUC91dk1PSEJaYVhaL0txOUJDeFRub1NBZEFHb3JW?=
 =?utf-8?B?VXl4L2hVbEFvUXY0THVKekZkbjhWNDZ0UTFEblR2dE9KYWxNNi9sT2NiRE52?=
 =?utf-8?B?UDZ3UFRNVWJSeUpCdU1uc1pET25ldWdwVkczK2t6d3RGNDBMSHQ0TkZ3RC8z?=
 =?utf-8?B?dWlTRitmTVRCdG5uYVV2dFQ3NlpaWU5nNCt0MXVxWGdjazlKcjRaNFp6Wlh3?=
 =?utf-8?B?MkFyMHZIdUxLeHNmaDN5cWZjQXY3SHB2bllLRzhleWgzWFhkdWFkYlZDWGpF?=
 =?utf-8?B?N3hncy9TWXphK3QvcEhOSEg0b1pCczVXaVBYbHc3ZmErbWZjaXQ3NjVZRm5N?=
 =?utf-8?B?NVlrRlp6VUR6WkdlZnFzWHlKYmhsTWI1MlNsdTgxVjh1REdVYzdUQndxdjVv?=
 =?utf-8?B?QW9hamdYOHp0dEozbkllNDBuNDJmUDRqbUdZSG1oMjV5WnFQeTNCR2lHeWJY?=
 =?utf-8?B?YXVWZ2tnNzAzUUtmQzY2S2pENVovZnZ6NHRsSE4wYXU2S2lBb29lTUdOUHhw?=
 =?utf-8?B?Nk9WVmZmMTM1TFdzSzV5N3ZBemF6cVMxVFlYSGtwczlPbEdTekJna2xsUnli?=
 =?utf-8?B?aFBPaXh5aVUyMWZ0L3FnOUNLckVHTTRDOXp4MWlCVmFLQ0Jib05vY0Yxd0dl?=
 =?utf-8?B?OHZKUGNxQktmMDBlOHk3Rlpnb2R2dzBNeTJMc0FuZVV6Y3hJZll0UDgzUWwz?=
 =?utf-8?B?MVRYR1d2WXZ3QXErTzFrMFlkbzVwMU1YUE5ZNWdjYnF6RXNwSHNLWUIvMHIw?=
 =?utf-8?Q?0oZ2QHK1VSjz+0eZm2S9qkI=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8a303859-c004-4d80-29ad-08d9f30d78e2
X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5115.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Feb 2022 18:35:43.8941
 (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: D8sDWxjsQFlDm+LHZDMBgpSouYK+Iwylin19GYMD0qaxhedege8LOFni9jMgnxEw29skB1PdyAMrLlkSjf1BPA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4034
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 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;

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;
>   }
>