[PATCH 0/2] dma-debug: prevent early callers from crashing

[PATCH 0/2] dma-debug: prevent early callers from crashing

[Florian Fainelli]

Hi Dan,

This patch series addresses a problem seen on the brcmstb ARM platform where
dma_debug_init is called by the ARM kernel at fs_initcall time, while some of
our callers using the DMA-API were running at arch_initcall time.

Unless CONFIG_DMA_API_DEBUG is set, this is completely silent.

First patch introduces a helper function to check whether a DMA-API debug
implementation is allowed to proceed, while the second patch introduces an
initialization flag and uses the helper. A complete writeup of how and why this
crashes is provided in patch 2.

Thank you!

Florian Fainelli (2):
  dma-debug: introduce dma_debug_disabled
  dma-debug: prevent early callers from crashing

 lib/dma-debug.c | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

-- 
1.9.1

[PATCH 1/2] dma-debug: introduce dma_debug_disabled

[Florian Fainelli]

Add a helper function which returns whether the DMA debugging API is
disabled, right now we only check for global_disable, but in order to
accomodate early callers of the DMA-API, we will check for more
initialization flags in the future.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 lib/dma-debug.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index add80cc02dbe..1ac35dbaf8e0 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -102,6 +102,11 @@ static DEFINE_SPINLOCK(free_entries_lock);
 /* Global disable flag - will be set in case of an error */
 static u32 global_disable __read_mostly;
 
+static inline bool dma_debug_disabled(void)
+{
+	return global_disable;
+}
+
 /* Global error count */
 static u32 error_count;
 
@@ -945,7 +950,7 @@ static int dma_debug_device_change(struct notifier_block *nb, unsigned long acti
 	struct dma_debug_entry *uninitialized_var(entry);
 	int count;
 
-	if (global_disable)
+	if (dma_debug_disabled())
 		return 0;
 
 	switch (action) {
@@ -973,7 +978,7 @@ void dma_debug_add_bus(struct bus_type *bus)
 {
 	struct notifier_block *nb;
 
-	if (global_disable)
+	if (dma_debug_disabled())
 		return;
 
 	nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
@@ -994,7 +999,7 @@ void dma_debug_init(u32 num_entries)
 {
 	int i;
 
-	if (global_disable)
+	if (dma_debug_disabled())
 		return;
 
 	for (i = 0; i < HASH_SIZE; ++i) {
@@ -1243,7 +1248,7 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 {
 	struct dma_debug_entry *entry;
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	if (dma_mapping_error(dev, dma_addr))
@@ -1283,7 +1288,7 @@ void debug_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
 	struct hash_bucket *bucket;
 	unsigned long flags;
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	ref.dev = dev;
@@ -1325,7 +1330,7 @@ void debug_dma_unmap_page(struct device *dev, dma_addr_t addr,
 		.direction      = direction,
 	};
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	if (map_single)
@@ -1342,7 +1347,7 @@ void debug_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	struct scatterlist *s;
 	int i;
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	for_each_sg(sg, s, mapped_ents, i) {
@@ -1395,7 +1400,7 @@ void debug_dma_unmap_sg(struct device *dev, struct scatterlist *sglist,
 	struct scatterlist *s;
 	int mapped_ents = 0, i;
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	for_each_sg(sglist, s, nelems, i) {
@@ -1427,7 +1432,7 @@ void debug_dma_alloc_coherent(struct device *dev, size_t size,
 {
 	struct dma_debug_entry *entry;
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	if (unlikely(virt == NULL))
@@ -1462,7 +1467,7 @@ void debug_dma_free_coherent(struct device *dev, size_t size,
 		.direction      = DMA_BIDIRECTIONAL,
 	};
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	check_unmap(&ref);
@@ -1474,7 +1479,7 @@ void debug_dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle,
 {
 	struct dma_debug_entry ref;
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	ref.type         = dma_debug_single;
@@ -1494,7 +1499,7 @@ void debug_dma_sync_single_for_device(struct device *dev,
 {
 	struct dma_debug_entry ref;
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	ref.type         = dma_debug_single;
@@ -1515,7 +1520,7 @@ void debug_dma_sync_single_range_for_cpu(struct device *dev,
 {
 	struct dma_debug_entry ref;
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	ref.type         = dma_debug_single;
@@ -1536,7 +1541,7 @@ void debug_dma_sync_single_range_for_device(struct device *dev,
 {
 	struct dma_debug_entry ref;
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	ref.type         = dma_debug_single;
@@ -1556,7 +1561,7 @@ void debug_dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
 	struct scatterlist *s;
 	int mapped_ents = 0, i;
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	for_each_sg(sg, s, nelems, i) {
@@ -1589,7 +1594,7 @@ void debug_dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
 	struct scatterlist *s;
 	int mapped_ents = 0, i;
 
-	if (unlikely(global_disable))
+	if (unlikely(dma_debug_disabled()))
 		return;
 
 	for_each_sg(sg, s, nelems, i) {
-- 
1.9.1

[PATCH 2/2] dma-debug: prevent early callers from crashing

[Florian Fainelli]

dma_debug_init() is called by architecture specific code at different
levels, but typically as a fs_initcall due to the debugfs
initialization. Some platforms may have early callers of the DMA-API,
running prior to the fs_initcall() level, which is not much of an issue
unless CONFIG_DMA_API_DEBUG is set. Whe the DMA-API debugging facilities
are turned on a caller will go through:

debug_dma_map_{single,page}
	-> dma_mapping_error (inline function usually)
		-> debug_dma_mapping_error
			-> get_hash_bucket

Calling get_hash_bucket() returns a valid hash value since we hash on
high bits of the dma_addr cookie, but we will grab an unitialized
spinlock, which typically won't crash but produce a warning, the real
crash will however happen during the bucket list traversal because the
list has not been initialized yet.

An obvious solution is of course to move some of the offenders to run
after the fs_initcall level, but since this might not always be an
option, we add a flag "dma_debug_initialized" which is set to false by
default, and set to true once dma_debug_init() has had a chance to run.

The dma_debug_disabled() helper function previously introduced just
needs to check for dma_debug_initialized to allow the caller to proceed
or not.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 lib/dma-debug.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 1ac35dbaf8e0..9722bd2dbc9b 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -102,9 +102,12 @@ static DEFINE_SPINLOCK(free_entries_lock);
 /* Global disable flag - will be set in case of an error */
 static u32 global_disable __read_mostly;
 
+/* Early initialization disable flag, set at the end of dma_debug_init */
+static bool dma_debug_initialized __read_mostly;
+
 static inline bool dma_debug_disabled(void)
 {
-	return global_disable;
+	return global_disable || !dma_debug_initialized;
 }
 
 /* Global error count */
@@ -999,7 +1002,10 @@ void dma_debug_init(u32 num_entries)
 {
 	int i;
 
-	if (dma_debug_disabled())
+	/* Do not use dma_debug_initialized here, since we really want to be
+	 * called to set dma_debug_initialized
+	 */
+	if (global_disable)
 		return;
 
 	for (i = 0; i < HASH_SIZE; ++i) {
@@ -1026,6 +1032,8 @@ void dma_debug_init(u32 num_entries)
 
 	nr_total_entries = num_free_entries;
 
+	dma_debug_initialized = true;
+
 	pr_info("DMA-API: debugging enabled by kernel config\n");
 }
 
-- 
1.9.1

Re: [PATCH 2/2] dma-debug: prevent early callers from crashing

[Florian Fainelli]

On 11/13/2014 08:31 PM, Florian Fainelli wrote:
> dma_debug_init() is called by architecture specific code at different
> levels, but typically as a fs_initcall due to the debugfs
> initialization. Some platforms may have early callers of the DMA-API,
> running prior to the fs_initcall() level, which is not much of an issue
> unless CONFIG_DMA_API_DEBUG is set. Whe the DMA-API debugging facilities
> are turned on a caller will go through:
> 
> debug_dma_map_{single,page}
> 	-> dma_mapping_error (inline function usually)
> 		-> debug_dma_mapping_error
> 			-> get_hash_bucket
> 
> Calling get_hash_bucket() returns a valid hash value since we hash on
> high bits of the dma_addr cookie, but we will grab an unitialized
> spinlock, which typically won't crash but produce a warning, the real
> crash will however happen during the bucket list traversal because the
> list has not been initialized yet.
> 
> An obvious solution is of course to move some of the offenders to run
> after the fs_initcall level, but since this might not always be an
> option, we add a flag "dma_debug_initialized" which is set to false by
> default, and set to true once dma_debug_init() has had a chance to run.
> 
> The dma_debug_disabled() helper function previously introduced just
> needs to check for dma_debug_initialized to allow the caller to proceed
> or not.

BTW, one approach I initially took was to add an "initialized" flag to
the hash bucket structure that would be set to false by default, set to
true once we have initialized the array in dma_debug_init(). The problem
with that is that debug_dma_mapping_error() returns void, so we can't
propagate an error back to the caller: debug_dma_map_page().

debug_dma_map_page() then proceeds with creating a new entry, but since
we still have not initialized everything, in particular the free_entries
list is empty, and this triggers a fatal error that leads to setting
global_disable, not ideal, hence this approach.

> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  lib/dma-debug.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index 1ac35dbaf8e0..9722bd2dbc9b 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -102,9 +102,12 @@ static DEFINE_SPINLOCK(free_entries_lock);
>  /* Global disable flag - will be set in case of an error */
>  static u32 global_disable __read_mostly;
>  
> +/* Early initialization disable flag, set at the end of dma_debug_init */
> +static bool dma_debug_initialized __read_mostly;
> +
>  static inline bool dma_debug_disabled(void)
>  {
> -	return global_disable;
> +	return global_disable || !dma_debug_initialized;
>  }
>  
>  /* Global error count */
> @@ -999,7 +1002,10 @@ void dma_debug_init(u32 num_entries)
>  {
>  	int i;
>  
> -	if (dma_debug_disabled())
> +	/* Do not use dma_debug_initialized here, since we really want to be
> +	 * called to set dma_debug_initialized
> +	 */
> +	if (global_disable)
>  		return;
>  
>  	for (i = 0; i < HASH_SIZE; ++i) {
> @@ -1026,6 +1032,8 @@ void dma_debug_init(u32 num_entries)
>  
>  	nr_total_entries = num_free_entries;
>  
> +	dma_debug_initialized = true;
> +
>  	pr_info("DMA-API: debugging enabled by kernel config\n");
>  }
>  
>