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@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.
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)
