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
| ||
|
Date: Tue, 4 Jun 2019 11:13:02 -0500 From: Richard Gong <richard.gong@...ux.intel.com> To: Greg KH <gregkh@...uxfoundation.org>, Romain Izard <romain.izard.pro@...il.com> Cc: robh+dt@...nel.org, mark.rutland@....com, dinguyen@...nel.org, atull@...nel.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, sen.li@...el.com, Richard Gong <richard.gong@...el.com> Subject: Re: A potential broken at platform driver? Hi Greg, On 6/4/19 9:28 AM, Greg KH wrote: > On Tue, Jun 04, 2019 at 12:33:03PM +0200, Romain Izard wrote: >> On Mon, Jun 03, 2019 at 08:02:55PM +0200, Greg KH wrote: >>>> @@ -394,7 +432,7 @@ static struct platform_driver stratix10_rsu_driver = { >>>> .remove = stratix10_rsu_remove, >>>> .driver = { >>>> .name = "stratix10-rsu", >>>> - .groups = rsu_groups, >>>> +// .groups = rsu_groups, >>> >>> Are you sure this is the correct pointer? I think that might be >>> pointing to the driver's attributes, not the device's attributes. >>> >>> If platform drivers do not have a way to register groups properly, then >>> that really needs to be fixed, as trying to register it by yourself as >>> you are doing, is ripe for racing with userspace. >> >> This is a very common issue with platform drivers, and it seems to me that >> it is not possible to add device attributes when binding a device to a >> driver without entering the race condition. >> >> My understanding is the following one: >> >> The root cause is that the device has already been created and reported >> to the userspace with a KOBJ_ADD uevent before the device and the driver >> are bound together. On receiving this event, userspace will react, and >> it will try to read the device's attributes. In parallel the kernel will >> try to find a matching driver. If a driver is found, the kernel will >> call the probe function from the driver with the device as a parameter, >> and if successful a KOBJ_BIND uevent will be sent to userspace, but this >> is a recent addition. >> >> Unfortunately, not all created devices will be bound to a driver, and the >> existing udev code relies on KOBJ_ADD uevents rather than KOBJ_BIND uevents. >> If new per-device attributes have been added to the device during the >> binding stage userspace may or may not see them, depending on when userspace >> tries to read the device's attributes. >> >> I have this possible workaround, but I do not know if it is a good solution: >> >> When binding the device and the driver together, create a new device as a >> child to the current device, and fill its "groups" member to point to the >> per-device attributes' group. As the device will be created with all the >> attributes, it will not be affected by the race issues. The functions >> handling the attributes will need to be modified to use the parents of their >> "device" parameter, instead of the device itself. Additionnaly, the sysfs >> location of the attributes will be different, as the child device will show >> up in the sysfs path. But for a newly introduced device this will not be >> a problem. >> >> Is this a good compromise ? > > Not really. You just want the attributes on the platform device itself. > > Given the horrible hack that platform devices are today, what's one more > hack! > > Here's a patch below of what should probably be done here. Richard, can > you change your code to use the new dev_groups pointer in the struct > platform_driver and this patch and let me know if that works or not? > > Note, I've only compiled this code, not tested it... > Your patch works. Many thanks for your help! Regards, Richard > thanks, > > greg k-h > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 4d1729853d1a..3dd4b73a9b30 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -598,6 +598,7 @@ struct platform_device *platform_device_register_full( > } > EXPORT_SYMBOL_GPL(platform_device_register_full); > > +static int platform_drv_remove(struct device *_dev); > static int platform_drv_probe(struct device *_dev) > { > struct platform_driver *drv = to_platform_driver(_dev->driver); > @@ -614,8 +615,18 @@ static int platform_drv_probe(struct device *_dev) > > if (drv->probe) { > ret = drv->probe(dev); > - if (ret) > + if (ret) { > dev_pm_domain_detach(_dev, true); > + goto out; > + } > + } > + if (drv->dev_groups) { > + ret = device_add_groups(_dev, drv->dev_groups); > + if (ret) { > + platform_drv_remove(_dev); > + return ret; > + } > + kobject_uevent(&_dev->kobj, KOBJ_CHANGE); > } > > out: > @@ -640,6 +651,8 @@ static int platform_drv_remove(struct device *_dev) > > if (drv->remove) > ret = drv->remove(dev); > + if (drv->dev_groups) > + device_remove_groups(_dev, drv->dev_groups); > dev_pm_domain_detach(_dev, true); > > return ret; > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index cc464850b71e..027f1e1d7af8 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -190,6 +190,7 @@ struct platform_driver { > int (*resume)(struct platform_device *); > struct device_driver driver; > const struct platform_device_id *id_table; > + const struct attribute_group **dev_groups; > bool prevent_deferred_probe; > }; > >
Powered by blists - more mailing lists