On Thu Feb 20, 2025 at 5:19 PM CET, Greg Kroah-Hartman wrote: > On Thu, Feb 20, 2025 at 04:46:59PM +0100, Théo Lebrun wrote: >> On Thu Feb 20, 2025 at 3:06 PM CET, Greg Kroah-Hartman wrote: >> > On Thu, Feb 20, 2025 at 02:31:29PM +0100, Théo Lebrun wrote: >> >> On Thu Feb 20, 2025 at 1:41 PM CET, Greg Kroah-Hartman wrote: >> >> > On Tue, Feb 18, 2025 at 12:00:11PM +0100, Théo Lebrun wrote: >> >> >> The solution proposed is to add a flag to platform_device that tells if >> >> >> it is responsible for freeing its name. We can then duplicate the >> >> >> device name inside of_device_add() instead of copying the pointer. >> >> > >> >> > Ick. >> >> > >> >> >> What is done elsewhere? >> >> >> - Platform bus code does a copy of the argument name that is stored >> >> >> alongside the struct platform_device; see platform_device_alloc()[1]. >> >> >> - Other busses duplicate the device name; either through a dynamic >> >> >> allocation [2] or through an array embedded inside devices [3]. >> >> >> - Some busses don't have a separate name; when they want a name they >> >> >> take it from the device [4]. >> >> > >> >> > Really ick. >> >> > >> >> > Let's do the right thing here and just get rid of the name pointer >> >> > entirely in struct platform_device please. Isn't that the correct >> >> > thing that way the driver core logic will work properly for all of this. >> >> >> >> I would agree, if it wasn't for this consideration that is found in the >> >> commit message [0]: >> > >> > What, that the of code is broken? Then it should be fixed, why does it >> > need a pointer to a name at all anyway? It shouldn't be needed there >> > either. >> >> I cannot guess why it originally has a separate pdev->name field. > > Many people got this wrong when we designed busses, it's not unique. > But we should learn from our mistakes where we can :) > >> >> > It is important to duplicate! pdev->name must not change to make sure >> >> > the platform_match() return value is stable over time. If we updated >> >> > pdev->name alongside dev->name, once a device probes and changes its >> >> > name then the platform_match() return value would change. >> >> >> >> I'd be fine sending a V2 that removes the field *and the fallback* [1], >> >> but I don't have the full scope in mind to know what would become broken. >> >> >> >> [0]: https://lore.kernel.org/lkml/20250218-pdev-uaf-v1-2-5ea1a0d3aba0@xxxxxxxxxxx/ >> >> [1]: https://elixir.bootlin.com/linux/v6.13.3/source/drivers/base/platform.c#L1357 >> > >> > The fallback will not need to be removed, properly point to the name of >> > the device and it should work correctly. >> >> No, it will not work correctly, as the above quote indicates. > > I don't know which quote, sorry. > >> Let's assume we remove the field, this situation would be broken: >> - OF allocates platform devices and gives them names. >> - A device matches with a driver, which gets probed. >> - During the probe, driver does a dev_set_name(). >> - Afterwards, the upcoming platform_match() against other drivers are >> called with another device name. >> >> We should be safe as there are guardraids to not probe twice a device, >> see __driver_probe_device() that checks dev->driver is NULL. But it >> isn't a situation we should be in. > > The fragility of attempting to match a driver to a device purely by a > name is a very week part of using platform devices. I never said the opposite, and I agree. However the mechanism exists and I was focused on not breaking it. > Why would a driver change the device name? It's been given to the > driver to "bind to" not to change its name. That shouldn't be ok, fix > those drivers. I do get the argument that devices shouldn't change device names. I'll take the devil's advocate and give at least one argument FOR allowing changing names: prettier names, especially as device names leak into userspace through pseudo filesystems. If we agree that device names shouldn't be changed one a device is matched with a driver, then (1) we can remove the pdev->name field and (2) `dev_set_name()` should warn when used too late. Turn the implicit explicit. diff --git a/drivers/base/core.c b/drivers/base/core.c index 5a1f05198114..3532b068e32d 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3462,10 +3462,13 @@ static void device_remove_class_symlinks(struct device *dev) int dev_set_name(struct device *dev, const char *fmt, ...) { va_list vargs; int err; + if (dev_WARN_ONCE(dev, dev->driver, "device name is static once matched")) + return -EPERM; + va_start(vargs, fmt); err = kobject_set_name_vargs(&dev->kobj, fmt, vargs); va_end(vargs); return err; } (Unsure about the exact error code to return.) [...] > Do we have examples today of platform drivers that like to rename > devices? I did a quick search and couldn't find any in-tree, but I > might have missed some. The cover letter expands on the quest for those drivers: On Tue Feb 18, 2025 at 12:00 PM CET, Théo Lebrun wrote: > Out of the 37 drivers that deal with platform devices and do a > dev_set_name() call, only one might be affected. That driver is > loongson-i2s-plat [0]. All other dev_set_name() calls are on children > devices created on the spot. The issue was found on downstream kernels > and we don't have what it takes to test loongson-i2s-plat. [...] > > ⟩ # Finding potential trouble-makers: > ⟩ git grep -l 'struct platform_device' | xargs grep -l dev_set_name > [...] > [0]: https://elixir.bootlin.com/linux/v6.13.2/source/sound/soc/loongson/loongson_i2s_plat.c#L155 [...] > Or if this really is an issue, let's fix OF to not use the platform bus > and have it's own bus for stuff like this. That used to exist! I cannot see how it could be a good idea to reintroduce the distinction though. commit eca3930163ba8884060ce9d9ff5ef0d9b7c7b00f Author: Grant Likely <grant.likely@xxxxxxxxxxxx> Date: Tue Jun 8 07:48:21 2010 -0600 of: Merge of_platform_bus_type with platform_bus_type Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
