| Message ID | 155727336530.292046.2926860263201336366.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 2374b682255184d7ef75fcb507ce5af4995ead32 |
| Headers | show |
| Series | mm/devm_memremap_pages: Fix page release race | expand |
On Tue, May 07, 2019 at 04:56:05PM -0700, Dan Williams wrote: > The devm_add_action() facility allows a resource allocation routine to > add custom devm semantics. One such user is devm_memremap_pages(). > > There is now a need to manually trigger devm_memremap_pages_release(). > Introduce devm_release_action() so the release action can be triggered > via a new devm_memunmap_pages() api in a follow-on change. > > Cc: Logan Gunthorpe <logang@deltatee.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/base/devres.c | 24 +++++++++++++++++++++++- > include/linux/device.h | 1 + > 2 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > index e038e2b3b7ea..0bbb328bd17f 100644 > --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -755,10 +755,32 @@ void devm_remove_action(struct device *dev, void (*action)(void *), void *data) > > WARN_ON(devres_destroy(dev, devm_action_release, devm_action_match, > &devres)); > - > } > EXPORT_SYMBOL_GPL(devm_remove_action); > > +/** > + * devm_release_action() - release previously added custom action > + * @dev: Device that owns the action > + * @action: Function implementing the action > + * @data: Pointer to data passed to @action implementation > + * > + * Releases and removes instance of @action previously added by > + * devm_add_action(). Both action and data should match one of the > + * existing entries. > + */ > +void devm_release_action(struct device *dev, void (*action)(void *), void *data) > +{ > + struct action_devres devres = { > + .data = data, > + .action = action, > + }; > + > + WARN_ON(devres_release(dev, devm_action_release, devm_action_match, > + &devres)); What does WARN_ON help here? are we going to start getting syzbot reports of this happening? How can this fail? thanks, greg k-h
On Tue, May 14, 2019 at 12:12 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, May 07, 2019 at 04:56:05PM -0700, Dan Williams wrote: > > The devm_add_action() facility allows a resource allocation routine to > > add custom devm semantics. One such user is devm_memremap_pages(). > > > > There is now a need to manually trigger devm_memremap_pages_release(). > > Introduce devm_release_action() so the release action can be triggered > > via a new devm_memunmap_pages() api in a follow-on change. > > > > Cc: Logan Gunthorpe <logang@deltatee.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/base/devres.c | 24 +++++++++++++++++++++++- > > include/linux/device.h | 1 + > > 2 files changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > > index e038e2b3b7ea..0bbb328bd17f 100644 > > --- a/drivers/base/devres.c > > +++ b/drivers/base/devres.c > > @@ -755,10 +755,32 @@ void devm_remove_action(struct device *dev, void (*action)(void *), void *data) > > > > WARN_ON(devres_destroy(dev, devm_action_release, devm_action_match, > > &devres)); > > - > > } > > EXPORT_SYMBOL_GPL(devm_remove_action); > > > > +/** > > + * devm_release_action() - release previously added custom action > > + * @dev: Device that owns the action > > + * @action: Function implementing the action > > + * @data: Pointer to data passed to @action implementation > > + * > > + * Releases and removes instance of @action previously added by > > + * devm_add_action(). Both action and data should match one of the > > + * existing entries. > > + */ > > +void devm_release_action(struct device *dev, void (*action)(void *), void *data) > > +{ > > + struct action_devres devres = { > > + .data = data, > > + .action = action, > > + }; > > + > > + WARN_ON(devres_release(dev, devm_action_release, devm_action_match, > > + &devres)); > > What does WARN_ON help here? are we going to start getting syzbot > reports of this happening? Hopefully, yes, if developers misuse the api they get a loud notification similar to devm_remove_action() misuse. > How can this fail? It's a catch to make sure that @dev actually has a live devres resource that can be found via @action and @data.
diff --git a/drivers/base/devres.c b/drivers/base/devres.c index e038e2b3b7ea..0bbb328bd17f 100644 --- a/drivers/base/devres.c +++ b/drivers/base/devres.c @@ -755,10 +755,32 @@ void devm_remove_action(struct device *dev, void (*action)(void *), void *data) WARN_ON(devres_destroy(dev, devm_action_release, devm_action_match, &devres)); - } EXPORT_SYMBOL_GPL(devm_remove_action); +/** + * devm_release_action() - release previously added custom action + * @dev: Device that owns the action + * @action: Function implementing the action + * @data: Pointer to data passed to @action implementation + * + * Releases and removes instance of @action previously added by + * devm_add_action(). Both action and data should match one of the + * existing entries. + */ +void devm_release_action(struct device *dev, void (*action)(void *), void *data) +{ + struct action_devres devres = { + .data = data, + .action = action, + }; + + WARN_ON(devres_release(dev, devm_action_release, devm_action_match, + &devres)); + +} +EXPORT_SYMBOL_GPL(devm_release_action); + /* * Managed kmalloc/kfree */ diff --git a/include/linux/device.h b/include/linux/device.h index 4e6987e11f68..6d7fd5370f3d 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -713,6 +713,7 @@ void __iomem *devm_of_iomap(struct device *dev, /* allows to add/remove a custom action to devres stack */ int devm_add_action(struct device *dev, void (*action)(void *), void *data); void devm_remove_action(struct device *dev, void (*action)(void *), void *data); +void devm_release_action(struct device *dev, void (*action)(void *), void *data); static inline int devm_add_action_or_reset(struct device *dev, void (*action)(void *), void *data)