Skip to contentSkip to conversation

dma-debug: Add dma map/unmap error tracking support

PATCH In review Shuah Khan 5 participants 14 messages Sep 2, 2012 – Sep 10, 2012

Subsystems (2)
lore
Share & tools
lore

Shuah Khan

author
#0 lore raw

PATCH 1/1 dma-debug: Add dma map/unmap error tracking support

A recent dma mapping error analysis effort showed that a large precentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors. Reference: https://lkml.org/lkml/2012/8/10/326

Adding support for tracking dma mapping and unmapping errors to help assess the following:

When do dma mapping errors get detected?
How often do these errors occur?
Why don't we see failures related to missing dma mapping error checks? Are they silent failures?

---
 Documentation/DMA-API.txt |    7 +++++++
 lib/dma-debug.c           |   26 +++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)
Documentation/DMA-API.txt +7 -0
index 66bd97a..ee10a11 100644
@@ -638,6 +638,13 @@ this directory the following files can currently be found:
 	dma-api/error_count	This file is read-only and shows the total
 				numbers of errors found.
 
+	dma-api/dma_map_errors  This file is read-only and shows the total
+				number of dma mapping errors detected.
+
+	dma-api/dma_unmap_errors
+				This file is read-only and shows the total
+				number of invalid dma unmapping attempts.
+
 	dma-api/num_errors	The number in this file shows how many
 				warnings will be printed to the kernel log
 				before it stops. This number is initialized to
lib/dma-debug.c +25 -1
index 66ce414..8596114 100644
@@ -83,6 +83,10 @@ static u32 global_disable __read_mostly;
 /* Global error count */
 static u32 error_count;
 
+/* dma mapping error counts */
+static u32 dma_map_errors;
+static u32 dma_unmap_errors;
+
 /* Global error show enable*/
 static u32 show_all_errors __read_mostly;
 /* Number of errors to show */
@@ -104,6 +108,8 @@ static struct dentry *show_num_errors_dent  __read_mostly;
 static struct dentry *num_free_entries_dent __read_mostly;
 static struct dentry *min_free_entries_dent __read_mostly;
 static struct dentry *filter_dent           __read_mostly;
+static struct dentry *dma_map_errors_dent   __read_mostly;
+static struct dentry *dma_unmap_errors_dent __read_mostly;
 
 /* per-driver filter related state */
 
@@ -695,6 +701,19 @@ static int dma_debug_fs_init(void)
 	if (!filter_dent)
 		goto out_err;
 
+	dma_map_errors_dent = debugfs_create_u32("dma_map_errors", 0444,
+			dma_debug_dent,
+			&dma_map_errors);
+
+	if (!dma_map_errors_dent)
+		goto out_err;
+
+	dma_unmap_errors_dent = debugfs_create_u32("dma_unmap_errors", 0444,
+			dma_debug_dent,
+			&dma_unmap_errors);
+	if (!dma_unmap_errors_dent)
+		goto out_err;
+
 	return 0;
 
 out_err:
@@ -850,6 +869,7 @@ static void check_unmap(struct dma_debug_entry *ref)
 	unsigned long flags;
 
 	if (dma_mapping_error(ref->dev, ref->dev_addr)) {
+		dma_unmap_errors += 1;
 		err_printk(ref->dev, NULL, "DMA-API: device driver tries "
 			   "to free an invalid DMA memory address\n");
 		return;
@@ -1022,8 +1042,12 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 	if (unlikely(global_disable))
 		return;
 
-	if (unlikely(dma_mapping_error(dev, dma_addr)))
+	if (unlikely(dma_mapping_error(dev, dma_addr))) {
+		dma_map_errors += 1;
+		err_printk(dev, NULL,
+			   "DMA-API: dma_map_page() returned error\n");
 		return;
+	}
 
 	entry = dma_entry_alloc();
 	if (!entry)
Signature · 1 line
1.7.9.5
#1 lore raw
Quoted 4 lines · Shuah Khan

A recent dma mapping error analysis effort showed that a large precentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors. Reference: https://lkml.org/lkml/2012/8/10/326

So were you able to catch some naughty drivers with this?

Quoted 85 lines

Adding support for tracking dma mapping and unmapping errors to help assess the following:

When do dma mapping errors get detected?
How often do these errors occur?
Why don't we see failures related to missing dma mapping error checks? Are they silent failures?

---
 Documentation/DMA-API.txt |    7 +++++++
 lib/dma-debug.c           |   26 +++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)
Documentation/DMA-API.txt +7 -0
index 66bd97a..ee10a11 100644
@@ -638,6 +638,13 @@ this directory the following files can currently be found:
 	dma-api/error_count	This file is read-only and shows the total
 				numbers of errors found.
 
+	dma-api/dma_map_errors  This file is read-only and shows the total
+				number of dma mapping errors detected.
+
+	dma-api/dma_unmap_errors
+				This file is read-only and shows the total
+				number of invalid dma unmapping attempts.
+
 	dma-api/num_errors	The number in this file shows how many
 				warnings will be printed to the kernel log
 				before it stops. This number is initialized to
lib/dma-debug.c +20 -0
index 66ce414..8596114 100644
@@ -83,6 +83,10 @@ static u32 global_disable __read_mostly;
 /* Global error count */
 static u32 error_count;
 
+/* dma mapping error counts */
+static u32 dma_map_errors;
+static u32 dma_unmap_errors;
+
 /* Global error show enable*/
 static u32 show_all_errors __read_mostly;
 /* Number of errors to show */
@@ -104,6 +108,8 @@ static struct dentry *show_num_errors_dent  __read_mostly;
 static struct dentry *num_free_entries_dent __read_mostly;
 static struct dentry *min_free_entries_dent __read_mostly;
 static struct dentry *filter_dent           __read_mostly;
+static struct dentry *dma_map_errors_dent   __read_mostly;
+static struct dentry *dma_unmap_errors_dent __read_mostly;
 
 /* per-driver filter related state */
 
@@ -695,6 +701,19 @@ static int dma_debug_fs_init(void)
 	if (!filter_dent)
 		goto out_err;
 
+	dma_map_errors_dent = debugfs_create_u32("dma_map_errors", 0444,
+			dma_debug_dent,
+			&dma_map_errors);
+
+	if (!dma_map_errors_dent)
+		goto out_err;
+
+	dma_unmap_errors_dent = debugfs_create_u32("dma_unmap_errors", 0444,
+			dma_debug_dent,
+			&dma_unmap_errors);
+	if (!dma_unmap_errors_dent)
+		goto out_err;
+
 	return 0;
 
 out_err:
@@ -850,6 +869,7 @@ static void check_unmap(struct dma_debug_entry *ref)
 	unsigned long flags;
 
 	if (dma_mapping_error(ref->dev, ref->dev_addr)) {
+		dma_unmap_errors += 1;
 		err_printk(ref->dev, NULL, "DMA-API: device driver tries "
 			   "to free an invalid DMA memory address\n");
 		return;

lib/dma-debug.c void debug_dma_map_page(struct device *dev, struct page *page, size_t offset, · line 1050

Quoted 10 lines
lib/dma-debug.c +5 -1
@@ -1022,8 +1042,12 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 	if (unlikely(global_disable))
 		return;
 
-	if (unlikely(dma_mapping_error(dev, dma_addr)))
+	if (unlikely(dma_mapping_error(dev, dma_addr))) {
+		dma_map_errors += 1;
+		err_printk(dev, NULL,
+			   "DMA-API: dma_map_page() returned error\n");
 		return;
+	}

So this will print if the dma_map_page failed (which can happen).

I was initially thinking that this patch would contain a state for the driver of whether after map it has called dma_mapping_error. So this function would increment some internal state, and if dma_mapping_error on that specific dma_addr it would decrement it. If it never occured, then we would print on the unmap that the device never had called dma_mapping_error on said dma_addr?

Quoted 7 lines
lib/dma-debug.c +0 -0
 	entry = dma_entry_alloc();
 	if (!entry)
Signature · 1 line
1.7.9.5
author
#2 lore raw
Quoted 7 lines · Konrad Rzeszutek Wilk
Quoted 4 lines · Shuah Khan

A recent dma mapping error analysis effort showed that a large precentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors. Reference: https://lkml.org/lkml/2012/8/10/326

So were you able to catch some naughty drivers with this?

I did compile a complete list of drivers that don't check dma mapping errors from my analysis. Are you interested in seeing the full analysis?

Quoted 89 lines
Quoted 89 lines

Adding support for tracking dma mapping and unmapping errors to help assess the following:

When do dma mapping errors get detected?
How often do these errors occur?
Why don't we see failures related to missing dma mapping error checks? Are they silent failures?

---
 Documentation/DMA-API.txt |    7 +++++++
 lib/dma-debug.c           |   26 +++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)
Documentation/DMA-API.txt +7 -0
index 66bd97a..ee10a11 100644
@@ -638,6 +638,13 @@ this directory the following files can currently be found:
 	dma-api/error_count	This file is read-only and shows the total
 				numbers of errors found.
 
+	dma-api/dma_map_errors  This file is read-only and shows the total
+				number of dma mapping errors detected.
+
+	dma-api/dma_unmap_errors
+				This file is read-only and shows the total
+				number of invalid dma unmapping attempts.
+
 	dma-api/num_errors	The number in this file shows how many
 				warnings will be printed to the kernel log
 				before it stops. This number is initialized to
lib/dma-debug.c +20 -0
index 66ce414..8596114 100644
@@ -83,6 +83,10 @@ static u32 global_disable __read_mostly;
 /* Global error count */
 static u32 error_count;
 
+/* dma mapping error counts */
+static u32 dma_map_errors;
+static u32 dma_unmap_errors;
+
 /* Global error show enable*/
 static u32 show_all_errors __read_mostly;
 /* Number of errors to show */
@@ -104,6 +108,8 @@ static struct dentry *show_num_errors_dent  __read_mostly;
 static struct dentry *num_free_entries_dent __read_mostly;
 static struct dentry *min_free_entries_dent __read_mostly;
 static struct dentry *filter_dent           __read_mostly;
+static struct dentry *dma_map_errors_dent   __read_mostly;
+static struct dentry *dma_unmap_errors_dent __read_mostly;
 
 /* per-driver filter related state */
 
@@ -695,6 +701,19 @@ static int dma_debug_fs_init(void)
 	if (!filter_dent)
 		goto out_err;
 
+	dma_map_errors_dent = debugfs_create_u32("dma_map_errors", 0444,
+			dma_debug_dent,
+			&dma_map_errors);
+
+	if (!dma_map_errors_dent)
+		goto out_err;
+
+	dma_unmap_errors_dent = debugfs_create_u32("dma_unmap_errors", 0444,
+			dma_debug_dent,
+			&dma_unmap_errors);
+	if (!dma_unmap_errors_dent)
+		goto out_err;
+
 	return 0;
 
 out_err:
@@ -850,6 +869,7 @@ static void check_unmap(struct dma_debug_entry *ref)
 	unsigned long flags;
 
 	if (dma_mapping_error(ref->dev, ref->dev_addr)) {
+		dma_unmap_errors += 1;
 		err_printk(ref->dev, NULL, "DMA-API: device driver tries "
 			   "to free an invalid DMA memory address\n");
 		return;
@@ -1022,8 +1042,12 @@ void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 	if (unlikely(global_disable))
 		return;
 
Quoted 9 lines

lib/dma-debug.c void debug_dma_map_page(struct device *dev, struct page *page, size_t offset, · line 1050

Quoted 7 lines
lib/dma-debug.c +5 -1
-	if (unlikely(dma_mapping_error(dev, dma_addr)))
+	if (unlikely(dma_mapping_error(dev, dma_addr))) {
+		dma_map_errors += 1;
+		err_printk(dev, NULL,
+			   "DMA-API: dma_map_page() returned error\n");
 		return;
+	}

So this will print if the dma_map_page failed (which can happen).

Correct. This gets printed DMA DEBUG only mode, whenever mapping fails.

Quoted 6 lines

I was initially thinking that this patch would contain a state for the driver of whether after map it has called dma_mapping_error. So this function would increment some internal state, and if dma_mapping_error on that specific dma_addr it would decrement it. If it never occured, then we would print on the unmap that the device never had called dma_mapping_error on said dma_addr?

That is a good idea. Let me see if I understand what you are saying correctly. Add a new field to dma_debug_entry structure and keep state and clear it if dma_mapping_error() is called. This will require adding a debug interface for dma_mapping_error() which is not hard to do. Is this close to what you are thinking?

Quoted 8 lines
Quoted 8 lines
lib/dma-debug.c +0 -0
 	entry = dma_entry_alloc();
 	if (!entry)
Signature · 1 line
1.7.9.5
#3 lore raw
Quoted 11 lines · Shuah Khan
Quoted 7 lines · Konrad Rzeszutek Wilk
Quoted 4 lines · Shuah Khan

A recent dma mapping error analysis effort showed that a large precentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors. Reference: https://lkml.org/lkml/2012/8/10/326

So were you able to catch some naughty drivers with this?

I did compile a complete list of drivers that don't check dma mapping errors from my analysis. Are you interested in seeing the full analysis?

Yes, plus the authors of the drivers are probably interested in it as well.
..snip..

Quoted 11 lines
Quoted 5 lines

I was initially thinking that this patch would contain a state for the driver of whether after map it has called dma_mapping_error. So this function would increment some internal state, and if dma_mapping_error on that specific dma_addr it would decrement it. If it never occured, then we would print on the unmap that the device never had called dma_mapping_error on said dma_addr?

That is a good idea. Let me see if I understand what you are saying correctly. Add a new field to dma_debug_entry structure and keep state and clear it if dma_mapping_error() is called. This will require adding a debug interface for dma_mapping_error() which is not hard to do. Is this close to what you are thinking?

Right. It is more complex than this patch but it should provide a nicer "trap" mechanism to alert driver writers that they are not checking DMA addresses properly.

author
#4 lore raw
Quoted 8 lines · Konrad Rzeszutek Wilk
Quoted 7 lines · Shuah Khan
Quoted 6 lines · Konrad Rzeszutek Wilk
Quoted 4 lines · Shuah Khan

A recent dma mapping error analysis effort showed that a large precentage of dma_map_single() and dma_map_page() returns are not checked for mapping errors. Reference: https://lkml.org/lkml/2012/8/10/326

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

So were you able to catch some naughty drivers with this?

I did compile a complete list of drivers that don't check dma mapping errors from my analysis. Are you interested in seeing the full analysis?

Yes, plus the authors of the drivers are probably interested in it as well.
..snip..

I compiled very detailed information on the nature of problems with dma mapping error checking, ranging from not checking mapping errors to not unmapping etc. I would like to share this information in such a way that others can leverage the research I already did. One good way to communicate this information would be on one of the kernel wikis. Ideas, suggestions?

Quoted 12 lines
Quoted 11 lines
Quoted 5 lines

I was initially thinking that this patch would contain a state for the driver of whether after map it has called dma_mapping_error. So this function would increment some internal state, and if dma_mapping_error on that specific dma_addr it would decrement it. If it never occured, then we would print on the unmap that the device never had called dma_mapping_error on said dma_addr?

That is a good idea. Let me see if I understand what you are saying correctly. Add a new field to dma_debug_entry structure and keep state and clear it if dma_mapping_error() is called. This will require adding a debug interface for dma_mapping_error() which is not hard to do. Is this close to what you are thinking?

Quoted 3 lines

Right. It is more complex than this patch but it should provide a nicer "trap" mechanism to alert driver writers that they are not checking DMA addresses properly.

I will go back and work on improving this patch. I will have to think about how to track overflow buffer triggers as well without impacting the production kernel. That is a separate effort.

-- Shuah

author
#5 lore raw
Quoted 8 lines · Shuah Khan
Quoted 7 lines · Konrad Rzeszutek Wilk
Quoted 3 lines

I did compile a complete list of drivers that don't check dma mapping errors from my analysis. Are you interested in seeing the full analysis?

Yes, plus the authors of the drivers are probably interested in it as well.
..snip..

Work in progress (I have some ways to go) - but if you want to take an early look at it:

http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

-- Shuah

author
#6 lore raw

[RFC] DMA mapping error check analysis

In reply to Shuah Khan

I analyzed all calls to dma_map_single() and dma_map_page() in the kernel, to see if callers check for mapping errors, before using the returned address.

The goal of this analysis is to find drivers that currently do not check dma mapping errors, and fix them.

I documented the results of this analysis:

http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

Please review and give me feedback on the analysis and the proposed next steps.

Thanks,
-- Shuah

#7 lore raw

Re: [RFC] DMA mapping error check analysis

Quoted 7 lines · Shuah Khan

I analyzed all calls to dma_map_single() and dma_map_page() in the kernel, to see if callers check for mapping errors, before using the returned address.

The goal of this analysis is to find drivers that currently do not check dma mapping errors, and fix them.

Quoted 6 lines

I documented the results of this analysis:

http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

Please review and give me feedback on the analysis and the proposed next steps.

Your first table (dma_map_single) lists drivers/usb/core/usb.c and marks it as Bad. This is a mistake because the code is #ifdef'ed out. It hasn't been used in many years; it should be removed.

Alan Stern

author
#8 lore raw

Re: [RFC] DMA mapping error check analysis

Quoted 11 lines · Alan Stern
Quoted 9 lines · Shuah Khan

I analyzed all calls to dma_map_single() and dma_map_page() in the kernel, to see if callers check for mapping errors, before using the returned address.

The goal of this analysis is to find drivers that currently do not check dma mapping errors, and fix them.

I documented the results of this analysis:

Quoted 8 lines
Quoted 4 lines

http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

Please review and give me feedback on the analysis and the proposed next steps.

Your first table (dma_map_single) lists drivers/usb/core/usb.c and marks it as Bad. This is a mistake because the code is #ifdef'ed out. It hasn't been used in many years; it should be removed.

Thanks for catching it. I did note that in my research notes and that was left out by mistake when I put the table together. Table is updated now with your comment and marked it a Cleanup item.

-- Shuah

#9 lore raw

Re: [RFC] DMA mapping error check analysis

In reply to Shuah Khan
Quoted 7 lines · Shuah Khan

I analyzed all calls to dma_map_single() and dma_map_page() in the kernel, to see if callers check for mapping errors, before using the returned address.

The goal of this analysis is to find drivers that currently do not check dma mapping errors, and fix them.

Quoted 7 lines

I documented the results of this analysis:

http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

File Name                  # of calls  Status 	
drivers/firewire/core-iso.c   1        Unmap Broken
drivers/firewire/ohci.c       1        Unmap Broken

In ohci.c, ar_context_release() takes care of cleanup.

In core-iso.c, on failure, the callers are responsible to call fw_iso_buffer_destroy() eventually. (ioctl_create_iso_context() doesn't do this correctly if it's called multiple times.)

Regards,
Clemens

author
#10 lore raw

Re: [RFC] DMA mapping error check analysis

Quoted 8 lines
Quoted 7 lines

http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

File Name                  # of calls  Status 	
drivers/firewire/core-iso.c   1        Unmap Broken
drivers/firewire/ohci.c       1        Unmap Broken
Quoted 6 lines

In ohci.c, ar_context_release() takes care of cleanup.

In core-iso.c, on failure, the callers are responsible to call fw_iso_buffer_destroy() eventually. (ioctl_create_iso_context() doesn't do this correctly if it's called multiple times.)

Thanks. I updated the page with your comments. I moved ohci.c to Good status and left core-iso.c in Unmap Broken in case ioctl_create_iso_context() case is worth fixing.

-- Shuah

#11 lore raw

Re: [RFC] DMA mapping error check analysis

Quoted 10 lines · On Sep 10 Shuah Khan wrote:
Quoted 10 lines
Quoted 7 lines

http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

File Name                  # of calls  Status 	
drivers/firewire/core-iso.c   1        Unmap Broken
drivers/firewire/ohci.c       1        Unmap Broken

In ohci.c, ar_context_release() takes care of cleanup.

Quoted 8 lines
Quoted 4 lines

In core-iso.c, on failure, the callers are responsible to call fw_iso_buffer_destroy() eventually. (ioctl_create_iso_context() doesn't do this correctly if it's called multiple times.)

Thanks. I updated the page with your comments. I moved ohci.c to Good status and left core-iso.c in Unmap Broken in case ioctl_create_iso_context() case is worth fixing.

I don't see what could go wrong if ioctl_create_iso_context() is called multiple times. But I wrote the current (= v3.5-rc1) serialization code in it, hence am blind for mistakes which are my own. So anyboy who spots an actual problem please describe it, or even better send a patch.

(Hmm, fw_device_op_mmap()'s fail: path is executed outside the client->lock protected section. That might be a problem. I need to look further into it.)

Signature · 3 lines
Stefan Richter
-=====-===-- =--= -=-=-
http://arcgraph.de/sr/
#12 lore raw

Re: [RFC] DMA mapping error check analysis

Quoted 13 lines · Stefan Richter
Quoted 12 lines · On Sep 10 Shuah Khan wrote:
Quoted 11 lines
Quoted 5 lines

http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

File Name                  # of calls  Status 	
drivers/firewire/core-iso.c   1        Unmap Broken
drivers/firewire/ohci.c       1        Unmap Broken

In ohci.c, ar_context_release() takes care of cleanup.

In core-iso.c, on failure, the callers are responsible to call fw_iso_buffer_destroy() eventually. (ioctl_create_iso_context() doesn't do this correctly if it's called multiple times.)

Quoted 6 lines
Quoted 3 lines

Thanks. I updated the page with your comments. I moved ohci.c to Good status and left core-iso.c in Unmap Broken in case ioctl_create_iso_context() case is worth fixing.

I don't see what could go wrong if ioctl_create_iso_context() is called multiple times.

fw_iso_buffer_map_dma() maps as many pages as it can, and saves in ->page_count_mapped how many pages need unmapping.

When fw_iso_buffer_map_dma() fails, ioctl_create_iso_context() does _not_ call fw_iso_buffer_destroy() but takes care to not change the cdev's state in any other way. So ioctl_create_iso_context() can be called again and will then call fw_iso_buffer_map_dma(), which will happily map the pages a second time, overwriting the previous mapped addresses.

Regards,
Clemens

#13 lore raw

Re: [RFC] DMA mapping error check analysis

Quoted 8 lines · On Sep 10 Clemens Ladisch wrote:

fw_iso_buffer_map_dma() maps as many pages as it can, and saves in ->page_count_mapped how many pages need unmapping.

When fw_iso_buffer_map_dma() fails, ioctl_create_iso_context() does _not_ call fw_iso_buffer_destroy() but takes care to not change the cdev's state in any other way. So ioctl_create_iso_context() can be called again and will then call fw_iso_buffer_map_dma(), which will happily map the pages a second time, overwriting the previous mapped addresses.

Indeed; thank you. I make a note to fix this when I get some time.

Signature · 3 lines
Stefan Richter
-=====-===-- =--= -=-=-
http://arcgraph.de/sr/