Message-ID: <7909080.EvYhyI6sBW@workhorse>
Date: Mon, 09 Feb 2026 20:38:48 +0100
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Ville Syrjälä <ville.syrjala@...ux.intel.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Haneen Mohammed <hamohammed.sa@...il.com>,
 Melissa Wen <melissa.srw@...il.com>,
 Daniel Stone <daniel.stone@...labora.com>,
 Ian Forbes <ian.forbes@...adcom.com>,
 Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
 Louis Chauvet <louis.chauvet@...tlin.com>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 kernel@...labora.com, Marius Vlad <marius.vlad@...labora.com>
Subject:
 Re: [PATCH v6 4/4] vkms: Pass the vkms connector as opposed to the device on
 hotplug

Hi Louis,

On Saturday, 24 January 2026 02:06:27 Central European Standard Time Louis Chauvet wrote:
> Hello Nicolas,
> 
> On the principle I agree with this patch, I just need to take time to 
> properly think about lifetime of vkms_config vs connector and pointer 
> validity to avoid use-after-free / null pointer dereference. I will try 
> to review it next week or more probably just after the FOSDEM.

I should've offered we could've discussed it in person at FOSDEM since
I was there as well, but I forgot. :(

> 
> In the meantime, if you want to try / think about the possible issue: I 
> think there will be a use-after-free if you unbind the driver using the 
> sysfs interface and interract with configfs interface.

I can sort of see what I think you're seeing, but the specific concern
below looks to me like something that already is a potential problem
in vkms. That is, if I understood the scenario envisioned here.

> 
> Thanks a lot for this series,
> Louis Chauvet
> 
> 
> On 1/23/26 20:44, Nicolas Frattaroli wrote:
> > From: Marius Vlad <marius.vlad@...labora.com>
> > 
> > By passing the connector rather than the device to
> > vkms_trigger_connector_hotplug, vkms can trigger connector hotplugging
> > events that contain the connector ID.
> > 
> > Signed-off-by: Marius Vlad <marius.vlad@...labora.com>
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
> > ---
> >   drivers/gpu/drm/vkms/vkms_configfs.c  | 2 +-
> >   drivers/gpu/drm/vkms/vkms_connector.c | 6 +++---
> >   drivers/gpu/drm/vkms/vkms_connector.h | 4 ++--
> >   3 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> > index d6e203d59b45..63a27f671e6a 100644
> > --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> > +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> > @@ -554,7 +554,7 @@ static ssize_t connector_status_store(struct config_item *item,
> >   		vkms_config_connector_set_status(connector->config, status);
> >   
> >   		if (connector->dev->enabled && old_status != status)
> > -			vkms_trigger_connector_hotplug(connector->dev->config->dev);
> > +			vkms_trigger_connector_hotplug(connector->config->connector);
> 
> Here connector->config is valid, but connector->config->connector is 
> probably invalid if the driver is unbind. We may need to add some 
> refcount to avoid this kind of issue.

I'm fairly sure whatever race condition you're worried about here
already exists in the old code anyway. connector->dev->config will
have its vkms_device first released, and then the config->dev member
set to NULL, without a lock, in vkms_destroy. That vkms_device is
allocated in vkms_create with devm_drm_dev_alloc using the fauxdev's
dev as a devres group, which is the one being freed in vkms_destroy
while momentarily leaving a dangling pointer in the config struct.

There's nothing fundamentally more broken here as this already is
if the configfs can somehow persist during vkms unbind, if I
understand you correctly by what you mean with driver unbind. The
vkms_configfs_unregister() call in vkms_exit before the vkms teardown
makes me think that this isn't supposed to happen anyway; on vkms
module exit, the configfs stuff is torn down before the vkms
resources are freed.

If it is a realistic scenario and I just disclosed a potential
use-after-free that's already in the kernel on a public mailing list,
then: oops, guess this one's a freebie for the red teams.

> The other way around, I think there could be issue if the configfs 
> folder is completly removed (that possible, there is no way to forbid 
> deletions in configfs), the config object is freed but maybe used in the 
> "DRM" part of VKMS (for connector status update maybe).

Is there ever a situation where a non-root user can delete configfs that
wouldn't also be a whole can of worms in other aspects?

A more general point:

The vkms part here was only ever there so the HPD stuff can be tested
without a hardware test setup or by poking the sysfs of real hardware.
I inherited it when I took the series over, but don't use it myself
beyond testing these two vkms patches.

If I need to add some refcounting scheme to vkms connectors then I
assume this will take a few more revisions to get right. I hope the
non-vkms parts can be reviewed by those who raised concerns on the
previous revision in the meantime, so I know whether I'm making
progress here.

I'll discuss with my colleagues whether the vkms parts are something
we absolutely need. If not, I'll just drop them in the next revision,
unless there's a fairly good case to be made from your side that
they'd find use and are worth the time investment.

Kind regards,
Nicolas Frattaroli

> 
> >   	}
> >   
> >   	return (ssize_t)count;
> > diff --git a/drivers/gpu/drm/vkms/vkms_connector.c b/drivers/gpu/drm/vkms/vkms_connector.c
> > index b0a6b212d3f4..cad64eff72ea 100644
> > --- a/drivers/gpu/drm/vkms/vkms_connector.c
> > +++ b/drivers/gpu/drm/vkms/vkms_connector.c
> > @@ -88,9 +88,9 @@ struct vkms_connector *vkms_connector_init(struct vkms_device *vkmsdev)
> >   	return connector;
> >   }
> >   
> > -void vkms_trigger_connector_hotplug(struct vkms_device *vkmsdev)
> > +void vkms_trigger_connector_hotplug(struct vkms_connector *vkms_connector)
> >   {
> > -	struct drm_device *dev = &vkmsdev->drm;
> > +	struct drm_connector *connector = &vkms_connector->base;
> >   
> > -	drm_kms_helper_hotplug_event(dev);
> > +	drm_kms_helper_connector_hotplug_event(connector);
> >   }
> > diff --git a/drivers/gpu/drm/vkms/vkms_connector.h b/drivers/gpu/drm/vkms/vkms_connector.h
> > index ed312f4eff3a..7cd76d01b10b 100644
> > --- a/drivers/gpu/drm/vkms/vkms_connector.h
> > +++ b/drivers/gpu/drm/vkms/vkms_connector.h
> > @@ -28,8 +28,8 @@ struct vkms_connector *vkms_connector_init(struct vkms_device *vkmsdev);
> >   
> >   /**
> >    * vkms_trigger_connector_hotplug() - Update the device's connectors status
> > - * @vkmsdev: VKMS device to update
> > + * @vkms_connector: VKMS connector to update
> >    */
> > -void vkms_trigger_connector_hotplug(struct vkms_device *vkmsdev);
> > +void vkms_trigger_connector_hotplug(struct vkms_connector *vkms_connector);
> >   
> >   #endif /* _VKMS_CONNECTOR_H_ */
> > 
> 
> 

Powered by blists - more mailing lists