* [PATCH v1] iio: trigger: warn about non-registered iio trigger getting attempt @ 2022-05-31 18:15 Dmitry Rokosov 2022-05-31 18:57 ` Dmitry Rokosov 0 siblings, 1 reply; 6+ messages in thread From: Dmitry Rokosov @ 2022-05-31 18:15 UTC (permalink / raw) To: robh+dt, jic23, lars, andy.shevchenko Cc: linux-iio, kernel, linux-kernel, Dmitry Rokosov As a part of patch series about wrong trigger register() and get() calls order in the some IIO drivers trigger initialization path: https://lore.kernel.org/all/20220524181150.9240-1-ddrokosov@sberdevices.ru/ runtime WARN() is added to alarm IIO driver authors who make such a mistake. When IIO driver allocates a new IIO trigger, it should register it before calling get() operation. Otherwise, the next iio_trigger_put() will upset refcnt balance. Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> --- include/linux/iio/trigger.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h index 4c69b144677b..4a008b952710 100644 --- a/include/linux/iio/trigger.h +++ b/include/linux/iio/trigger.h @@ -93,6 +93,15 @@ static inline void iio_trigger_put(struct iio_trigger *trig) static inline struct iio_trigger *iio_trigger_get(struct iio_trigger *trig) { get_device(&trig->dev); + + /* + * If driver hasn't called iio_trigger_register() before and trig->owner + * wasn't initialized properly, trigger will have wrong number of users + */ + WARN(!trig->owner, + "Ignore module getting for non-registered iio trigger %s\n", + trig->name); + __module_get(trig->owner); return trig; -- 2.36.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1] iio: trigger: warn about non-registered iio trigger getting attempt 2022-05-31 18:15 [PATCH v1] iio: trigger: warn about non-registered iio trigger getting attempt Dmitry Rokosov @ 2022-05-31 18:57 ` Dmitry Rokosov 2022-06-01 8:47 ` Nuno Sá 0 siblings, 1 reply; 6+ messages in thread From: Dmitry Rokosov @ 2022-05-31 18:57 UTC (permalink / raw) To: robh+dt, jic23, lars, andy.shevchenko; +Cc: linux-iio, kernel, linux-kernel Hi Jonathan, I have one question about a cases when trigger owner is builtin module. In the such cases trig->owner == null, because THIS_MODULE equals to null. How do you think, should we take into account such situations? IMHO we have to take in and save this information to trig_info during trigger allocation call. For example we can check THIS_MODULE from the iio_trigger_alloc(), save builtin status to trig_info and look into it from iio_trigger_get(). On Tue, May 31, 2022 at 06:15:05PM +0000, Dmitry Rokosov wrote: > As a part of patch series about wrong trigger register() and get() > calls order in the some IIO drivers trigger initialization path: > > https://lore.kernel.org/all/20220524181150.9240-1-ddrokosov@sberdevices.ru/ > > runtime WARN() is added to alarm IIO driver authors who make such > a mistake. > > When IIO driver allocates a new IIO trigger, it should register it before > calling get() operation. Otherwise, the next iio_trigger_put() will upset > refcnt balance. > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > --- > include/linux/iio/trigger.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h > index 4c69b144677b..4a008b952710 100644 > --- a/include/linux/iio/trigger.h > +++ b/include/linux/iio/trigger.h > @@ -93,6 +93,15 @@ static inline void iio_trigger_put(struct iio_trigger *trig) > static inline struct iio_trigger *iio_trigger_get(struct iio_trigger *trig) > { > get_device(&trig->dev); > + > + /* > + * If driver hasn't called iio_trigger_register() before and trig->owner > + * wasn't initialized properly, trigger will have wrong number of users > + */ > + WARN(!trig->owner, > + "Ignore module getting for non-registered iio trigger %s\n", > + trig->name); > + > __module_get(trig->owner); > > return trig; > -- > 2.36.0 -- Thank you, Dmitry ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] iio: trigger: warn about non-registered iio trigger getting attempt 2022-05-31 18:57 ` Dmitry Rokosov @ 2022-06-01 8:47 ` Nuno Sá 2022-06-01 10:33 ` Dmitry Rokosov 0 siblings, 1 reply; 6+ messages in thread From: Nuno Sá @ 2022-06-01 8:47 UTC (permalink / raw) To: Dmitry Rokosov, robh+dt, jic23, lars, andy.shevchenko Cc: linux-iio, kernel, linux-kernel On Tue, 2022-05-31 at 18:57 +0000, Dmitry Rokosov wrote: > Hi Jonathan, > > I have one question about a cases when trigger owner is builtin > module. > In the such cases trig->owner == null, because THIS_MODULE equals to > null. How do you think, should we take into account such situations? > > IMHO we have to take in and save this information to trig_info during > trigger allocation call. For example we can check THIS_MODULE from > the Hmmm, If we were to do something during iio_trigger_alloc(), we would rather assign already THIS_MODULE to owner and we would not need this WARN(). I mean, if someone calls iio_trigger_get() before allocating it, it will have bigger problems :). I think this could actually be something reasonable... - Nuno Sá > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] iio: trigger: warn about non-registered iio trigger getting attempt 2022-06-01 8:47 ` Nuno Sá @ 2022-06-01 10:33 ` Dmitry Rokosov 2022-06-01 13:09 ` Nuno Sá 0 siblings, 1 reply; 6+ messages in thread From: Dmitry Rokosov @ 2022-06-01 10:33 UTC (permalink / raw) To: Nuno Sá Cc: robh+dt, jic23, lars, andy.shevchenko, linux-iio, kernel, linux-kernel Hi Nuno, Thank you for comments! On Wed, Jun 01, 2022 at 10:47:54AM +0200, Nuno Sá wrote: > On Tue, 2022-05-31 at 18:57 +0000, Dmitry Rokosov wrote: > > Hi Jonathan, > > > > I have one question about a cases when trigger owner is builtin > > module. > > In the such cases trig->owner == null, because THIS_MODULE equals to > > null. How do you think, should we take into account such situations? > > > > IMHO we have to take in and save this information to trig_info during > > trigger allocation call. For example we can check THIS_MODULE from > > the > > Hmmm, If we were to do something during iio_trigger_alloc(), we would > rather assign already THIS_MODULE to owner and we would not need this > WARN(). I mean, if someone calls iio_trigger_get() before allocating > it, it will have bigger problems :). > You are right, non-allocated pointer dereference is much bigger problem :) > I think this could actually be something reasonable... I think it could be a good solution, but it's required a lot of changes in the IIO drivers code, because if we assign trig->owner from iio_trigger_alloc(), we do not need this_mod parameter in the iio_trigger_register() iface and its wrappers. So it means to make it workable we must: - rework iio_trigger_alloc() - redesign iio_trigger_register() iface and its wrappers - correct iio_trigger_register() call from all IIO drivers I suppose we need to wait for Jonathan's comments here... -- Thank you, Dmitry ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] iio: trigger: warn about non-registered iio trigger getting attempt 2022-06-01 10:33 ` Dmitry Rokosov @ 2022-06-01 13:09 ` Nuno Sá 2022-06-01 18:03 ` Dmitry Rokosov 0 siblings, 1 reply; 6+ messages in thread From: Nuno Sá @ 2022-06-01 13:09 UTC (permalink / raw) To: Dmitry Rokosov Cc: robh+dt, jic23, lars, andy.shevchenko, linux-iio, kernel, linux-kernel On Wed, 2022-06-01 at 10:33 +0000, Dmitry Rokosov wrote: > Hi Nuno, > > Thank you for comments! > > On Wed, Jun 01, 2022 at 10:47:54AM +0200, Nuno Sá wrote: > > On Tue, 2022-05-31 at 18:57 +0000, Dmitry Rokosov wrote: > > > Hi Jonathan, > > > > > > I have one question about a cases when trigger owner is builtin > > > module. > > > In the such cases trig->owner == null, because THIS_MODULE equals > > > to > > > null. How do you think, should we take into account such > > > situations? > > > > > > IMHO we have to take in and save this information to trig_info > > > during > > > trigger allocation call. For example we can check THIS_MODULE > > > from > > > the > > > > Hmmm, If we were to do something during iio_trigger_alloc(), we > > would > > rather assign already THIS_MODULE to owner and we would not need > > this > > WARN(). I mean, if someone calls iio_trigger_get() before > > allocating > > it, it will have bigger problems :). > > > > You are right, non-allocated pointer dereference is much bigger > problem :) > > > I think this could actually be something reasonable... > > I think it could be a good solution, but it's required a lot of > changes > in the IIO drivers code, because if we assign trig->owner from > iio_trigger_alloc(), we do not need this_mod parameter in the > iio_trigger_register() iface and its wrappers. > So it means to make it workable we must: > - rework iio_trigger_alloc() > - redesign iio_trigger_register() iface and its wrappers > - correct iio_trigger_register() call from all IIO drivers > > I suppose we need to wait for Jonathan's comments here... > I think we could actually get this done without having to change all the drivers. Note on how iio_trigger_register() passes THIS_MODULE to the internal API. We could also use macros in the alloc function in a way that only internal functions would need to be changed. But it all depends on whether or not Jonathan wants this moved... - Nuno Sá ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] iio: trigger: warn about non-registered iio trigger getting attempt 2022-06-01 13:09 ` Nuno Sá @ 2022-06-01 18:03 ` Dmitry Rokosov 0 siblings, 0 replies; 6+ messages in thread From: Dmitry Rokosov @ 2022-06-01 18:03 UTC (permalink / raw) To: Nuno Sá, jic23 Cc: robh+dt, lars, andy.shevchenko, linux-iio, kernel, linux-kernel Jonathan, Nuno, I've sent RFC patch with trig->owner pointer initialization moval from register() to allocate() stage as Nuno suggested before: https://lore.kernel.org/linux-iio/20220601174837.20292-1-ddrokosov@sberdevices.ru/ Please review if possible and share your thoughts. On Wed, Jun 01, 2022 at 03:09:03PM +0200, Nuno Sá wrote: > On Wed, 2022-06-01 at 10:33 +0000, Dmitry Rokosov wrote: > > Hi Nuno, > > > > Thank you for comments! > > > > On Wed, Jun 01, 2022 at 10:47:54AM +0200, Nuno Sá wrote: > > > On Tue, 2022-05-31 at 18:57 +0000, Dmitry Rokosov wrote: > > > > Hi Jonathan, > > > > > > > > I have one question about a cases when trigger owner is builtin > > > > module. > > > > In the such cases trig->owner == null, because THIS_MODULE equals > > > > to > > > > null. How do you think, should we take into account such > > > > situations? > > > > > > > > IMHO we have to take in and save this information to trig_info > > > > during > > > > trigger allocation call. For example we can check THIS_MODULE > > > > from > > > > the > > > > > > Hmmm, If we were to do something during iio_trigger_alloc(), we > > > would > > > rather assign already THIS_MODULE to owner and we would not need > > > this > > > WARN(). I mean, if someone calls iio_trigger_get() before > > > allocating > > > it, it will have bigger problems :). > > > > > > > You are right, non-allocated pointer dereference is much bigger > > problem :) > > > > > I think this could actually be something reasonable... > > > > I think it could be a good solution, but it's required a lot of > > changes > > in the IIO drivers code, because if we assign trig->owner from > > iio_trigger_alloc(), we do not need this_mod parameter in the > > iio_trigger_register() iface and its wrappers. > > So it means to make it workable we must: > > - rework iio_trigger_alloc() > > - redesign iio_trigger_register() iface and its wrappers > > - correct iio_trigger_register() call from all IIO drivers > > > > I suppose we need to wait for Jonathan's comments here... > > > > I think we could actually get this done without having to change all > the drivers. Note on how iio_trigger_register() passes THIS_MODULE to > the internal API. We could also use macros in the alloc function in a > way that only internal functions would need to be changed. But it all > depends on whether or not Jonathan wants this moved... > > - Nuno Sá -- Thank you, Dmitry ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-01 18:03 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-31 18:15 [PATCH v1] iio: trigger: warn about non-registered iio trigger getting attempt Dmitry Rokosov 2022-05-31 18:57 ` Dmitry Rokosov 2022-06-01 8:47 ` Nuno Sá 2022-06-01 10:33 ` Dmitry Rokosov 2022-06-01 13:09 ` Nuno Sá 2022-06-01 18:03 ` Dmitry Rokosov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).