Le 03/03/2025 à 09:50, José Expósito a écrit :
Hi Louis,
On Fri, Feb 28, 2025 at 03:43:25PM +0100, Louis Chauvet wrote:
Le 25/02/2025 à 18:59, José Expósito a écrit :
Create a default subgroup at /config/vkms/planes to allow to create as
many planes as required.
Reviewed-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
Co-developed-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx>
[...]
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 92512d52ddae..4f9d3341e6c0 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
[...]
+static void plane_release(struct config_item *item)
+{
+ struct vkms_configfs_plane *plane;
+ struct mutex *lock;
+
+ plane = plane_item_to_vkms_configfs_plane(item);
+ lock = &plane->dev->lock;
+
+ guard(mutex)(lock);
+ vkms_config_destroy_plane(plane->config);
+ kfree(plane);
+}
I just found a flaw in our work: there is currently no way to forbid the
deletion of item/symlinks...
If you do:
modprobe vkms
cd /sys/kernel/config/vkms/
mkdir DEV
mkdir DEV/connectors/CON
mkdir DEV/planes/PLA
mkdir DEV/crtcs/CRT
mkdir DEV/encoders/ENC
ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
echo 1 > DEV/planes/PLA/type
tree
echo 1 > DEV/enabled
modetest -M vkms
=> everything fine
rm DEV/connectors/CON/possible_encoders/ENC
rmdir DEV/connectors/CON
modetest -M vkms
=> BUG: KASAN: slab-use-after-free
I see two solutions:
- we don't care and keep as is: if the device is enabled, and you delete
link/groups, it is your fault. As shown above: it can crash the kernel, so
it is a no-go.
I was aware of this limitation and, since the configfs is clear about
deleting items: [1]
Important:
drop_item() is void, and as such cannot fail. When rmdir(2) is called,
configfs WILL remove the item from the filesystem tree (assuming that
it has no children to keep it busy).
The subsystem is responsible for responding to this. [...]
Thanks for pointing out. I read it and I think you're right, they
clearly want the user space to be able to delete any item at any time.
I decided to follow this approach, i.e., allowing the user to delete the items.
I think a simple fix would be to have something like that:
int device_enabled_store(...) {
if enabled == True:
for item in configfs_items:
configfs_get_item(item);
vkms_device_enable(...)
else:
vkms_device_disable(...)
for item in configfs_items:
configfs_put_item(item)
}
void device_release(...) {
if enable == True:
vkms_device_disable(...)
for item in configfs_items:
configfs_put_item(item)
}
This way:
- no change in VKMS code
- no ConfigFS change
- we never have use-after-free (at least in my head)
- we never "lose" a DRM device
I did not test it, there is maybe some issue in this implementation (the
"for item in configfs_items" may be a bit complex to write for example).
However, that use-after-free is a bug I need to fix. I was wondering how I didn't
catch it with IGT... Turns out, I didn't enable Kasan in my QEMU .config (ops!).
Do you agree on folowing this solution? If so, I'll send v3 fixing the memory
issues.
Indeed yes! If possible, I would like to avoid "complex" refcount/mutex
in vkms_config structure, but if my proposition does not work, feel free
to add whatever you think relevant!
Thanks a lot,
Louis Chauvet
Best wishes,
Jose
[1] https://docs.kernel.org/filesystems/configfs.html
- we care and we don't want to touch configfs: we need to implement a kind
of refcount for all vkms_config elements. Issue: non-trivial work, may allow
memory leaks/use after free...
- we care and we want to touch configfs: see my two patches (they apply on
the v1 of this series). This solution allows adding a check before removing
configfs item/group/link. I found it cleaner and way easier to understand.
What do you think about my proposition? Do you have another idea?
+static struct configfs_item_operations plane_item_operations = {
+ .release = &plane_release,
+};
+
+static const struct config_item_type plane_item_type = {
+ .ct_item_ops = &plane_item_operations,
+ .ct_owner = THIS_MODULE,
+};
+
+static struct config_group *make_plane_group(struct config_group *group,
+ const char *name)
+{
+ struct vkms_configfs_device *dev;
+ struct vkms_configfs_plane *plane;
+
+ dev = child_group_to_vkms_configfs_device(group);
+
+ guard(mutex)(&dev->lock);
+
+ if (dev->enabled)
+ return ERR_PTR(-EBUSY);
+
+ plane = kzalloc(sizeof(*plane), GFP_KERNEL);
+ if (!plane)
+ return ERR_PTR(-ENOMEM);
+
+ plane->dev = dev;
+
+ plane->config = vkms_config_create_plane(dev->config);
+ if (IS_ERR(plane->config)) {
+ kfree(plane);
+ return ERR_CAST(plane->config);
+ }
+
+ config_group_init_type_name(&plane->group, name, &plane_item_type);
+
+ return &plane->group;
+}
+
+static struct configfs_group_operations planes_group_operations = {
+ .make_group = &make_plane_group,
+};
+
+static const struct config_item_type plane_group_type = {
+ .ct_group_ops = &planes_group_operations,
+ .ct_owner = THIS_MODULE,
+};
+
static ssize_t device_enabled_show(struct config_item *item, char *page)
{
struct vkms_configfs_device *dev;
@@ -125,6 +208,10 @@ static struct config_group *make_device_group(struct config_group *group,
config_group_init_type_name(&dev->group, name, &device_item_type);
mutex_init(&dev->lock);
+ config_group_init_type_name(&dev->planes_group, "planes",
+ &plane_group_type);
+ configfs_add_default_group(&dev->planes_group, &dev->group);
+
return &dev->group;
}
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
