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

["José Expósito" <jose.exposito89@gmail.com>]

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. [...]

I decided to follow this approach, i.e., allowing the user to delete the items.

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.

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
>