lkml.org 
[lkml]   [2022]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/1] drm/amdkfd: Protect the Client whilst it is being operated on
On Thu, 17 Mar 2022, philip yang wrote:

> On 2022-03-17 11:13 a.m., Lee Jones wrote:
>
> On Thu, 17 Mar 2022, Felix Kuehling wrote:
>
>
> Am 2022-03-17 um 11:00 schrieb Lee Jones:
>
> Good afternoon Felix,
>
> Thanks for your review.
>
>
> Am 2022-03-17 um 09:16 schrieb Lee Jones:
>
> Presently the Client can be freed whilst still in use.
>
> Use the already provided lock to prevent this.
>
> Cc: Felix Kuehling [1]<Felix.Kuehling@amd.com>
> Cc: Alex Deucher [2]<alexander.deucher@amd.com>
> Cc: "Christian König" [3]<christian.koenig@amd.com>
> Cc: "Pan, Xinhui" [4]<Xinhui.Pan@amd.com>
> Cc: David Airlie [5]<airlied@linux.ie>
> Cc: Daniel Vetter [6]<daniel@ffwll.ch>
> Cc: [7]amd-gfx@lists.freedesktop.org
> Cc: [8]dri-devel@lists.freedesktop.org
> Signed-off-by: Lee Jones [9]<lee.jones@linaro.org>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c b/drivers/gpu/drm/amd/a
> mdkfd/kfd_smi_events.c
> index e4beebb1c80a2..3b9ac1e87231f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> @@ -145,8 +145,11 @@ static int kfd_smi_ev_release(struct inode *inode, struct f
> ile *filep)
> spin_unlock(&dev->smi_lock);
> synchronize_rcu();
> +
> + spin_lock(&client->lock);
> kfifo_free(&client->fifo);
> kfree(client);
> + spin_unlock(&client->lock);
>
> The spin_unlock is after the spinlock data structure has been freed.
>
> Good point.
>
> If we go forward with this approach the unlock should perhaps be moved
> to just before the kfree().
>
>
> There
> should be no concurrent users here, since we are freeing the data structure.
> If there still are concurrent users at this point, they will crash anyway.
> So the locking is unnecessary.
>
> The users may well crash, as does the kernel unfortunately.
>
> We only get to kfd_smi_ev_release when the file descriptor is closed. User
> mode has no way to use the client any more at this point. This function also
> removes the client from the dev->smi_cllients list. So no more events will
> be added to the client. Therefore it is safe to free the client.
>
> If any of the above were not true, it would not be safe to kfree(client).
>
> But if it is safe to kfree(client), then there is no need for the locking.
>
> I'm not keen to go into too much detail until it's been patched.
>
> However, there is a way to free the client while it is still in use.
>
> Remember we are multi-threaded.
>
> files_struct->count refcount is used to handle this race, as
> vfs_read/vfs_write takes file refcount and fput calls release only if
> refcount is 1, to guarantee that read/write from user space is finished
> here.
>
> Another race is driver add_event_to_kfifo while closing the handler. We
> use rcu_read_lock in add_event_to_kfifo, and kfd_smi_ev_release calls
> synchronize_rcu to wait for all rcu_read done. So it is safe to call
> kfifo_free(&client->fifo) and kfree(client).

Philip, please reach out to Felix.

We have discussed this in more detail off-line.

--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

\
 
 \ /
  Last update: 2022-03-17 17:31    [W:0.068 / U:0.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site