linux-kernel - Re: [PATCH 1/2] devres: add devm_remove_action

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z3vC6MzIG7zN8zBT@phenom.ffwll.local>
Date: Mon, 6 Jan 2025 12:47:52 +0100
From: Simona Vetter <simona.vetter@...ll.ch>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org,
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
	bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 1/2] devres: add devm_remove_action_nowarn()

On Fri, Jan 03, 2025 at 05:44:30PM +0100, Danilo Krummrich wrote:
> devm_remove_action() warns if the action to remove does not exist
> (anymore).
> 
> The Rust devres abstraction, however, has a use-case to call
> devm_remove_action() at a point where it can't be guaranteed that the
> corresponding action hasn't been released yet.
> 
> In particular, an instance of `Devres<T>` may be dropped after the
> action has been released. So far, `Devres<T>` worked around this by
> keeping the inner type alive.
> 
> Hence, add devm_remove_action_nowarn(), which returns an error code if
> the action has been removed already.
> 
> A subsequent patch uses devm_remove_action_nowarn() to remove the action
> when `Devres<T>` is dropped.
> 
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> ---
>  drivers/base/devres.c  | 17 ++++++++++++-----
>  include/linux/device.h | 18 +++++++++++++++++-
>  2 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 2152eec0c135..d59b8078fc33 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -750,25 +750,32 @@ int __devm_add_action(struct device *dev, void (*action)(void *), void *data, co
>  EXPORT_SYMBOL_GPL(__devm_add_action);
>  
>  /**
> - * devm_remove_action() - removes previously added custom action
> + * devm_remove_action_nowarn() - removes previously added custom action
>   * @dev: Device that owns the action
>   * @action: Function implementing the action
>   * @data: Pointer to data passed to @action implementation
>   *
>   * Removes instance of @action previously added by devm_add_action().
>   * Both action and data should match one of the existing entries.
> + *
> + * In contrast to devm_remove_action(), this function does not WARN() if no
> + * entry could have been found.

I'd put a caution here that most likely, using this is a bad idea. Maybe
something like:

"This should only be used if the action is contained in an object with
independent lifetime management, like the Devres rust abstraction.
Anywhere is the warning most likely indicates a driver bug."

At least I really can't come up with a reasonable design in a C driver
that would ever need this.

Cheers, Sima

> + *
> + * Returns: 0 on success, -ENOENT if no entry could have been found.
>   */
> -void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
> +int devm_remove_action_nowarn(struct device *dev,
> +			      void (*action)(void *),
> +			      void *data)
>  {
>  	struct action_devres devres = {
>  		.data = data,
>  		.action = action,
>  	};
>  
> -	WARN_ON(devres_destroy(dev, devm_action_release, devm_action_match,
> -			       &devres));
> +	return devres_destroy(dev, devm_action_release, devm_action_match,
> +			      &devres);
>  }
> -EXPORT_SYMBOL_GPL(devm_remove_action);
> +EXPORT_SYMBOL_GPL(devm_remove_action_nowarn);
>  
>  /**
>   * devm_release_action() - release previously added custom action
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 667cb6db9019..6879d5e8ac3d 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -399,7 +399,23 @@ void __iomem *devm_of_iomap(struct device *dev,
>  #endif
>  
>  /* allows to add/remove a custom action to devres stack */
> -void devm_remove_action(struct device *dev, void (*action)(void *), void *data);
> +int devm_remove_action_nowarn(struct device *dev, void (*action)(void *), void *data);
> +
> +/**
> + * devm_remove_action() - removes previously added custom action
> + * @dev: Device that owns the action
> + * @action: Function implementing the action
> + * @data: Pointer to data passed to @action implementation
> + *
> + * Removes instance of @action previously added by devm_add_action().
> + * Both action and data should match one of the existing entries.
> + */
> +static inline
> +void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
> +{
> +	WARN_ON(devm_remove_action_nowarn(dev, action, data));
> +}
> +
>  void devm_release_action(struct device *dev, void (*action)(void *), void *data);
>  
>  int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name);
> 
> base-commit: 06e843bbbf2107463249ea6f6b1a736f5647e24a
> -- 
> 2.47.1
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch