Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v3 2/2] of: __of_detach_node() - remove node from phandle
 cache
From:   Frank Rowand <frowand.list@gmail.com>
To:     robh+dt@kernel.org, Michael Bringmann <mwb@linux.vnet.ibm.com>,
        linuxppc-dev@lists.ozlabs.org
Cc:     Michael Ellerman <mpe@ellerman.id.au>,
        Tyrel Datwyler <tyreld@linux.vnet.ibm.com>,
        Thomas Falcon <tlfalcon@linux.vnet.ibm.com>,
        Juliet Kim <minkim@us.ibm.com>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1545162003-11577-1-git-send-email-frowand.list@gmail.com>
 <1545162003-11577-3-git-send-email-frowand.list@gmail.com>
Message-ID: <8236132c-ccfd-4d2a-c490-7a52ec9df0e8@gmail.com>
Date:   Tue, 18 Dec 2018 11:44:07 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <1545162003-11577-3-git-send-email-frowand.list@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 12/18/18 11:40 AM, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> 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@linux.vnet.ibm.com>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
> 
> 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);
>