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@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'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@kernel-dev:/sys/kernel/config/vkms# tree
.
└── DEV
├── connectors
├── crtcs
├── enabled
├── encoders
└── planes
root@kernel-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
>
