[v2] of: allocate / free phandle cache outside of the devtree_lock
diff mbox series

Message ID 20191129140446.dqb5y4uzv57cislp@linutronix.de
State Superseded
Headers show
Series
  • [v2] of: allocate / free phandle cache outside of the devtree_lock
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Sebastian Andrzej Siewior Nov. 29, 2019, 2:04 p.m. UTC
The phandle cache code allocates memory while holding devtree_lock which
is a raw_spinlock_t. Memory allocation (and free()) is not possible on
RT while a raw_spinlock_t is held.
Invoke the kfree() and kcalloc() while the lock is dropped.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2:
  - Add comments regarding memory allocation under the raw_spinlock_t
  - use a temporary variable for the memory allocation.

 drivers/of/base.c |   32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

Comments

Frank Rowand Nov. 30, 2019, 2:46 a.m. UTC | #1
On 11/29/19 8:04 AM, Sebastian Andrzej Siewior wrote:
> The phandle cache code allocates memory while holding devtree_lock which
> is a raw_spinlock_t. Memory allocation (and free()) is not possible on
> RT while a raw_spinlock_t is held.
> Invoke the kfree() and kcalloc() while the lock is dropped.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v1…v2:
>   - Add comments regarding memory allocation under the raw_spinlock_t
>   - use a temporary variable for the memory allocation.
> 
>  drivers/of/base.c |   32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -136,33 +136,37 @@ static struct device_node **phandle_cach
>  static u32 phandle_cache_mask;
>  
>  /*
> - * Caller must hold devtree_lock.
> + * Caller must hold devtree_lock under which kfree() can't be used so caller
> + * needs to kfree() the returned pointer.
>   */
> -static void __of_free_phandle_cache(void)
> +static struct device_node **__of_free_phandle_cache(void)
>  {
>  	u32 cache_entries = phandle_cache_mask + 1;
>  	u32 k;
> +	struct device_node **shadow;
>  
>  	if (!phandle_cache)
> -		return;
> +		return NULL;
>  
>  	for (k = 0; k < cache_entries; k++)
>  		of_node_put(phandle_cache[k]);
>  
> -	kfree(phandle_cache);
> +	shadow = phandle_cache;
>  	phandle_cache = NULL;
> +	return shadow;
>  }
>  
>  int of_free_phandle_cache(void)
>  {
>  	unsigned long flags;
> +	struct device_node **shadow;
>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  
> -	__of_free_phandle_cache();
> +	shadow = __of_free_phandle_cache();
>  
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> -
> +	kfree(shadow);
>  	return 0;
>  }
>  #if !defined(CONFIG_MODULES)
> @@ -197,10 +201,12 @@ void of_populate_phandle_cache(void)
>  	u32 cache_entries;
>  	struct device_node *np;
>  	u32 phandles = 0;
> +	struct device_node **shadow;
> +	struct device_node **tmp_phandle_cache;
>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  
> -	__of_free_phandle_cache();
> +	shadow = __of_free_phandle_cache();
>  
>  	for_each_of_allnodes(np)
>  		if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
> @@ -208,12 +214,19 @@ void of_populate_phandle_cache(void)
>  
>  	if (!phandles)
>  		goto out;
> +	/*
> +	 * kcalloc() can't be used while a raw_spinlock_t held so it is dropped
> +	 * for the allocation.
> +	 */
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
>  	cache_entries = roundup_pow_of_two(phandles);
>  	phandle_cache_mask = cache_entries - 1;
>  
> -	phandle_cache = kcalloc(cache_entries, sizeof(*phandle_cache),
> -				GFP_ATOMIC);
> +	tmp_phandle_cache = kcalloc(cache_entries, sizeof(*phandle_cache),
> +				    GFP_ATOMIC);
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	phandle_cache = tmp_phandle_cache;
>  	if (!phandle_cache)
>  		goto out;
>  
> @@ -225,6 +238,7 @@ void of_populate_phandle_cache(void)
>  
>  out:
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	kfree(shadow);
>  }
>  
>  void __init of_core_init(void)
> 

Reviewed-by: Frank Rowand <frank.rowand@sony.com>

Compiled and booted on 5.4.  Did not do any more detailed testing.

-Frank

Patch
diff mbox series

--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -136,33 +136,37 @@  static struct device_node **phandle_cach
 static u32 phandle_cache_mask;
 
 /*
- * Caller must hold devtree_lock.
+ * Caller must hold devtree_lock under which kfree() can't be used so caller
+ * needs to kfree() the returned pointer.
  */
-static void __of_free_phandle_cache(void)
+static struct device_node **__of_free_phandle_cache(void)
 {
 	u32 cache_entries = phandle_cache_mask + 1;
 	u32 k;
+	struct device_node **shadow;
 
 	if (!phandle_cache)
-		return;
+		return NULL;
 
 	for (k = 0; k < cache_entries; k++)
 		of_node_put(phandle_cache[k]);
 
-	kfree(phandle_cache);
+	shadow = phandle_cache;
 	phandle_cache = NULL;
+	return shadow;
 }
 
 int of_free_phandle_cache(void)
 {
 	unsigned long flags;
+	struct device_node **shadow;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 
-	__of_free_phandle_cache();
+	shadow = __of_free_phandle_cache();
 
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-
+	kfree(shadow);
 	return 0;
 }
 #if !defined(CONFIG_MODULES)
@@ -197,10 +201,12 @@  void of_populate_phandle_cache(void)
 	u32 cache_entries;
 	struct device_node *np;
 	u32 phandles = 0;
+	struct device_node **shadow;
+	struct device_node **tmp_phandle_cache;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 
-	__of_free_phandle_cache();
+	shadow = __of_free_phandle_cache();
 
 	for_each_of_allnodes(np)
 		if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
@@ -208,12 +214,19 @@  void of_populate_phandle_cache(void)
 
 	if (!phandles)
 		goto out;
+	/*
+	 * kcalloc() can't be used while a raw_spinlock_t held so it is dropped
+	 * for the allocation.
+	 */
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	cache_entries = roundup_pow_of_two(phandles);
 	phandle_cache_mask = cache_entries - 1;
 
-	phandle_cache = kcalloc(cache_entries, sizeof(*phandle_cache),
-				GFP_ATOMIC);
+	tmp_phandle_cache = kcalloc(cache_entries, sizeof(*phandle_cache),
+				    GFP_ATOMIC);
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	phandle_cache = tmp_phandle_cache;
 	if (!phandle_cache)
 		goto out;
 
@@ -225,6 +238,7 @@  void of_populate_phandle_cache(void)
 
 out:
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	kfree(shadow);
 }
 
 void __init of_core_init(void)