linux-kernel - Re: [RFC] driver core: Fix repeated device_is

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] [day] [month] [year] [list]

Message-ID: <YsRDKmW1ZSbGN4l0@linaro.org>
Date:   Tue, 5 Jul 2022 16:56:58 +0300
From:   Abel Vesa <abel.vesa@...aro.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Saravana Kannan <saravanak@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org
Subject: Re: [RFC] driver core: Fix repeated device_is_dependent check for
 same link

On 22-07-05 16:45:02, Abel Vesa wrote:
> In case of a cyclic dependency, if the supplier is not yet available,
> the parent of the supplier is checked for dependency. But if there are
> more than one suppliers with the same parent, the first check returns
> true while the next checks skip that specific link entirely. So add a
> flag that marks the link for future checks and bail early if it is
> already marked with that flag.
>
> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
> ---
>
> For more details about this issue, have a look at this thread:
> https://lore.kernel.org/all/CAGETcx8F0wP+RA0KpjOJeZfc=DVG-MbM_=SkRHD4UhD2ReL7Kw@mail.gmail.com/
>
>  drivers/base/core.c    | 14 +++++++++++---
>  include/linux/device.h |  2 ++
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index ccdd5b4295de..38cb478ae400 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -297,12 +297,18 @@ int device_is_dependent(struct device *dev, void *target)
>  		return ret;
>
>  	list_for_each_entry(link, &dev->links.consumers, s_node) {
> +		/* if already marked before as dependent, bail early */
> +		if (link->flags & DL_FLAG_DEVICE_IS_DEPENDENT)
> +			return 1;

On a second thought, this is wrong since it might be a different
consumer link.

> +
>  		if ((link->flags & ~DL_FLAG_INFERRED) ==
>  		    (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED))
>  			continue;
>
> -		if (link->consumer == target)
> +		if (link->consumer == target) {
> +			link->flags |= DL_FLAG_DEVICE_IS_DEPENDENT;
>  			return 1;
> +		}
>
>  		ret = device_is_dependent(link->consumer, target);
>  		if (ret)
> @@ -1660,11 +1666,13 @@ static void fw_devlink_relax_link(struct device_link *link)
>  	if (!(link->flags & DL_FLAG_INFERRED))
>  		return;
>
> -	if (link->flags == (DL_FLAG_MANAGED | FW_DEVLINK_FLAGS_PERMISSIVE))
> +	if ((link->flags & (DL_FLAG_MANAGED | FW_DEVLINK_FLAGS_PERMISSIVE)) ==
> +			(DL_FLAG_MANAGED | FW_DEVLINK_FLAGS_PERMISSIVE))
>  		return;
>
>  	pm_runtime_drop_link(link);
> -	link->flags = DL_FLAG_MANAGED | FW_DEVLINK_FLAGS_PERMISSIVE;
> +	link->flags &= DL_FLAG_DEVICE_IS_DEPENDENT;
> +	link->flags |= DL_FLAG_MANAGED | FW_DEVLINK_FLAGS_PERMISSIVE;
>  	dev_dbg(link->consumer, "Relaxing link with %s\n",
>  		dev_name(link->supplier));
>  }
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 424b55df0272..3b0c4b777a60 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -317,6 +317,7 @@ enum device_link_state {
>   * MANAGED: The core tracks presence of supplier/consumer drivers (internal).
>   * SYNC_STATE_ONLY: Link only affects sync_state() behavior.
>   * INFERRED: Inferred from data (eg: firmware) and not from driver actions.
> + * DEVICE_IS_DEPENDENT: The consumer is dependent on the supplier
>   */
>  #define DL_FLAG_STATELESS		BIT(0)
>  #define DL_FLAG_AUTOREMOVE_CONSUMER	BIT(1)
> @@ -327,6 +328,7 @@ enum device_link_state {
>  #define DL_FLAG_MANAGED			BIT(6)
>  #define DL_FLAG_SYNC_STATE_ONLY		BIT(7)
>  #define DL_FLAG_INFERRED		BIT(8)
> +#define DL_FLAG_DEVICE_IS_DEPENDENT	BIT(9)
>
>  /**
>   * enum dl_dev_state - Device driver presence tracking information.
> --
> 2.34.3
>