Good afternoon Felix,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.
Thanks for your review.
Am 2022-03-17 um 09:16 schrieb Lee Jones:Good point.
Presently the Client can be freed whilst still in use.The spin_unlock is after the spinlock data structure has been freed.
Use the already provided lock to prevent this.
Cc: Felix Kuehling <Felix.Kuehling@xxxxxxx>
Cc: Alex Deucher <alexander.deucher@xxxxxxx>
Cc: "Christian König" <christian.koenig@xxxxxxx>
Cc: "Pan, Xinhui" <Xinhui.Pan@xxxxxxx>
Cc: David Airlie <airlied@xxxxxxxx>
Cc: Daniel Vetter <daniel@xxxxxxxx>
Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Lee Jones <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/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);
If we go forward with this approach the unlock should perhaps be moved
to just before the kfree().
ThereThe users may well crash, as does the kernel unfortunately.
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.
Bingo. Well done. :)return 0;The client was just allocated, and it wasn't added to the client list or
}
@@ -247,11 +250,13 @@ int kfd_smi_event_open(struct kfd_dev *dev, uint32_t *fd)
return ret;
}
+ spin_lock(&client->lock);
given to user mode yet. So there can be no concurrent users at this point.
The locking is unnecessary.
There could be potential issues if someone uses the file descriptor by dumb
luck before this function returns. So maybe we need to move the
anon_inode_getfd to the end of the function (just before list_add_rcu) so
that we only create the file descriptor after the client structure is fully
initialized.
I can move the function as suggested if that is the best route forward?