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

Hi Louis,

On 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@...tlin.com>
> > > > > > > > Co-developed-by: Louis Chauvet <louis.chauvet@...tlin.com>
> > > > > > > > Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>
> > > > > > > > Signed-off-by: José Expósito <jose.exposito89@...il.com>
> > > > > > > > [...]
> > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > > > > > index 92512d52ddae..4f9d3341e6c0 100644
> > > > > > > > --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> > > > > > > > [...]
> > > > > > > > +static void plane_release(struct config_item *item)
> > > > > > > > +{
> > > > > > > > +	struct vkms_configfs_plane *plane;
> > > > > > > > +	struct mutex *lock;
> > > > > > > > +
> > > > > > > > +	plane = plane_item_to_vkms_configfs_plane(item);
> > > > > > > > +	lock = &plane->dev->lock;
> > > > > > > > +
> > > > > > > > +	guard(mutex)(lock);
> > > > > > > > +	vkms_config_destroy_plane(plane->config);
> > > > > > > > +	kfree(plane);
> > > > > > > > +}
> > > > > > > 
> > > > > > > I just found a flaw in our work: there is currently no way to forbid the
> > > > > > > deletion of item/symlinks...
> > > > > > > 
> > > > > > > If you do:
> > > > > > > 
> > > > > > > modprobe vkms
> > > > > > > cd /sys/kernel/config/vkms/
> > > > > > > mkdir DEV
> > > > > > > mkdir DEV/connectors/CON
> > > > > > > mkdir DEV/planes/PLA
> > > > > > > mkdir DEV/crtcs/CRT
> > > > > > > mkdir DEV/encoders/ENC
> > > > > > > ln -s DEV/crtcs/CRT DEV/planes/PLA/possible_crtcs/
> > > > > > > ln -s DEV/crtcs/CRT DEV/encoders/ENC/possible_crtcs
> > > > > > > ln -s DEV/encoders/ENC DEV/connectors/CON/possible_encoders
> > > > > > > echo 1 > DEV/planes/PLA/type
> > > > > > > tree
> > > > > > > echo 1 > DEV/enabled
> > > > > > > modetest -M vkms
> > > > > > > => everything fine
> > > > > > > 
> > > > > > > rm DEV/connectors/CON/possible_encoders/ENC
> > > > > > > rmdir DEV/connectors/CON
> > > > > > > modetest -M vkms
> > > > > > > => BUG: KASAN: slab-use-after-free
> > > > 
> > > > I'm trying to reproduce this issue, but those commands don't show any BUG
> > > > in dmesg. This is my Kasan .config:
> > > > 
> > > >       CONFIG_HAVE_ARCH_KASAN=y
> > > >       CONFIG_HAVE_ARCH_KASAN_VMALLOC=y
> > > >       CONFIG_CC_HAS_KASAN_GENERIC=y
> > > >       CONFIG_CC_HAS_KASAN_SW_TAGS=y
> > > >       CONFIG_CC_HAS_WORKING_NOSANITIZE_ADDRESS=y
> > > >       CONFIG_KASAN=y
> > > >       CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX=y
> > > >       CONFIG_KASAN_GENERIC=y
> > > >       # CONFIG_KASAN_OUTLINE is not set
> > > >       CONFIG_KASAN_INLINE=y
> > > >       CONFIG_KASAN_STACK=y
> > > >       CONFIG_KASAN_VMALLOC=y
> > > >       # CONFIG_KASAN_KUNIT_TEST is not set
> > > >       CONFIG_KASAN_EXTRA_INFO=y
> > > > 
> > > > I tryed to delete even more items:
> > > > 
> > > >       root@...nel-dev:/sys/kernel/config/vkms# tree
> > > >       .
> > > >       └── DEV
> > > >           ├── connectors
> > > >           ├── crtcs
> > > >           ├── enabled
> > > >           ├── encoders
> > > >           └── planes
> > > > 
> > > >       root@...nel-dev:/sys/kernel/config/vkms# cat DEV/enabled
> > > >       1
> > > > 
> > > > And I still don't see any errors. Is it possible that we are running different
> > > > branches? Asking because of the failing IGT tests you reported. There seems to
> > > > be a difference in our code or setup that is creating these differences.
> > > 
> > > 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)
> 
> 

Powered by blists - more mailing lists