linux-kernel - Re: [PATCH v3 8/8] drm/vkms: convert to use faux

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z6cX5MG3yM4XL9jR@Host-003>
Date: Sat, 8 Feb 2025 09:37:56 +0100
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org, "Rafael J. Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>, Lyude Paul <lyude@...hat.com>,
	Alexander Lobakin <aleksander.lobakin@...el.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	Liam Girdwood <lgirdwood@...il.com>, Lukas Wunner <lukas@...ner.de>,
	Mark Brown <broonie@...nel.org>,
	Maíra Canal <mairacanal@...eup.net>,
	Robin Murphy <robin.murphy@....com>,
	Simona Vetter <simona.vetter@...ll.ch>,
	Zijun Hu <quic_zijuhu@...cinc.com>, linux-usb@...r.kernel.org,
	rust-for-linux@...r.kernel.org,
	Haneen Mohammed <hamohammed.sa@...il.com>,
	Simona Vetter <simona@...ll.ch>,
	Melissa Wen <melissa.srw@...il.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v3 8/8] drm/vkms: convert to use faux_device

On 08/02/25 - 08:12, Greg Kroah-Hartman wrote:
> On Fri, Feb 07, 2025 at 05:59:04PM +0100, Louis Chauvet wrote:
> > On 06/02/25 - 18:38, Greg Kroah-Hartman wrote:
> > > The vkms driver does not need to create a platform device, as there is
> > > no real platform resources associated it,  it only did so because it was
> > > simple to do that in order to get a device to use for resource
> > > management of drm resources.  Change the driver to use the faux device
> > > instead as this is NOT a real platform device.
> > > 
> > > Cc: Louis Chauvet <louis.chauvet@...tlin.com>
> > > Cc: Haneen Mohammed <hamohammed.sa@...il.com>
> > > Cc: Simona Vetter <simona@...ll.ch>
> > > Cc: Melissa Wen <melissa.srw@...il.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> > > Cc: Maxime Ripard <mripard@...nel.org>
> > > Cc: Thomas Zimmermann <tzimmermann@...e.de>
> > > Cc: David Airlie <airlied@...il.com>
> > > Cc: dri-devel@...ts.freedesktop.org
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > > ---
> > >  v3: new patch in the series.  For an example of the api working, does
> > >      not have to be merged at this time, but I can take it if the
> > >      maintainers give an ack.
> > 
> > Hi,
> > 
> > This patch cannot be merged into drm-misc-next because we modified the 
> > vkms_device structure in commit 49a167c393b0 ("drm/vkms: Switch to dynamic 
> > allocation for CRTC"), which is present in linux-next.
> > 
> > Once this conflict is resolved, I agree with changing from platform_device 
> > to faux_device.
> > 
> > Apart from this minor conflict, I believe your patch has revealed an issue 
> > in VKMS:
> > 
> > Without your patch:
> > 
> > 	bash-5.2# modprobe vkms
> > 	[drm] Initialized vkms 1.0.0 for vkms on minor 0
> > 	bash-5.2#
> > 
> > With your patch:
> > 
> > 	bash-5.2# modprobe vkms
> > 	faux vkms: Resources present before probing
> > 	[drm] Initialized vkms 1.0.0 for vkms on minor 0
> > 	bash-5.2#
> > 
> > After some investigation, I found that the issue is not caused by your 
> > patch but by VKMS itself:
> > 
> > During faux_device_create, the device core postpones the device probe to 
> > run it later [0]. This probe checks if the devres list is empty [1] and 
> > fails if it is not.
> > 
> > [0]:https://elixir.bootlin.com/linux/v6.13.1/source/drivers/base/bus.c#L534
> > [1]:https://elixir.bootlin.com/linux/v6.13.1/source/drivers/base/dd.c#L626
> > 
> > With a platform driver, the order of execution was:
> > 
> > 	platform_device_register_simple();
> > 		device_add();
> > 	*async* device_probe(); /* no issue, the devres is untouched */
> > 	devres_open_group();
> > 
> > But with faux-device, the order is:
> > 
> > 	faux_device_create();
> > 		device_add();
> > 	devres_open_group();
> > 	*async* device_probe(); /* issue here, because of the previous 
> > 				   devres_open_group */
> 
> Wait, what?  It shouuld be the exact same codepath, as faux_device() is
> not doing anything different from platform here.  You might just be
> hitting a race condition as the async probing is the same here.

Yes, this is the same codepath, and this is a race condition. VKMS was 
just lucky it never happend before. 

> > How do you think this should be solved? I would like to keep a simple 
> > solution, given that:
> > - we want to have multiple vkms devices (configfs [2])
> > - we need to ensure that device_probe is called before devres_open_group 
> >   and devm_drm_dev_alloc to avoid this error
> 
> How about we take out the async probe?  You are getting lucky that it's
> not hit on the platform device code today.  Faux really doesn't need
> async, I was just trying to make the system work the same way that
> platform devices did.

I think this should be sufficient, and allows for a very simple interface: 
once faux_device_create returns, you can use the device "as-is", no 
need to wait for the probe.

What change can I do to disable async probe and test?

Thanks,
Louis Chauvet

> And as for the merge issue, not a problem, I just did this conversion
> for people to see how this works and ideally test it, as you did, to
> find issues!
> 
> thanks,
> 
> greg k-h