On Thu, Feb 20, 2025 at 07:26:41PM +0100, Théo Lebrun wrote: > 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. Then that same driver should have created a prettier name when it created the device and sent it to the driver core :) > 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; What? No, this is a platform driver thing, not a driver core thing. Let's just remove the name pointer in the platform driver structure and then we can handle the rest from there. > + > 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. out-of-tree drivers don't matter to us :) > [...] > > > > ⟩ # 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 True, that was nice, but we shouldn't let one force bugs in the other :) Anyway try removing the name pointer and let's see what falls out. thanks, greg k-h
