Re: [PATCH v2 03/16] drm/vkms: Allow to configure multiple planes via configfs

["José Expósito" <jose.exposito89@gmail.com>]

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
>