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>
---
Documentation/gpu/vkms.rst | 16 ++++-
drivers/gpu/drm/vkms/vkms_configfs.c | 87 ++++++++++++++++++++++++++++
2 files changed, 102 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 423bdf86b5b1..bf23d0da33fe 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -71,6 +71,19 @@ By default, the instance is disabled::
cat /config/vkms/my-vkms/enabled
0
+And directories are created for each configurable item of the display pipeline::
+
+ tree /config/vkms/my-vkms
+ ├── enabled
+ └── planes
+
+To add items to the display pipeline, create one or more directories under the
+available paths.
+
+Start by creating one or more planes::
+
+ sudo mkdir /config/vkms/my-vkms/planes/plane0
+
Once you are done configuring the VKMS instance, enable it::
echo "1" | sudo tee /config/vkms/my-vkms/enabled
@@ -79,8 +92,9 @@ Finally, you can remove the VKMS instance disabling it::
echo "0" | sudo tee /config/vkms/my-vkms/enabled
-And removing the top level directory::
+And removing the top level directory and its subdirectories::
+ sudo rmdir /config/vkms/my-vkms/planes/*
sudo rmdir /config/vkms/my-vkms
Testing With IGT
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
@@ -16,6 +16,7 @@ static bool is_configfs_registered;
*
* @group: Top level configuration group that represents a VKMS device.
* Initialized when a new directory is created under "/config/vkms/"
+ * @planes_group: Default subgroup of @group at "/config/vkms/planes"
* @lock: Lock used to project concurrent access to the configuration attributes
* @config: Protected by @lock. Configuration of the VKMS device
* @enabled: Protected by @lock. The device is created or destroyed when this
@@ -23,16 +24,98 @@ static bool is_configfs_registered;
*/
struct vkms_configfs_device {
struct config_group group;
+ struct config_group planes_group;
struct mutex lock;
struct vkms_config *config;
bool enabled;
};
+/**
+ * struct vkms_configfs_plane - Configfs representation of a plane
+ *
+ * @group: Top level configuration group that represents a plane.
+ * Initialized when a new directory is created under "/config/vkms/planes"
+ * @dev: The vkms_configfs_device this plane belongs to
+ * @config: Configuration of the VKMS plane
+ */
+struct vkms_configfs_plane {
+ struct config_group group;
+ struct vkms_configfs_device *dev;
+ struct vkms_config_plane *config;
+};
+
#define device_item_to_vkms_configfs_device(item) \
container_of(to_config_group((item)), struct vkms_configfs_device, \
group)
+#define child_group_to_vkms_configfs_device(group) \
+ device_item_to_vkms_configfs_device((&(group)->cg_item)->ci_parent)
+
+#define plane_item_to_vkms_configfs_plane(item) \
+ container_of(to_config_group((item)), struct vkms_configfs_plane, group)
+
+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);
+}
+
+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;
}
--
2.48.1
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>
> ---
> Documentation/gpu/vkms.rst | 16 ++++-
> drivers/gpu/drm/vkms/vkms_configfs.c | 87 ++++++++++++++++++++++++++++
> 2 files changed, 102 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> index 423bdf86b5b1..bf23d0da33fe 100644
> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -71,6 +71,19 @@ By default, the instance is disabled::
> cat /config/vkms/my-vkms/enabled
> 0
>
> +And directories are created for each configurable item of the display pipeline::
> +
> + tree /config/vkms/my-vkms
> + ├── enabled
> + └── planes
> +
> +To add items to the display pipeline, create one or more directories under the
> +available paths.
> +
> +Start by creating one or more planes::
> +
> + sudo mkdir /config/vkms/my-vkms/planes/plane0
> +
> Once you are done configuring the VKMS instance, enable it::
>
> echo "1" | sudo tee /config/vkms/my-vkms/enabled
> @@ -79,8 +92,9 @@ Finally, you can remove the VKMS instance disabling it::
>
> echo "0" | sudo tee /config/vkms/my-vkms/enabled
>
> -And removing the top level directory::
> +And removing the top level directory and its subdirectories::
>
> + sudo rmdir /config/vkms/my-vkms/planes/*
> sudo rmdir /config/vkms/my-vkms
>
> Testing With IGT
> 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
> @@ -16,6 +16,7 @@ static bool is_configfs_registered;
> *
> * @group: Top level configuration group that represents a VKMS device.
> * Initialized when a new directory is created under "/config/vkms/"
> + * @planes_group: Default subgroup of @group at "/config/vkms/planes"
> * @lock: Lock used to project concurrent access to the configuration attributes
> * @config: Protected by @lock. Configuration of the VKMS device
> * @enabled: Protected by @lock. The device is created or destroyed when this
> @@ -23,16 +24,98 @@ static bool is_configfs_registered;
> */
> struct vkms_configfs_device {
> struct config_group group;
> + struct config_group planes_group;
>
> struct mutex lock;
> struct vkms_config *config;
> bool enabled;
> };
>
> +/**
> + * struct vkms_configfs_plane - Configfs representation of a plane
> + *
> + * @group: Top level configuration group that represents a plane.
> + * Initialized when a new directory is created under "/config/vkms/planes"
> + * @dev: The vkms_configfs_device this plane belongs to
> + * @config: Configuration of the VKMS plane
> + */
> +struct vkms_configfs_plane {
> + struct config_group group;
> + struct vkms_configfs_device *dev;
> + struct vkms_config_plane *config;
> +};
> +
> #define device_item_to_vkms_configfs_device(item) \
> container_of(to_config_group((item)), struct vkms_configfs_device, \
> group)
>
> +#define child_group_to_vkms_configfs_device(group) \
> + device_item_to_vkms_configfs_device((&(group)->cg_item)->ci_parent)
> +
> +#define plane_item_to_vkms_configfs_plane(item) \
> + container_of(to_config_group((item)), struct vkms_configfs_plane, group)
> +
> +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.
- 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
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
>
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
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@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'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
>
Le 04/03/2025 à 15:54, José Expósito a écrit :
> 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@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'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.
I just re-applied your last vkms-config version and this series on top
of drm-misc-next. See [1] for the exact commits.
Argg sorry, I just noticed something: you need to disable the default
vkms device (I had this option in my kernel command line...), otherwise
modetest only use the first vkms gpu...
I will check again the igt tests, but I don't think this is the same
issue (it should not use the default device to test)
So, with [1] and the defconfig below, I have this:
1 modprobe vkms create_default_dev=0
2 cd /sys/kernel/config/vkms/
3 mkdir DEV
4 mkdir DEV/connectors/CON
5 mkdir DEV/planes/PLA
6 mkdir DEV/crtcs/CRT
7 mkdir DEV/encoders/ENC
8 ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
9 ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
10 ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
11 echo 1 > DEV/planes/PLA/type
12 tree
13 echo 1 > DEV/enabled
14 modetest -M vkms
15 rm DEV/connectors/CON/possible_encoders/ENC
16 rmdir DEV/connectors/CON
17 modetest -M vkms
KASAN: slab-use-after-free
[1]:https://github.com/Fomys/linux/tree/20250225175936.7223-1-jose.exposito89%40gmail.com
===== defconfig =====
CONFIG_SYSVIPC=y
CONFIG_CGROUPS=y
CONFIG_KALLSYMS_ALL=y
CONFIG_SMP=y
CONFIG_HYPERVISOR_GUEST=y
CONFIG_PARAVIRT=y
# CONFIG_VIRTUALIZATION is not set
CONFIG_JUMP_LABEL=y
CONFIG_MODULES=y
CONFIG_MODULE_UNLOAD=y
CONFIG_NET=y
CONFIG_PACKET=y
# CONFIG_WIRELESS is not set
CONFIG_NET_9P=y
CONFIG_NET_9P_VIRTIO=y
CONFIG_PCI=y
CONFIG_DEVTMPFS=y
CONFIG_DEVTMPFS_MOUNT=y
CONFIG_VIRTIO_BLK=y
# CONFIG_INTEL_MEI is not set
CONFIG_NETDEVICES=y
CONFIG_VIRTIO_NET=y
# CONFIG_ETHERNET is not set
# CONFIG_WLAN is not set
CONFIG_INPUT_EVDEV=y
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_VIRTIO_CONSOLE=y
CONFIG_HW_RANDOM_VIRTIO=m
CONFIG_PTP_1588_CLOCK=y
# CONFIG_HWMON is not set
CONFIG_THERMAL_GOV_USER_SPACE=y
CONFIG_DRM=y
CONFIG_DRM_KUNIT_TEST=m
CONFIG_DRM_VKMS=m
CONFIG_DRM_VKMS_KUNIT_TEST=m
# CONFIG_USB_SUPPORT is not set
CONFIG_VIRTIO_PCI=y
CONFIG_VIRTIO_BALLOON=y
CONFIG_VIRTIO_INPUT=y
CONFIG_VIRTIO_MMIO=y
CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES=y
# CONFIG_SURFACE_PLATFORMS is not set
CONFIG_EXT4_FS=y
CONFIG_FUSE_FS=y
CONFIG_VIRTIO_FS=y
CONFIG_OVERLAY_FS=y
CONFIG_TMPFS=y
CONFIG_TMPFS_POSIX_ACL=y
CONFIG_CONFIGFS_FS=y
CONFIG_9P_FS=y
CONFIG_CRYPTO=y
CONFIG_CRYPTO_CRC32C=y
CONFIG_DYNAMIC_DEBUG=y
CONFIG_DEBUG_KERNEL=y
CONFIG_DEBUG_INFO_DWARF5=y
CONFIG_MAGIC_SYSRQ=y
CONFIG_DEBUG_FS=y
CONFIG_DEBUG_PAGEALLOC=y
CONFIG_DEBUG_PAGEALLOC_ENABLE_DEFAULT=y
CONFIG_PAGE_POISONING=y
CONFIG_DEBUG_OBJECTS=y
CONFIG_DEBUG_OBJECTS_RCU_HEAD=y
CONFIG_SCHED_STACK_END_CHECK=y
CONFIG_KASAN=y
CONFIG_KASAN_VMALLOC=y
CONFIG_KASAN_EXTRA_INFO=y
CONFIG_KFENCE=y
# CONFIG_FTRACE is not set
CONFIG_UNWINDER_FRAME_POINTER=y
CONFIG_KUNIT=y
CONFIG_TEST_DYNAMIC_DEBUG=m
On Tue, Mar 04, 2025 at 04:35:50PM +0100, Louis Chauvet wrote:
>
>
> Le 04/03/2025 à 15:54, José Expósito a écrit :
> > 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@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'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.
>
> I just re-applied your last vkms-config version and this series on top of
> drm-misc-next. See [1] for the exact commits.
>
> Argg sorry, I just noticed something: you need to disable the default vkms
> device (I had this option in my kernel command line...), otherwise modetest
> only use the first vkms gpu...
>
> I will check again the igt tests, but I don't think this is the same issue
> (it should not use the default device to test)
>
> So, with [1] and the defconfig below, I have this:
>
>
> 1 modprobe vkms create_default_dev=0
> 2 cd /sys/kernel/config/vkms/
> 3 mkdir DEV
> 4 mkdir DEV/connectors/CON
> 5 mkdir DEV/planes/PLA
> 6 mkdir DEV/crtcs/CRT
> 7 mkdir DEV/encoders/ENC
> 8 ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
> 9 ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
> 10 ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
> 11 echo 1 > DEV/planes/PLA/type
> 12 tree
> 13 echo 1 > DEV/enabled
> 14 modetest -M vkms
> 15 rm DEV/connectors/CON/possible_encoders/ENC
> 16 rmdir DEV/connectors/CON
> 17 modetest -M vkms
> KASAN: slab-use-after-free
>
>
> [1]:https://github.com/Fomys/linux/tree/20250225175936.7223-1-jose.exposito89%40gmail.com
Aha! Are you testing without a desktop environment running?
$ sudo systemctl isolate multi-user.target
Running that (^) command before yours gives me this use after free:
BUG: KASAN: slab-use-after-free in vkms_connector_detect+0x61/0x70 [vkms]
Is the same one you are seeing?
Looking at the connector_release() function in vkms_configfs.c, I see
that I'm freeing the configuration:
vkms_config_destroy_connector(connector->config);
And I don't think there is a reason to do it. vkms_config_destroy() in
device_release() will free everything once we are done.
I need to do more testing, but the solution might be that simple.
Good catch, I didn't test like that and I overlooked this issue :D
Jose
>
>
> ===== defconfig =====
>
> CONFIG_SYSVIPC=y
> CONFIG_CGROUPS=y
> CONFIG_KALLSYMS_ALL=y
> CONFIG_SMP=y
> CONFIG_HYPERVISOR_GUEST=y
> CONFIG_PARAVIRT=y
> # CONFIG_VIRTUALIZATION is not set
> CONFIG_JUMP_LABEL=y
> CONFIG_MODULES=y
> CONFIG_MODULE_UNLOAD=y
> CONFIG_NET=y
> CONFIG_PACKET=y
> # CONFIG_WIRELESS is not set
> CONFIG_NET_9P=y
> CONFIG_NET_9P_VIRTIO=y
> CONFIG_PCI=y
> CONFIG_DEVTMPFS=y
> CONFIG_DEVTMPFS_MOUNT=y
> CONFIG_VIRTIO_BLK=y
> # CONFIG_INTEL_MEI is not set
> CONFIG_NETDEVICES=y
> CONFIG_VIRTIO_NET=y
> # CONFIG_ETHERNET is not set
> # CONFIG_WLAN is not set
> CONFIG_INPUT_EVDEV=y
> CONFIG_SERIAL_8250=y
> CONFIG_SERIAL_8250_CONSOLE=y
> CONFIG_VIRTIO_CONSOLE=y
> CONFIG_HW_RANDOM_VIRTIO=m
> CONFIG_PTP_1588_CLOCK=y
> # CONFIG_HWMON is not set
> CONFIG_THERMAL_GOV_USER_SPACE=y
> CONFIG_DRM=y
> CONFIG_DRM_KUNIT_TEST=m
> CONFIG_DRM_VKMS=m
> CONFIG_DRM_VKMS_KUNIT_TEST=m
> # CONFIG_USB_SUPPORT is not set
> CONFIG_VIRTIO_PCI=y
> CONFIG_VIRTIO_BALLOON=y
> CONFIG_VIRTIO_INPUT=y
> CONFIG_VIRTIO_MMIO=y
> CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES=y
> # CONFIG_SURFACE_PLATFORMS is not set
> CONFIG_EXT4_FS=y
> CONFIG_FUSE_FS=y
> CONFIG_VIRTIO_FS=y
> CONFIG_OVERLAY_FS=y
> CONFIG_TMPFS=y
> CONFIG_TMPFS_POSIX_ACL=y
> CONFIG_CONFIGFS_FS=y
> CONFIG_9P_FS=y
> CONFIG_CRYPTO=y
> CONFIG_CRYPTO_CRC32C=y
> CONFIG_DYNAMIC_DEBUG=y
> CONFIG_DEBUG_KERNEL=y
> CONFIG_DEBUG_INFO_DWARF5=y
> CONFIG_MAGIC_SYSRQ=y
> CONFIG_DEBUG_FS=y
> CONFIG_DEBUG_PAGEALLOC=y
> CONFIG_DEBUG_PAGEALLOC_ENABLE_DEFAULT=y
> CONFIG_PAGE_POISONING=y
> CONFIG_DEBUG_OBJECTS=y
> CONFIG_DEBUG_OBJECTS_RCU_HEAD=y
> CONFIG_SCHED_STACK_END_CHECK=y
> CONFIG_KASAN=y
> CONFIG_KASAN_VMALLOC=y
> CONFIG_KASAN_EXTRA_INFO=y
> CONFIG_KFENCE=y
> # CONFIG_FTRACE is not set
> CONFIG_UNWINDER_FRAME_POINTER=y
> CONFIG_KUNIT=y
> CONFIG_TEST_DYNAMIC_DEBUG=m
>
Le 04/03/2025 à 17:23, José Expósito a écrit :
> On Tue, Mar 04, 2025 at 04:35:50PM +0100, Louis Chauvet wrote:
>>
>>
>> Le 04/03/2025 à 15:54, José Expósito a écrit :
>>> 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@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'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.
>>
>> I just re-applied your last vkms-config version and this series on top of
>> drm-misc-next. See [1] for the exact commits.
>>
>> Argg sorry, I just noticed something: you need to disable the default vkms
>> device (I had this option in my kernel command line...), otherwise modetest
>> only use the first vkms gpu...
>>
>> I will check again the igt tests, but I don't think this is the same issue
>> (it should not use the default device to test)
>>
>> So, with [1] and the defconfig below, I have this:
>>
>>
>> 1 modprobe vkms create_default_dev=0
>> 2 cd /sys/kernel/config/vkms/
>> 3 mkdir DEV
>> 4 mkdir DEV/connectors/CON
>> 5 mkdir DEV/planes/PLA
>> 6 mkdir DEV/crtcs/CRT
>> 7 mkdir DEV/encoders/ENC
>> 8 ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
>> 9 ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
>> 10 ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
>> 11 echo 1 > DEV/planes/PLA/type
>> 12 tree
>> 13 echo 1 > DEV/enabled
>> 14 modetest -M vkms
>> 15 rm DEV/connectors/CON/possible_encoders/ENC
>> 16 rmdir DEV/connectors/CON
>> 17 modetest -M vkms
>> KASAN: slab-use-after-free
>>
>>
>> [1]:https://github.com/Fomys/linux/tree/20250225175936.7223-1-jose.exposito89%40gmail.com
>
> Aha! Are you testing without a desktop environment running?
Yes! I run a minimal VM (virtme-ng), so no services are started.
>
> $ sudo systemctl isolate multi-user.target
>
> Running that (^) command before yours gives me this use after free:
>
> BUG: KASAN: slab-use-after-free in vkms_connector_detect+0x61/0x70 [vkms]
>
> Is the same one you are seeing?
Yes!
> Looking at the connector_release() function in vkms_configfs.c, I see
> that I'm freeing the configuration:
>
> vkms_config_destroy_connector(connector->config);
>
> And I don't think there is a reason to do it. vkms_config_destroy() in
> device_release() will free everything once we are done.
Yes, but if you don't free it will always remain in the config, which
means that:
modprobe vkms create_default_dev=0
cd /sys/kernel/config/vkms/
mkdir DEV
mkdir DEV/connectors/CON
mkdir DEV/crtcs/CRT
mkdir DEV/planes/PLA
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
echo 1 > DEV/enabled
echo 0 > DEV/enabled
rm DEV/connectors/CON/possible_encoders/ENC
rmdir DEV/connectors/CON
echo 1 > DEV/enabled
Expected (and current) result:
(NULL device *): [drm] The number of connectors must be between 1 and 31
Result with the diff:
- vkms_config_destroy_connector(connector->config);
+ //vkms_config_destroy_connector(connector->config);
(NULL device *): [drm] All connectors must have at least one possible
encoder
This is because the connector list in vkms_config contains the deleted
CON connector. If you also remove the connector from the list, it will
be a memory leak.
The solution I proposed with get/put should solve this: once the device
is disabled, all the release functions (and the corresponding
vkms_config_destroy) will be called, so no issue there.
I attempted to do it, but it looks a bit more complex than I expected:
- config_item_get works as expected, if you get all the items in
device_enabled_store they are not released, even if the directory is
deleted;
- but as the directory is deleted, you can't use cg_children to put your
last reference on it.
I think a solution could be to add refcounting in vkms_config, it seems
to work, and it is even better, the refcounting is done in the vkms
driver, not in configfs (I did it only for connector, see below). It
seems to work as expected and doesn't increase the complexity of the code.
Do you think this is sufficient/clear enough?
Have a nice day,
Louis Chauvet
diff --git a/drivers/gpu/drm/vkms/vkms_config.c
b/drivers/gpu/drm/vkms/vkms_config.c
index f8394a063ecf..4dc65ab15517 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0+
+#include <linux/kref.h>
#include <linux/slab.h>
#include <drm/drm_print.h>
@@ -123,7 +124,7 @@ void vkms_config_destroy(struct vkms_config *config)
vkms_config_destroy_encoder(config, encoder_cfg);
list_for_each_entry_safe(connector_cfg, connector_tmp,
&config->connectors, link)
- vkms_config_destroy_connector(connector_cfg);
+ vkms_config_connector_put(connector_cfg);
kfree_const(config->dev_name);
kfree(config);
@@ -596,17 +597,32 @@ struct vkms_config_connector
*vkms_config_create_connector(struct vkms_config *c
list_add_tail(&connector_cfg->link, &config->connectors);
+ kref_init(&connector_cfg->ref);
return connector_cfg;
}
EXPORT_SYMBOL_IF_KUNIT(vkms_config_create_connector);
-void vkms_config_destroy_connector(struct vkms_config_connector
*connector_cfg)
+static void vkms_config_destroy_connector(struct vkms_config_connector
*connector_cfg)
{
xa_destroy(&connector_cfg->possible_encoders);
list_del(&connector_cfg->link);
kfree(connector_cfg);
}
-EXPORT_SYMBOL_IF_KUNIT(vkms_config_destroy_connector);
+// EXPORT_SYMBOL_IF_KUNIT(vkms_config_destroy_connector);
+
+static void vkms_config_destroy_connector_kref(struct kref *kref)
+{
+ struct vkms_config_connector *connector = container_of(kref, struct
vkms_config_connector, ref);
+
+ vkms_config_destroy_connector(connector);
+}
+
+void vkms_config_connector_get(struct vkms_config_connector *connector) {
+ kref_get(&connector->ref);
+}
+void vkms_config_connector_put(struct vkms_config_connector *connector) {
+ kref_put(&connector->ref, vkms_config_destroy_connector_kref);
+}
int __must_check vkms_config_connector_attach_encoder(struct
vkms_config_connector *connector_cfg,
struct vkms_config_encoder *encoder_cfg)
diff --git a/drivers/gpu/drm/vkms/vkms_config.h
b/drivers/gpu/drm/vkms/vkms_config.h
index e202b5a84ddd..30e6c6bf34f4 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -3,6 +3,7 @@
#ifndef _VKMS_CONFIG_H_
#define _VKMS_CONFIG_H_
+#include <linux/kref.h>
#include <linux/list.h>
#include <linux/types.h>
#include <linux/xarray.h>
@@ -109,6 +110,7 @@ struct vkms_config_encoder {
* configuration and must be managed by other means.
*/
struct vkms_config_connector {
+ struct kref ref;
struct list_head link;
struct vkms_config *config;
@@ -416,11 +418,8 @@ void vkms_config_encoder_detach_crtc(struct
vkms_config_encoder *encoder_cfg,
*/
struct vkms_config_connector *vkms_config_create_connector(struct
vkms_config *config);
-/**
- * vkms_config_destroy_connector() - Remove and free a connector
configuration
- * @connector_cfg: Connector configuration to destroy
- */
-void vkms_config_destroy_connector(struct vkms_config_connector
*connector_cfg);
+void vkms_config_connector_get(struct vkms_config_connector *connector);
+void vkms_config_connector_put(struct vkms_config_connector *connector);
/**
* vkms_config_connector_attach_encoder - Attach a connector to an encoder
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c
b/drivers/gpu/drm/vkms/vkms_configfs.c
index 76afb0245388..ae929a084feb 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -550,7 +550,7 @@ static void connector_release(struct config_item *item)
lock = &connector->dev->lock;
guard(mutex)(lock);
- vkms_config_destroy_connector(connector->config);
+ vkms_config_connector_put(connector->config);
kfree(connector);
}
diff --git a/drivers/gpu/drm/vkms/vkms_connector.c
b/drivers/gpu/drm/vkms/vkms_connector.c
index ed99270c590f..c15451b50440 100644
--- a/drivers/gpu/drm/vkms/vkms_connector.c
+++ b/drivers/gpu/drm/vkms/vkms_connector.c
@@ -20,6 +20,15 @@ static enum drm_connector_status
vkms_connector_detect(struct drm_connector *con
return status;
}
+static void vkms_connector_destroy(struct drm_device *dev, void *raw)
+{
+ struct vkms_connector *vkms_connector = raw;
+
+ vkms_config_connector_put(vkms_connector->connector_cfg);
+
+ return;
+}
+
static const struct drm_connector_funcs vkms_connector_funcs = {
.detect = vkms_connector_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
@@ -65,8 +74,13 @@ struct vkms_connector *vkms_connector_init(struct
vkms_device *vkmsdev,
if (!connector)
return ERR_PTR(-ENOMEM);
+ vkms_config_connector_get(connector->connector_cfg);
connector->connector_cfg = connector_cfg;
+ ret = drmm_add_action_or_reset(dev, &vkms_connector_destroy, connector);
+ if (ret)
+ return ERR_PTR(ret);
+
ret = drmm_connector_init(dev, &connector->base, &vkms_connector_funcs,
DRM_MODE_CONNECTOR_VIRTUAL, NULL);
if (ret)
Hi Louis,
On Tue, Mar 04, 2025 at 07:17:53PM +0100, Louis Chauvet wrote:
>
>
> Le 04/03/2025 à 17:23, José Expósito a écrit :
> > On Tue, Mar 04, 2025 at 04:35:50PM +0100, Louis Chauvet wrote:
> > >
> > >
> > > Le 04/03/2025 à 15:54, José Expósito a écrit :
> > > > 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@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'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.
> > >
> > > I just re-applied your last vkms-config version and this series on top of
> > > drm-misc-next. See [1] for the exact commits.
> > >
> > > Argg sorry, I just noticed something: you need to disable the default vkms
> > > device (I had this option in my kernel command line...), otherwise modetest
> > > only use the first vkms gpu...
> > >
> > > I will check again the igt tests, but I don't think this is the same issue
> > > (it should not use the default device to test)
> > >
> > > So, with [1] and the defconfig below, I have this:
> > >
> > >
> > > 1 modprobe vkms create_default_dev=0
> > > 2 cd /sys/kernel/config/vkms/
> > > 3 mkdir DEV
> > > 4 mkdir DEV/connectors/CON
> > > 5 mkdir DEV/planes/PLA
> > > 6 mkdir DEV/crtcs/CRT
> > > 7 mkdir DEV/encoders/ENC
> > > 8 ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
> > > 9 ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
> > > 10 ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
> > > 11 echo 1 > DEV/planes/PLA/type
> > > 12 tree
> > > 13 echo 1 > DEV/enabled
> > > 14 modetest -M vkms
> > > 15 rm DEV/connectors/CON/possible_encoders/ENC
> > > 16 rmdir DEV/connectors/CON
> > > 17 modetest -M vkms
> > > KASAN: slab-use-after-free
> > >
> > >
> > > [1]:https://github.com/Fomys/linux/tree/20250225175936.7223-1-jose.exposito89%40gmail.com
> >
> > Aha! Are you testing without a desktop environment running?
>
> Yes! I run a minimal VM (virtme-ng), so no services are started.
> >
> > $ sudo systemctl isolate multi-user.target
> >
> > Running that (^) command before yours gives me this use after free:
> >
> > BUG: KASAN: slab-use-after-free in vkms_connector_detect+0x61/0x70 [vkms]
> >
> > Is the same one you are seeing?
>
> Yes!
>
> > Looking at the connector_release() function in vkms_configfs.c, I see
> > that I'm freeing the configuration:
> >
> > vkms_config_destroy_connector(connector->config);
> >
> > And I don't think there is a reason to do it. vkms_config_destroy() in
> > device_release() will free everything once we are done.
>
> Yes, but if you don't free it will always remain in the config, which means
> that:
Busy week, but I finally have a couple of hours to sit and look into this
with detail.
The problem is in my patch to implement connector hot-plug ("drm/vkms: Allow
to update the connector status").
I was storing a pointer to the connector configuration in vkms_connector (OK)
and using it to get the connector status (wrong).
The configuration is mean to be used during device initialization, but after
that, it can change at any point responding to configfs changes.
I even documented that accessing vkms_config_connector->connector is not
safe... Just to access it a few patches later (sigh).
In my opinion, the solution is to avoid this behavior. This is how the fix
looks like [1]. The code is even simpler than the previous version.
[1] https://github.com/JoseExposito/linux/commit/da116085590d644575e9d78fb5c3a665aa7848f5
> modprobe vkms create_default_dev=0
> cd /sys/kernel/config/vkms/
> mkdir DEV
> mkdir DEV/connectors/CON
> mkdir DEV/crtcs/CRT
> mkdir DEV/planes/PLA
> 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
> echo 1 > DEV/enabled
> echo 0 > DEV/enabled
> rm DEV/connectors/CON/possible_encoders/ENC
> rmdir DEV/connectors/CON
> echo 1 > DEV/enabled
>
> Expected (and current) result:
>
> (NULL device *): [drm] The number of connectors must be between 1 and 31
>
> Result with the diff:
> - vkms_config_destroy_connector(connector->config);
> + //vkms_config_destroy_connector(connector->config);
>
> (NULL device *): [drm] All connectors must have at least one possible
> encoder
>
> This is because the connector list in vkms_config contains the deleted CON
> connector. If you also remove the connector from the list, it will be a
> memory leak.
>
> The solution I proposed with get/put should solve this: once the device is
> disabled, all the release functions (and the corresponding
> vkms_config_destroy) will be called, so no issue there.
>
> I attempted to do it, but it looks a bit more complex than I expected:
> - config_item_get works as expected, if you get all the items in
> device_enabled_store they are not released, even if the directory is
> deleted;
> - but as the directory is deleted, you can't use cg_children to put your
> last reference on it.
>
> I think a solution could be to add refcounting in vkms_config, it seems to
> work, and it is even better, the refcounting is done in the vkms driver, not
> in configfs (I did it only for connector, see below). It seems to work as
> expected and doesn't increase the complexity of the code.
>
> Do you think this is sufficient/clear enough?
This would also work. But, is it worth to include this complexity now?
I think that the configuration is handled correctly as it is now, we just
need to make sure that we don't rely on invalid pointers (as documented).
Enjoy your day,
Jose
> Have a nice day,
> Louis Chauvet
>
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c
> b/drivers/gpu/drm/vkms/vkms_config.c
> index f8394a063ecf..4dc65ab15517 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0+
>
> +#include <linux/kref.h>
> #include <linux/slab.h>
>
> #include <drm/drm_print.h>
> @@ -123,7 +124,7 @@ void vkms_config_destroy(struct vkms_config *config)
> vkms_config_destroy_encoder(config, encoder_cfg);
>
> list_for_each_entry_safe(connector_cfg, connector_tmp,
> &config->connectors, link)
> - vkms_config_destroy_connector(connector_cfg);
> + vkms_config_connector_put(connector_cfg);
>
> kfree_const(config->dev_name);
> kfree(config);
> @@ -596,17 +597,32 @@ struct vkms_config_connector
> *vkms_config_create_connector(struct vkms_config *c
>
> list_add_tail(&connector_cfg->link, &config->connectors);
>
> + kref_init(&connector_cfg->ref);
> return connector_cfg;
> }
> EXPORT_SYMBOL_IF_KUNIT(vkms_config_create_connector);
>
> -void vkms_config_destroy_connector(struct vkms_config_connector
> *connector_cfg)
> +static void vkms_config_destroy_connector(struct vkms_config_connector
> *connector_cfg)
> {
> xa_destroy(&connector_cfg->possible_encoders);
> list_del(&connector_cfg->link);
> kfree(connector_cfg);
> }
> -EXPORT_SYMBOL_IF_KUNIT(vkms_config_destroy_connector);
> +// EXPORT_SYMBOL_IF_KUNIT(vkms_config_destroy_connector);
> +
> +static void vkms_config_destroy_connector_kref(struct kref *kref)
> +{
> + struct vkms_config_connector *connector = container_of(kref, struct
> vkms_config_connector, ref);
> +
> + vkms_config_destroy_connector(connector);
> +}
> +
> +void vkms_config_connector_get(struct vkms_config_connector *connector) {
> + kref_get(&connector->ref);
> +}
> +void vkms_config_connector_put(struct vkms_config_connector *connector) {
> + kref_put(&connector->ref, vkms_config_destroy_connector_kref);
> +}
>
> int __must_check vkms_config_connector_attach_encoder(struct
> vkms_config_connector *connector_cfg,
> struct vkms_config_encoder *encoder_cfg)
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h
> b/drivers/gpu/drm/vkms/vkms_config.h
> index e202b5a84ddd..30e6c6bf34f4 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -3,6 +3,7 @@
> #ifndef _VKMS_CONFIG_H_
> #define _VKMS_CONFIG_H_
>
> +#include <linux/kref.h>
> #include <linux/list.h>
> #include <linux/types.h>
> #include <linux/xarray.h>
> @@ -109,6 +110,7 @@ struct vkms_config_encoder {
> * configuration and must be managed by other means.
> */
> struct vkms_config_connector {
> + struct kref ref;
> struct list_head link;
> struct vkms_config *config;
>
> @@ -416,11 +418,8 @@ void vkms_config_encoder_detach_crtc(struct
> vkms_config_encoder *encoder_cfg,
> */
> struct vkms_config_connector *vkms_config_create_connector(struct
> vkms_config *config);
>
> -/**
> - * vkms_config_destroy_connector() - Remove and free a connector
> configuration
> - * @connector_cfg: Connector configuration to destroy
> - */
> -void vkms_config_destroy_connector(struct vkms_config_connector
> *connector_cfg);
> +void vkms_config_connector_get(struct vkms_config_connector *connector);
> +void vkms_config_connector_put(struct vkms_config_connector *connector);
>
> /**
> * vkms_config_connector_attach_encoder - Attach a connector to an encoder
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c
> b/drivers/gpu/drm/vkms/vkms_configfs.c
> index 76afb0245388..ae929a084feb 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -550,7 +550,7 @@ static void connector_release(struct config_item *item)
> lock = &connector->dev->lock;
>
> guard(mutex)(lock);
> - vkms_config_destroy_connector(connector->config);
> + vkms_config_connector_put(connector->config);
> kfree(connector);
> }
>
> diff --git a/drivers/gpu/drm/vkms/vkms_connector.c
> b/drivers/gpu/drm/vkms/vkms_connector.c
> index ed99270c590f..c15451b50440 100644
> --- a/drivers/gpu/drm/vkms/vkms_connector.c
> +++ b/drivers/gpu/drm/vkms/vkms_connector.c
> @@ -20,6 +20,15 @@ static enum drm_connector_status
> vkms_connector_detect(struct drm_connector *con
> return status;
> }
>
> +static void vkms_connector_destroy(struct drm_device *dev, void *raw)
> +{
> + struct vkms_connector *vkms_connector = raw;
> +
> + vkms_config_connector_put(vkms_connector->connector_cfg);
> +
> + return;
> +}
> +
> static const struct drm_connector_funcs vkms_connector_funcs = {
> .detect = vkms_connector_detect,
> .fill_modes = drm_helper_probe_single_connector_modes,
> @@ -65,8 +74,13 @@ struct vkms_connector *vkms_connector_init(struct
> vkms_device *vkmsdev,
> if (!connector)
> return ERR_PTR(-ENOMEM);
>
> + vkms_config_connector_get(connector->connector_cfg);
> connector->connector_cfg = connector_cfg;
>
> + ret = drmm_add_action_or_reset(dev, &vkms_connector_destroy, connector);
> + if (ret)
> + return ERR_PTR(ret);
> +
> ret = drmm_connector_init(dev, &connector->base, &vkms_connector_funcs,
> DRM_MODE_CONNECTOR_VIRTUAL, NULL);
> if (ret)
>
>
Introduce a new mechanism in configfs to prevent the deletion of certain
symlink.
This is particularly useful in scenarios where userspace should not be
allowed
to modify the configfs structure under some conditions, such as in VKMS.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_configfs.c | 20 ++++++++++++++++++++
fs/configfs/symlink.c | 12 +++++++++---
include/linux/configfs.h | 1 +
3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c
b/drivers/gpu/drm/vkms/vkms_configfs.c
index f0813536be12..8a7d954399e9 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -295,8 +295,28 @@ static void plane_possible_crtcs_drop_link(struct
config_item *src,
mutex_unlock(&plane->dev->lock);
}
+static int plane_possible_crtcs_allow_drop_link(struct config_item *src,
+ struct config_item *target)
+{
+ struct vkms_configfs_plane *plane;
+ struct vkms_configfs_crtc *crtc;
+ bool enabled;
+
+ plane = plane_possible_crtcs_item_to_vkms_configfs_plane(src);
+ crtc = crtc_item_to_vkms_configfs_crtc(target);
+
+ mutex_lock(&plane->dev->lock);
+ enabled = plane->dev->enabled;
+ mutex_unlock(&plane->dev->lock);
+
+ if (enabled)
+ return -EBUSY;
+ return 0;
+}
+
static struct configfs_item_operations
plane_possible_crtcs_item_operations = {
.allow_link = plane_possible_crtcs_allow_link,
+ .allow_drop_link = plane_possible_crtcs_allow_drop_link,
.drop_link = plane_possible_crtcs_drop_link,
};
diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 69133ec1fac2..925e2e15eb9b 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -233,6 +233,13 @@ int configfs_unlink(struct inode *dir, struct
dentry *dentry)
parent_item = configfs_get_config_item(dentry->d_parent);
type = parent_item->ci_type;
+ if (type && type->ct_item_ops &&
+ type->ct_item_ops->allow_drop_link) {
+ ret = type->ct_item_ops->allow_drop_link(parent_item,
target_sd->s_element);
+ if (ret)
+ goto out_put;
+ }
+
spin_lock(&configfs_dirent_lock);
list_del_init(&sd->s_sibling);
spin_unlock(&configfs_dirent_lock);
@@ -255,10 +262,9 @@ int configfs_unlink(struct inode *dir, struct
dentry *dentry)
spin_unlock(&configfs_dirent_lock);
configfs_put(target_sd);
- config_item_put(parent_item);
-
ret = 0;
-
+out_put:
+ config_item_put(parent_item);
out:
return ret;
}
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index c771e9d0d0b9..7fc52a78d6cd 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -208,6 +208,7 @@ static struct configfs_bin_attribute
_pfx##attr_##_name = { \
struct configfs_item_operations {
void (*release)(struct config_item *);
int (*allow_link)(struct config_item *src, struct config_item *target);
+ int (*allow_drop_link)(struct config_item *src, struct config_item
*target);
void (*drop_link)(struct config_item *src, struct config_item *target);
};
--
2.48.1Introduce a new mechanism in configfs to prevent the deletion of certain
item/group.
This is particularly useful in scenarios where userspace should not be
allowed
to modify the configfs structure under some conditions, such as in VKMS.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_configfs.c | 21 +++++++++++++++++++++
fs/configfs/dir.c | 8 +++++++-
include/linux/configfs.h | 1 +
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c
b/drivers/gpu/drm/vkms/vkms_configfs.c
index 8a7d954399e9..0a196a28ed4a 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -393,9 +393,30 @@ static struct configfs_item_operations
plane_item_operations = {
.release = &plane_release,
};
+int allow_drop_plane_group(struct config_group *group, struct
config_item *item)
+{
+ struct vkms_configfs_plane *plane;
+ bool enabled;
+
+ plane = plane_item_to_vkms_configfs_plane(item);
+
+ mutex_lock(&plane->dev->lock);
+ enabled = plane->dev->enabled;
+ mutex_unlock(&plane->dev->lock);
+
+ if (enabled)
+ return -EBUSY;
+ return 0;
+}
+
+static struct configfs_group_operations plane_group_operation ={
+ .allow_drop_item = &allow_drop_plane_group,
+};
+
static const struct config_item_type plane_item_type = {
.ct_attrs = plane_item_attrs,
.ct_item_ops = &plane_item_operations,
+ .ct_group_ops = &plane_group_operation,
.ct_owner = THIS_MODULE,
};
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 7d10278db30d..a103196af0f9 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1544,7 +1544,13 @@ static int configfs_rmdir(struct inode *dir,
struct dentry *dentry)
/* Drop reference from above, item already holds one. */
config_item_put(parent_item);
-
+ if (item->ci_type && item->ci_type->ct_group_ops &&
item->ci_type->ct_group_ops->allow_drop_item) {
+ ret =
item->ci_type->ct_group_ops->allow_drop_item(to_config_group(parent_item),
item);
+ if (ret) {
+ config_item_put(item);
+ return ret;
+ }
+ }
if (item->ci_type)
dead_item_owner = item->ci_type->ct_owner;
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 7fc52a78d6cd..92933397590d 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -216,6 +216,7 @@ struct configfs_group_operations {
struct config_item *(*make_item)(struct config_group *group, const
char *name);
struct config_group *(*make_group)(struct config_group *group, const
char *name);
void (*disconnect_notify)(struct config_group *group, struct
config_item *item);
+ int (*allow_drop_item)(struct config_group *group, struct config_item
*item);
void (*drop_item)(struct config_group *group, struct config_item *item);
bool (*is_visible)(struct config_item *item, struct
configfs_attribute *attr, int n);
bool (*is_bin_visible)(struct config_item *item, struct
configfs_bin_attribute *attr,
--
2.48.1© 2016 - 2026 Red Hat, Inc.