Re: [PATCH v2 03/16] drm/vkms: Allow to configure multiple planes via configfs

[Louis Chauvet <louis.chauvet@bootlin.com>]

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@bootlin.com>
>>> Co-developed-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
>>> [...]
>>> 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