Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <a85f7751-8e60-d8f4-a281-4fb50389ae7e@amd.com>
Date:   Wed, 30 Mar 2022 12:29:53 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH 1/1] drm/amdkfd: Create file descriptor after client is
 added to smi_clients list
Content-Language: en-US
To:     Lee Jones <lee.jones@linaro.org>
Cc:     linux-kernel@vger.kernel.org,
        Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>, amd-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org
References: <20220330075115.426035-1-lee.jones@linaro.org>
From:   Felix Kuehling <felix.kuehling@amd.com>
In-Reply-To: <20220330075115.426035-1-lee.jones@linaro.org>
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?L083WW9xU2JEd0RoVEdpVytXWWNqak1hemthc1BVUitnUVJSWmZNaEwweTFm?=
 =?utf-8?B?L3FuNFdBSzIyZEVkRkxYVWhpRGxHTW0rb1piQWFRdllmTmhyZHdYL3kwY3R1?=
 =?utf-8?B?bVFGaGZjMVJqVVduUVh5RDEzWUJoS09BakwvNEVrVlpwZGNMZklmdHFhdWtM?=
 =?utf-8?B?VjZXMUNhYU93KzZjRGE3R0dyWUZtcGdvTEdQdktsdGhkZXplcUxuSGx0ajlP?=
 =?utf-8?B?dXlablJGSE9aU3lzelg0T24rQVJoM0NqenM5SDhGT0hMOHpuV1pWb2ZtNlBj?=
 =?utf-8?B?eXdUTVM2UHAzNHhRSTVhb2xob3ZYOWJRNWhLTWRRNTZlZHhVdVJESWNJYmox?=
 =?utf-8?B?eVZlVWJDajZoMnBxWkJsTjdyZ1ZOR1hNYmJBMW00aEQ2RjQ3SnVHS05EYXFT?=
 =?utf-8?B?eGJ1dUh6aGFYQ2VrMmlTL1llOVNLMVplZXowZjFWT2Vrc21GeWlJM2VXOENr?=
 =?utf-8?B?QnJrdDNjTnFFbkFyYmdCRnB0UE5BN2xDZkZ3U0dvM0dML3Ixd3E5aCtCSkxv?=
 =?utf-8?B?VUtSaXZqWFFSdjJGdWYzSmI0bEc4bFJ0eFhLK3FmREIvekxWaHc4Y0FJb0tt?=
 =?utf-8?B?UGtIV000QUF6L3AzMGhGRnBCRTBxRXBTa3E1RGIvbnp0TXlvb1RpME5SRkY5?=
 =?utf-8?B?Vk13S3doRm5VKzlyRjhXTWZuMDFhSDVnUWV2K3VVVXFEaUlsVm5QUk0yZUhC?=
 =?utf-8?B?RENMSDVKenoxQXZzTU9wT3B0Z3h2ZG52cWx4TjhIdWxkRjZEVnFZem5RSEw2?=
 =?utf-8?B?cS9qcW9odEVjQkk1SGE2YVlMWkVqZFNQN3pmSldyMVpIRGRKVW50SStPSSsy?=
 =?utf-8?B?NXFDTmNveElPZ0ZaQ2dWMmVQTTd2RU5FOHo0ckUvVXdJN3A4RnFxYW1LcFZq?=
 =?utf-8?B?WjdEQjcrc3ZhSmpHczd0WUs4YlBMbVpwVVNKMndEcWhlNUR1QzBldE1nM21n?=
 =?utf-8?B?Z0tvbVJDZnNqdi92Y3lUTnhSMjVGQ3lsSDlERys5S0hxbkM5Q0pZcTJhd3ly?=
 =?utf-8?B?UXZCWHlpTGcycUxPY0VnY0gyZTVxWFJEUE0rTFppc01MdWRsaVRSYjIvOTlU?=
 =?utf-8?B?dXgvMHRoMms0dVErWk1sSXB1UlBHQldLUUtON3lucUliUTFmSWFIcHowTVRj?=
 =?utf-8?B?ekhyWmRaY1E1NDFVZitDSGRjOEVjVjdTR3owY0dLZjZsVE82b2MxL1hNRWtu?=
 =?utf-8?B?dWQzbkdUaHluekt2VjVKZmxSVFV3aUFYelVMVld3Q3ZvTS9zcFhiRkhjeksw?=
 =?utf-8?B?OUdEQWdTRy9rM2E5MlE4NDZEZGI3QWo2azgraFZJcmlINTN3bFZiSU9ndTNu?=
 =?utf-8?B?bFVBVTA0dzlTUDBkWmhXUnJucW9oQi9CcW5yUEE4TXA4bUordXY0czV4enB4?=
 =?utf-8?B?VFJORFV6TEllbmwyUGhyN2ZsNGFlYjlyTnlsY05RM1hMZzYzMXVjMnNaaXNH?=
 =?utf-8?B?M0dtVjVCNGV5aGRoRllscHdWcXhFcXNKU3RtQU1GbGdyS3E0MDFEeE1VSk0w?=
 =?utf-8?B?REtxdWtvbTFmZ2Vpa3JKMGJ3T3ZVaExyODdKMExyWlhlNjZDZXJpaDk1a2Vw?=
 =?utf-8?B?WTBXUitWNzdBVjUxSmlodm9YaUo2c0EzeVFERDNJRTRxTUNSWTdjTHE4TXpZ?=
 =?utf-8?B?MWNKbk0yME9kSGZ0MURSa1lKWEZzUlRldFVxc1pWUCtnanRtZ2lQbDQ3RDFx?=
 =?utf-8?B?Z1UzVjgwWGJQRllGMWk2aXlpZnQyNElNZE5UZWJCU0h6eEhaUnZKVi80Zmpz?=
 =?utf-8?B?cGh3c0Y2NjJ5bXpMby9seE41cTltU1NIMGpleWdVYjhiTmtxQUkycUlpTzFi?=
 =?utf-8?B?VklPUmdMWEIxN3hyL0ZCbXNGL1o3c0lDOWFlZDgyand2TTdjcTJCNjM2ZnF3?=
 =?utf-8?B?M0J1dTNZbHlqdS9odTdPSmRHbE0vTEJXSDVCOWl5NGVhODJzazRPaEtELzkv?=
 =?utf-8?B?VEhBRUw5eHdzWWxaUWdlMkwxa0FxWFFXSmZDeGpWS2RlTTU5Sk4yUjJuU0w5?=
 =?utf-8?B?amFENGFmSUs1RE5HSUs5cDFZV2NvUnZ2Ymg4RHNiNW9QbmtJMExvU1FCQ1pi?=
 =?utf-8?B?aXI5eEdaelJUZlJNQThBYjlqb1lxSUpSR3lKY01OVXdHTkNTZnN0d3BqZ0Iv?=
 =?utf-8?B?emhxdkx0K3V1NUhpVzdvVXVaOVBVcG9BZTNFVk1WbHpzK1JZdWYyTkNQTHkr?=
 =?utf-8?B?U1Q5WWFUd1Z1eTBUd0tmaS8wU2xYcW9PVWhad2tGTEhMcjlrenNyQTAyYWxm?=
 =?utf-8?B?YVBmd1NXYi9YUWl6bHBwTWhFV2Y2dUxSS3FxK0M5VXphUkVrZnpNaG55WW5k?=
 =?utf-8?B?VVJoK2VUd0N5WndNYytERmtBTlJ6T045SHlZTnk5cGJjSTdzcE5XUT09?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d778f507-6592-4213-494e-08da126a8671
X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5115.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Mar 2022 16:29:57.7906
 (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: P7ML60bU1mEZEATUgopxrR3oDblySMXIYxxJk+LHvQqTM68kTJsKDaqaW7NgTOh2OOi4DVu6Q3IiyFIzecrqrQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1593
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=no 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-03-30 um 03:51 schrieb Lee Jones:
> This ensures userspace cannot prematurely clean-up the client before
> it is fully initialised which has been proven to cause issues in the
> past.
>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: amd-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> index e4beebb1c80a2..c5d5398d45cbf 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> @@ -247,15 +247,6 @@ int kfd_smi_event_open(struct kfd_dev *dev, uint32_t *fd)
>   		return ret;
>   	}
>   
> -	ret = anon_inode_getfd(kfd_smi_name, &kfd_smi_ev_fops, (void *)client,
> -			       O_RDWR);
> -	if (ret < 0) {
> -		kfifo_free(&client->fifo);
> -		kfree(client);
> -		return ret;
> -	}
> -	*fd = ret;
> -
>   	init_waitqueue_head(&client->wait_queue);
>   	spin_lock_init(&client->lock);
>   	client->events = 0;
> @@ -265,5 +256,14 @@ int kfd_smi_event_open(struct kfd_dev *dev, uint32_t *fd)
>   	list_add_rcu(&client->list, &dev->smi_clients);
>   	spin_unlock(&dev->smi_lock);
>   
> +	ret = anon_inode_getfd(kfd_smi_name, &kfd_smi_ev_fops, (void *)client,
> +			       O_RDWR);
> +	if (ret < 0) {

Thank you for the patch. This looks like the correct solution. But you 
also need to remove the client from the dev->smi_clients list here 
before kfree(client). With that fixed, the patch is

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>


> +		kfifo_free(&client->fifo);
> +		kfree(client);
> +		return ret;
> +	}
> +	*fd = ret;
> +
>   	return 0;
>   }