On 12/18/18 11:40 AM, frowand.list@xxxxxxxxx wrote:
> From: Frank Rowand <frank.rowand@xxxxxxxx>
>
> Non-overlay dynamic devicetree node removal may leave the node in
> the phandle cache. Subsequent calls to of_find_node_by_phandle()
> will incorrectly find the stale entry. Remove the node from the
> cache.
>
> Add paranoia checks in of_find_node_by_phandle() as a second level
> of defense (do not return cached node if detached, do not add node
> to cache if detached).
>
> Fixes: 0b3ce78e90fc ("of: cache phandle nodes to reduce cost of of_find_node_by_phandle()")
> Reported-by: Michael Bringmann <mwb@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx>
> ---
>
> do not "cc: stable", unless the following commits are also in stable:
>
> commit e54192b48da7 ("of: fix phandle cache creation for DTs with no phandles")
> commit b9952b5218ad ("of: overlay: update phandle cache on overlay apply and remove")
> commit 0b3ce78e90fc ("of: cache phandle nodes to reduce cost of of_find_node_by_phandle()")
>
>
> Changes since v2:
> - add temporary variable np in __of_free_phandle_cache_entry() to improve
> readability
> - explain reason for WARN_ON() in comment
> - add Fixes tag in patch comment
I should have carried this forward:
changes since v1:
- add WARN_ON(1) for unexpected condition in of_find_node_by_phandle()
-Frank
>
> drivers/of/base.c | 31 ++++++++++++++++++++++++++++++-
> drivers/of/dynamic.c | 3 +++
> drivers/of/of_private.h | 4 ++++
> 3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 6c33d63361b8..6d20b6dcf034 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -162,6 +162,28 @@ int of_free_phandle_cache(void)
> late_initcall_sync(of_free_phandle_cache);
> #endif
>
> +/*
> + * Caller must hold devtree_lock.
> + */
> +void __of_free_phandle_cache_entry(phandle handle)
> +{
> + phandle masked_handle;
> + struct device_node *np;
> +
> + if (!handle)
> + return;
> +
> + masked_handle = handle & phandle_cache_mask;
> +
> + if (phandle_cache) {
> + np = phandle_cache[masked_handle];
> + if (np && handle == np->phandle) {
> + of_node_put(np);
> + phandle_cache[masked_handle] = NULL;
> + }
> + }
> +}
> +
> void of_populate_phandle_cache(void)
> {
> unsigned long flags;
> @@ -1209,11 +1231,18 @@ struct device_node *of_find_node_by_phandle(phandle handle)
> if (phandle_cache[masked_handle] &&
> handle == phandle_cache[masked_handle]->phandle)
> np = phandle_cache[masked_handle];
> + if (np && of_node_check_flag(np, OF_DETACHED)) {
> + WARN_ON(1); /* did not uncache np on node removal */
> + of_node_put(np);
> + phandle_cache[masked_handle] = NULL;
> + np = NULL;
> + }
> }
>
> if (!np) {
> for_each_of_allnodes(np)
> - if (np->phandle == handle) {
> + if (np->phandle == handle &&
> + !of_node_check_flag(np, OF_DETACHED)) {
> if (phandle_cache) {
> /* will put when removed from cache */
> of_node_get(np);
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index f4f8ed9b5454..ecea92f68c87 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -268,6 +268,9 @@ void __of_detach_node(struct device_node *np)
> }
>
> of_node_set_flag(np, OF_DETACHED);
> +
> + /* race with of_find_node_by_phandle() prevented by devtree_lock */
> + __of_free_phandle_cache_entry(np->phandle);
> }
>
> /**
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 5d1567025358..24786818e32e 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -84,6 +84,10 @@ static inline void __of_detach_node_sysfs(struct device_node *np) {}
> int of_resolve_phandles(struct device_node *tree);
> #endif
>
> +#if defined(CONFIG_OF_DYNAMIC)
> +void __of_free_phandle_cache_entry(phandle handle);
> +#endif
> +
> #if defined(CONFIG_OF_OVERLAY)
> void of_overlay_mutex_lock(void);
> void of_overlay_mutex_unlock(void);
>
