Message-ID: <Z8cUN8Q4L0VE-bVm@fedora>
Date: Tue, 4 Mar 2025 15:54:47 +0100
From: José Expósito <jose.exposito89@...il.com>
To: Louis Chauvet <louis.chauvet@...tlin.com>
Cc: hamohammed.sa@...il.com, simona@...ll.ch, melissa.srw@...il.com,
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
	tzimmermann@...e.de, airlied@...il.com,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 03/16] drm/vkms: Allow to configure multiple planes
 via configfs

Hi Louis,

On Mon, Mar 03, 2025 at 11:34:50AM +0100, Louis Chauvet wrote:
> 
> 
> 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@...tlin.com>
> > > > Co-developed-by: Louis Chauvet <louis.chauvet@...tlin.com>
> > > > Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>
> > > > Signed-off-by: José Expósito <jose.exposito89@...il.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'm trying to reproduce this issue, but those commands don't show any BUG
in dmesg. This is my Kasan .config:

    CONFIG_HAVE_ARCH_KASAN=y
    CONFIG_HAVE_ARCH_KASAN_VMALLOC=y
    CONFIG_CC_HAS_KASAN_GENERIC=y
    CONFIG_CC_HAS_KASAN_SW_TAGS=y
    CONFIG_CC_HAS_WORKING_NOSANITIZE_ADDRESS=y
    CONFIG_KASAN=y
    CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX=y
    CONFIG_KASAN_GENERIC=y
    # CONFIG_KASAN_OUTLINE is not set
    CONFIG_KASAN_INLINE=y
    CONFIG_KASAN_STACK=y
    CONFIG_KASAN_VMALLOC=y
    # CONFIG_KASAN_KUNIT_TEST is not set
    CONFIG_KASAN_EXTRA_INFO=y

I tryed to delete even more items:

    root@...nel-dev:/sys/kernel/config/vkms# tree
    .
    └── DEV
        ├── connectors
        ├── crtcs
        ├── enabled
        ├── encoders
        └── planes

    root@...nel-dev:/sys/kernel/config/vkms# cat DEV/enabled 
    1

And I still don't see any errors. Is it possible that we are running different
branches? Asking because of the failing IGT tests you reported. There seems to
be a difference in our code or setup that is creating these differences.

Best wishes,
Jose

> > > 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
> 

Powered by blists - more mailing lists