| Message ID | 20180703234705.227473-8-mka@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi, I didn't see any framework which exporting the class instance. It is very dangerous. Unknown device drivers is able to reset the 'devfreq_class' instance. I can't agree this approach. Regards, Chanwoo Choi On 2018년 07월 04일 08:47, Matthias Kaehlcke wrote: > Exporting the device class allows other parts of the kernel to enumerate > the devfreq devices and receive notification when a devfreq device is > added or removed. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > Reviewed-by: Brian Norris <briannorris@chromium.org> > --- > Changes in v5: > - none > > Changes in v4: > - added 'Reviewed-by: Brian Norris <briannorris@chromium.org>' tag > > Changes in v3: > - none > > Changes in v2: > - patch added to series > --- > drivers/devfreq/devfreq.c | 3 ++- > include/linux/devfreq.h | 2 ++ > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 4cbaa7ad1972..38b90b64fc6e 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -31,7 +31,8 @@ > #define MAX(a,b) ((a > b) ? a : b) > #define MIN(a,b) ((a < b) ? a : b) > > -static struct class *devfreq_class; > +struct class *devfreq_class; > +EXPORT_SYMBOL_GPL(devfreq_class); > > /* > * devfreq core provides delayed work based load monitoring helper > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index c4f84a769cb5..964e064a951f 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -206,6 +206,8 @@ struct devfreq_freqs { > }; > > #if defined(CONFIG_PM_DEVFREQ) > +extern struct class *devfreq_class; > + > extern struct devfreq *devfreq_add_device(struct device *dev, > struct devfreq_dev_profile *profile, > const char *governor_name, >
Hi, On Wed, Jul 04, 2018 at 02:30:32PM +0900, Chanwoo Choi wrote: > I didn't see any framework which exporting the class instance. > It is very dangerous. Unknown device drivers is able to reset > the 'devfreq_class' instance. I can't agree this approach. While I agree that it is potential dangerous it is actually a common practice to export the class: grep "extern struct class " include/linux/ -R include/linux/rio.h:extern struct class rio_mport_class; include/linux/tty.h:extern struct class *tty_class; include/linux/fb.h:extern struct class *fb_class; include/linux/ide.h:extern struct class *ide_port_class; include/linux/device.h:extern struct class * __must_check __class_create(struct module *owner, include/linux/devfreq.h:extern struct class *devfreq_class; include/linux/switchtec.h:extern struct class *switchtec_class; include/linux/input.h:extern struct class input_class; include/linux/genhd.h:extern struct class block_class; include/linux/power_supply.h:extern struct class *power_supply_class; include/linux/rtc.h:extern struct class *rtc_class; struct class_interface and class_interface_register() would be pointless without exported classes. My understanding is that the kernel is often lax on encapsulation and exposes private/delicate data pragmatically within the kernel when needed because "the kernel trusts itself". Thanks Matthias
Hi Matthias, On 2018년 07월 07일 03:09, Matthias Kaehlcke wrote: > Hi, > > On Wed, Jul 04, 2018 at 02:30:32PM +0900, Chanwoo Choi wrote: > >> I didn't see any framework which exporting the class instance. >> It is very dangerous. Unknown device drivers is able to reset >> the 'devfreq_class' instance. I can't agree this approach. > > While I agree that it is potential dangerous it is actually a common > practice to export the class: > I tried to find the real usage of exported class instance and I add the comment for each class instance. Almost exported class instance are used in the their own director or some exported class like rio_mport_class/switchtec_class are created from specific device driver instead of subsystem. Only following two cases are used on outside of subsystem directory. devtmpfs.c and alarmtimer.c are core feature of linux kernel. drivers/base/devtmpfs.c uses 'block_class'. kernel/time/alarmtimer.c uses 'rtc_class'. I cannot yet agree this approach due to only block_class and rtc_class. You added the following comment why devfreq_class instance is necessary. Actullay, I don't know the best solution right now. But, all device drivers can be added or removed if driver uses the module type. It is not a problem for only devfreq instance. /* + * devfreq devices can be added and removed at runtime, hence they + * must also be handled dynamically. The class_interface notifies us + * whenever a device is added or removed. When the interface is + * registered ci->add_dev() is called for all existing devfreq + * devices. */ > grep "extern struct class " include/linux/ -R > include/linux/rio.h:extern struct class rio_mport_class; rio_mport_class is created on drivers/rapidio/rio-drivers.c. It means that just device driver create the 'rio_mport_class' class instead of any linux kernel subsystem. > include/linux/tty.h:extern struct class *tty_class; tty_class is not used on outside of drivers/tty > include/linux/fb.h:extern struct class *fb_class; fb_class is not used on outside of drivers/video/fbdev > include/linux/ide.h:extern struct class *ide_port_class; ide_port_class is not used on outside of drivers/ide. > include/linux/device.h:extern struct class * __must_check __class_create(struct module *owner, > include/linux/devfreq.h:extern struct class *devfreq_class; not yet > include/linux/switchtec.h:extern struct class *switchtec_class; switchtec_class is created on drivers/pci/switch/switchtec.c and then switchtec_class is only used on drivers/ntb/hw/mscc/ntb_hw_switchtec.c. It is not subsystem. Just switchtec.c device driver makes the their own class. > include/linux/input.h:extern struct class input_class; input_class is not used on outside of drivers/input. > include/linux/power_supply.h:extern struct class *power_supply_class; power_supply_class is not used on outside of drivers/power/supply. > include/linux/genhd.h:extern struct class block_class; drivers/base/devtmpfs.c uses 'block_class'. > include/linux/rtc.h:extern struct class *rtc_class; kernel/time/alarmtimer.c uses 'rtc_class'. > > struct class_interface and class_interface_register() would be > pointless without exported classes. > > My understanding is that the kernel is often lax on encapsulation and > exposes private/delicate data pragmatically within the kernel when > needed because "the kernel trusts itself". > > Thanks > > Matthias > > >
Hi Chanwoo, On Thu, Jul 12, 2018 at 06:08:36PM +0900, Chanwoo Choi wrote: > Hi Matthias, > > On 2018년 07월 07일 03:09, Matthias Kaehlcke wrote: > > Hi, > > > > On Wed, Jul 04, 2018 at 02:30:32PM +0900, Chanwoo Choi wrote: > > > >> I didn't see any framework which exporting the class instance. > >> It is very dangerous. Unknown device drivers is able to reset > >> the 'devfreq_class' instance. I can't agree this approach. > > > > While I agree that it is potential dangerous it is actually a common > > practice to export the class: > > > > I tried to find the real usage of exported class instance > and I add the comment for each class instance. Almost exported class > instance are used in the their own director or some exported class > like rio_mport_class/switchtec_class are created from specific device driver > instead of subsystem. > > Only following two cases are used on outside of subsystem directory. > devtmpfs.c and alarmtimer.c are core feature of linux kernel. > > drivers/base/devtmpfs.c uses 'block_class'. > kernel/time/alarmtimer.c uses 'rtc_class'. > > I cannot yet agree this approach due to only block_class and rtc_class. I thought your main concern was that the class is exported, which is what several other subsystems do. That the class isn't used outside of the subsystem directory most likely means that there is no need for it, rather than that it shouldn't be done at all (depending on the type of use of course). In any case not exporting the class object provides a limited protection against potential abuse of the class at best. To use the class API all that is needed is a 'struct device' of a devfreq device, which has a pointer to the class object (dev->class). Theoretically I could register a fake devfreq device to obtain access to the class object, though that doesn't seem a very neat approach ;-) > You added the following comment why devfreq_class instance is necessary. > Actullay, I don't know the best solution right now. But, all device drivers > can be added or removed if driver uses the module type. It is not a problem > for only devfreq instance. Certainly it's not a problem limited to devfreq devices. In many other cases bus notifiers can be used, but since devfreq devices areen't tied to a specific bus this is not an option here. If you really don't want to export the class we could add wrappers for (un)registering a class interface: int devfreq_class_interface_register(struct class_interface *) void devfreq_class_interface_unregister(struct class_interface *) The wrappers would have to assign ci->class since the throttler can't see the class object. Or add notifiers for device addition/removal, though the throttler relies on the behavior of the class_interface which also notifies about devices added before registration. This might not be what other potential users of the notifiers expect. Thanks Matthias > /* > + * devfreq devices can be added and removed at runtime, hence they > + * must also be handled dynamically. The class_interface notifies us > + * whenever a device is added or removed. When the interface is > + * registered ci->add_dev() is called for all existing devfreq > + * devices. > */ > > > > grep "extern struct class " include/linux/ -R > > include/linux/rio.h:extern struct class rio_mport_class; > rio_mport_class is created on drivers/rapidio/rio-drivers.c. > It means that just device driver create the 'rio_mport_class' class > instead of any linux kernel subsystem. > > > include/linux/tty.h:extern struct class *tty_class; > tty_class is not used on outside of drivers/tty > > > include/linux/fb.h:extern struct class *fb_class; > fb_class is not used on outside of drivers/video/fbdev > > > include/linux/ide.h:extern struct class *ide_port_class; > ide_port_class is not used on outside of drivers/ide. > > > include/linux/device.h:extern struct class * __must_check __class_create(struct module *owner, > > > include/linux/devfreq.h:extern struct class *devfreq_class; > not yet > > > include/linux/switchtec.h:extern struct class *switchtec_class; > switchtec_class is created on drivers/pci/switch/switchtec.c > and then switchtec_class is only used on drivers/ntb/hw/mscc/ntb_hw_switchtec.c. > It is not subsystem. Just switchtec.c device driver makes the their own class. > > > include/linux/input.h:extern struct class input_class; > input_class is not used on outside of drivers/input. > > > include/linux/power_supply.h:extern struct class *power_supply_class; > power_supply_class is not used on outside of drivers/power/supply. > > > > include/linux/genhd.h:extern struct class block_class; > drivers/base/devtmpfs.c uses 'block_class'. > > > include/linux/rtc.h:extern struct class *rtc_class; > kernel/time/alarmtimer.c uses 'rtc_class'. > > > > > struct class_interface and class_interface_register() would be > > pointless without exported classes. > > > > My understanding is that the kernel is often lax on encapsulation and > > exposes private/delicate data pragmatically within the kernel when > > needed because "the kernel trusts itself".
On Mon, Jul 16, 2018 at 12:41:14PM -0700, Matthias Kaehlcke wrote: > Hi Chanwoo, > > On Thu, Jul 12, 2018 at 06:08:36PM +0900, Chanwoo Choi wrote: > > Hi Matthias, > > > > On 2018년 07월 07일 03:09, Matthias Kaehlcke wrote: > > > Hi, > > > > > > On Wed, Jul 04, 2018 at 02:30:32PM +0900, Chanwoo Choi wrote: > > > > > >> I didn't see any framework which exporting the class instance. > > >> It is very dangerous. Unknown device drivers is able to reset > > >> the 'devfreq_class' instance. I can't agree this approach. > > > > > > While I agree that it is potential dangerous it is actually a common > > > practice to export the class: > > > > > > > I tried to find the real usage of exported class instance > > and I add the comment for each class instance. Almost exported class > > instance are used in the their own director or some exported class > > like rio_mport_class/switchtec_class are created from specific device driver > > instead of subsystem. > > > > Only following two cases are used on outside of subsystem directory. > > devtmpfs.c and alarmtimer.c are core feature of linux kernel. > > > > drivers/base/devtmpfs.c uses 'block_class'. > > kernel/time/alarmtimer.c uses 'rtc_class'. > > > > I cannot yet agree this approach due to only block_class and rtc_class. > > I thought your main concern was that the class is exported, which is > what several other subsystems do. That the class isn't used outside of > the subsystem directory most likely means that there is no need for > it, rather than that it shouldn't be done at all (depending on the > type of use of course). > > In any case not exporting the class object provides a limited > protection against potential abuse of the class at best. To use the > class API all that is needed is a 'struct device' of a devfreq device, > which has a pointer to the class object (dev->class). > > Theoretically I could register a fake devfreq device to obtain access > to the class object, though that doesn't seem a very neat approach ;-) > > > You added the following comment why devfreq_class instance is necessary. > > Actullay, I don't know the best solution right now. But, all device drivers > > can be added or removed if driver uses the module type. It is not a problem > > for only devfreq instance. > > Certainly it's not a problem limited to devfreq devices. In many other > cases bus notifiers can be used, but since devfreq devices areen't > tied to a specific bus this is not an option here. > > If you really don't want to export the class we could add wrappers > for (un)registering a class interface: > > int devfreq_class_interface_register(struct class_interface *) > void devfreq_class_interface_unregister(struct class_interface *) > > The wrappers would have to assign ci->class since the throttler > can't see the class object. > > Or add notifiers for device addition/removal, though the throttler > relies on the behavior of the class_interface which also notifies > about devices added before registration. This might not be what other > potential users of the notifiers expect. Ping Could we please try to find a solution/reach a conclusion for this? Not that it should affect the outcome of this discussion, but I want to mention that from my point of view it is a bit unfortunate that this and other fundamental concerns were only raised after I spent significant time on repeatedly refactoring the throttler driver to address other comments. Since you and MyungJoo Ham previously had only minor comments on the other devfreq patches in this series I assumed there were no major concerns from your side :(
Hi Matthias, On 2018년 08월 01일 04:29, Matthias Kaehlcke wrote: > On Mon, Jul 16, 2018 at 12:41:14PM -0700, Matthias Kaehlcke wrote: >> Hi Chanwoo, >> >> On Thu, Jul 12, 2018 at 06:08:36PM +0900, Chanwoo Choi wrote: >>> Hi Matthias, >>> >>> On 2018년 07월 07일 03:09, Matthias Kaehlcke wrote: >>>> Hi, >>>> >>>> On Wed, Jul 04, 2018 at 02:30:32PM +0900, Chanwoo Choi wrote: >>>> >>>>> I didn't see any framework which exporting the class instance. >>>>> It is very dangerous. Unknown device drivers is able to reset >>>>> the 'devfreq_class' instance. I can't agree this approach. >>>> >>>> While I agree that it is potential dangerous it is actually a common >>>> practice to export the class: >>>> >>> >>> I tried to find the real usage of exported class instance >>> and I add the comment for each class instance. Almost exported class >>> instance are used in the their own director or some exported class >>> like rio_mport_class/switchtec_class are created from specific device driver >>> instead of subsystem. >>> >>> Only following two cases are used on outside of subsystem directory. >>> devtmpfs.c and alarmtimer.c are core feature of linux kernel. >>> >>> drivers/base/devtmpfs.c uses 'block_class'. >>> kernel/time/alarmtimer.c uses 'rtc_class'. >>> >>> I cannot yet agree this approach due to only block_class and rtc_class. >> >> I thought your main concern was that the class is exported, which is >> what several other subsystems do. That the class isn't used outside of >> the subsystem directory most likely means that there is no need for >> it, rather than that it shouldn't be done at all (depending on the >> type of use of course). >> >> In any case not exporting the class object provides a limited >> protection against potential abuse of the class at best. To use the >> class API all that is needed is a 'struct device' of a devfreq device, >> which has a pointer to the class object (dev->class). >> >> Theoretically I could register a fake devfreq device to obtain access >> to the class object, though that doesn't seem a very neat approach ;-) >> >>> You added the following comment why devfreq_class instance is necessary. >>> Actullay, I don't know the best solution right now. But, all device drivers >>> can be added or removed if driver uses the module type. It is not a problem >>> for only devfreq instance. >> >> Certainly it's not a problem limited to devfreq devices. In many other >> cases bus notifiers can be used, but since devfreq devices areen't >> tied to a specific bus this is not an option here. >> >> If you really don't want to export the class we could add wrappers >> for (un)registering a class interface: >> >> int devfreq_class_interface_register(struct class_interface *) >> void devfreq_class_interface_unregister(struct class_interface *) About this approach, I agree because it doesn't export the devfreq_class instance as you commented. >> >> The wrappers would have to assign ci->class since the throttler >> can't see the class object. >> >> Or add notifiers for device addition/removal, though the throttler >> relies on the behavior of the class_interface which also notifies >> about devices added before registration. This might not be what other >> potential users of the notifiers expect. > > Ping > > Could we please try to find a solution/reach a conclusion for this? > > Not that it should affect the outcome of this discussion, but I want > to mention that from my point of view it is a bit unfortunate that > this and other fundamental concerns were only raised after I spent > significant time on repeatedly refactoring the throttler driver to > address other comments. Since you and MyungJoo Ham previously had only > minor comments on the other devfreq patches in this series I assumed > there were no major concerns from your side :( > >
On Wed, Aug 01, 2018 at 05:18:40PM +0900, Chanwoo Choi wrote: > Hi Matthias, > > On 2018년 08월 01일 04:29, Matthias Kaehlcke wrote: > > On Mon, Jul 16, 2018 at 12:41:14PM -0700, Matthias Kaehlcke wrote: > >> Hi Chanwoo, > >> > >> On Thu, Jul 12, 2018 at 06:08:36PM +0900, Chanwoo Choi wrote: > >>> Hi Matthias, > >>> > >>> On 2018년 07월 07일 03:09, Matthias Kaehlcke wrote: > >>>> Hi, > >>>> > >>>> On Wed, Jul 04, 2018 at 02:30:32PM +0900, Chanwoo Choi wrote: > >>>> > >>>>> I didn't see any framework which exporting the class instance. > >>>>> It is very dangerous. Unknown device drivers is able to reset > >>>>> the 'devfreq_class' instance. I can't agree this approach. > >>>> > >>>> While I agree that it is potential dangerous it is actually a common > >>>> practice to export the class: > >>>> > >>> > >>> I tried to find the real usage of exported class instance > >>> and I add the comment for each class instance. Almost exported class > >>> instance are used in the their own director or some exported class > >>> like rio_mport_class/switchtec_class are created from specific device driver > >>> instead of subsystem. > >>> > >>> Only following two cases are used on outside of subsystem directory. > >>> devtmpfs.c and alarmtimer.c are core feature of linux kernel. > >>> > >>> drivers/base/devtmpfs.c uses 'block_class'. > >>> kernel/time/alarmtimer.c uses 'rtc_class'. > >>> > >>> I cannot yet agree this approach due to only block_class and rtc_class. > >> > >> I thought your main concern was that the class is exported, which is > >> what several other subsystems do. That the class isn't used outside of > >> the subsystem directory most likely means that there is no need for > >> it, rather than that it shouldn't be done at all (depending on the > >> type of use of course). > >> > >> In any case not exporting the class object provides a limited > >> protection against potential abuse of the class at best. To use the > >> class API all that is needed is a 'struct device' of a devfreq device, > >> which has a pointer to the class object (dev->class). > >> > >> Theoretically I could register a fake devfreq device to obtain access > >> to the class object, though that doesn't seem a very neat approach ;-) > >> > >>> You added the following comment why devfreq_class instance is necessary. > >>> Actullay, I don't know the best solution right now. But, all device drivers > >>> can be added or removed if driver uses the module type. It is not a problem > >>> for only devfreq instance. > >> > >> Certainly it's not a problem limited to devfreq devices. In many other > >> cases bus notifiers can be used, but since devfreq devices areen't > >> tied to a specific bus this is not an option here. > >> > >> If you really don't want to export the class we could add wrappers > >> for (un)registering a class interface: > >> > >> int devfreq_class_interface_register(struct class_interface *) > >> void devfreq_class_interface_unregister(struct class_interface *) > > About this approach, I agree because it doesn't export the devfreq_class > instance as you commented. Great, I'll change it in the next revision!
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 4cbaa7ad1972..38b90b64fc6e 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -31,7 +31,8 @@ #define MAX(a,b) ((a > b) ? a : b) #define MIN(a,b) ((a < b) ? a : b) -static struct class *devfreq_class; +struct class *devfreq_class; +EXPORT_SYMBOL_GPL(devfreq_class); /* * devfreq core provides delayed work based load monitoring helper diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index c4f84a769cb5..964e064a951f 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -206,6 +206,8 @@ struct devfreq_freqs { }; #if defined(CONFIG_PM_DEVFREQ) +extern struct class *devfreq_class; + extern struct devfreq *devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile, const char *governor_name,