<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 2022-03-17 11:13 a.m., Lee Jones wrote: </div> <blockquote type="cite" cite="mid:YjNQA80wkWpy+AmA@google.com"> <pre class="moz-quote-pre" wrap="">On Thu, 17 Mar 2022, Felix Kuehling wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> Am 2022-03-17 um 11:00 schrieb Lee Jones: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">Good afternoon Felix, Thanks for your review. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">Am 2022-03-17 um 09:16 schrieb Lee Jones: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">Presently the Client can be freed whilst still in use. Use the already provided lock to prevent this. Cc: Felix Kuehling <a class="moz-txt-link-rfc2396E" href="mailto:Felix.Kuehling@amd.com"><Felix.Kuehling@amd.com></a> Cc: Alex Deucher <a class="moz-txt-link-rfc2396E" href="mailto:alexander.deucher@amd.com"><alexander.deucher@amd.com></a> Cc: "Christian König" <a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com"><christian.koenig@amd.com></a> Cc: "Pan, Xinhui" <a class="moz-txt-link-rfc2396E" href="mailto:Xinhui.Pan@amd.com"><Xinhui.Pan@amd.com></a> Cc: David Airlie <a class="moz-txt-link-rfc2396E" href="mailto:airlied@linux.ie"><airlied@linux.ie></a> Cc: Daniel Vetter <a class="moz-txt-link-rfc2396E" href="mailto:daniel@ffwll.ch"><daniel@ffwll.ch></a> Cc: <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> Cc: <a class="moz-txt-link-abbreviated" href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> Signed-off-by: Lee Jones <a class="moz-txt-link-rfc2396E" href="mailto:lee.jones@linaro.org"><lee.jones@linaro.org></a> --- 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/amdkfd/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 file *filep) spin_unlock(&dev->smi_lock); synchronize_rcu(); + + spin_lock(&client->lock); kfifo_free(&client->fifo); kfree(client); + spin_unlock(&client->lock); </pre> </blockquote> <pre class="moz-quote-pre" wrap="">The spin_unlock is after the spinlock data structure has been freed. </pre> </blockquote> <pre class="moz-quote-pre" wrap="">Good point. If we go forward with this approach the unlock should perhaps be moved to just before the kfree(). </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">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. </pre> </blockquote> <pre class="moz-quote-pre" wrap="">The users may well crash, as does the kernel unfortunately. </pre> </blockquote> <pre class="moz-quote-pre" wrap="">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. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> 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. </pre> </blockquote> 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). Regards, Philip <blockquote type="cite" cite="mid:YjNQA80wkWpy+AmA@google.com"> <pre class="moz-quote-pre" wrap=""> </pre> </blockquote> </body> </html>