Re: [PATCH 1/1] drm/amdkfd: Protect the Client whilst it is being operated on

[Lee Jones]

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@xxxxxxx>
> Cc: Alex Deucher [2]<alexander.deucher@xxxxxxx>
> Cc: "Christian König" [3]<christian.koenig@xxxxxxx>
> Cc: "Pan, Xinhui" [4]<Xinhui.Pan@xxxxxxx>
> Cc: David Airlie [5]<airlied@xxxxxxxx>
> Cc: Daniel Vetter [6]<daniel@xxxxxxxx>
> Cc: [7]amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: [8]dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Lee Jones [9]<lee.jones@xxxxxxxxxx>
> ---
>    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