dma-debug: Add dma map/unmap error tracking support
Subsystems (2)
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
@@ -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
@@ -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
Quoted 4 lines · Shuah Khan
A recent dma mapping error analysis effort showed that a large precentage of
dma_map_single()anddma_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?Signed-off-by Shuah Khan <[email protected]>--- Documentation/DMA-API.txt | 7 +++++++ lib/dma-debug.c | 26 +++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-)Documentation/DMA-API.txt +7
-0@@ -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 tolib/dma-debug.c +20
-0@@ -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
-0entry =dma_entry_alloc ();if (!entry)Signature · 1 line
1.7.9.5
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()anddma_map_page()returns are not checked for mapping errors. Reference: https://lkml.org/lkml/2012/8/10/326So 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?Signed-off-by Shuah Khan <[email protected]>--- Documentation/DMA-API.txt | 7 +++++++ lib/dma-debug.c | 26 +++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-)Documentation/DMA-API.txt +7
-0@@ -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 tolib/dma-debug.c +20
-0@@ -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_pagefailed (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 ifdma_mapping_erroron that specificdma_addrit would decrement it. If it never occured, then we would print on the unmap that the device never had calleddma_mapping_erroron saiddma_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
-0entry =dma_entry_alloc ();if (!entry)Signature · 1 line
1.7.9.5
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()anddma_map_page()returns are not checked for mapping errors. Reference: https://lkml.org/lkml/2012/8/10/326So 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 ifdma_mapping_erroron that specificdma_addrit would decrement it. If it never occured, then we would print on the unmap that the device never had calleddma_mapping_erroron saiddma_addr?That is a good idea. Let me see if I understand what you are saying correctly. Add a new field to
dma_debug_entrystructure and keep state and clear it ifdma_mapping_error()is called. This will require adding a debug interface fordma_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.
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()anddma_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 ifdma_mapping_erroron that specificdma_addrit would decrement it. If it never occured, then we would print on the unmap that the device never had calleddma_mapping_erroron saiddma_addr?That is a good idea. Let me see if I understand what you are saying correctly. Add a new field to
dma_debug_entrystructure and keep state and clear it ifdma_mapping_error()is called. This will require adding a debug interface fordma_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
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
[RFC] DMA mapping error check analysis
In reply to Shuah KhanI 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
Re: [RFC] DMA mapping error check analysis
Quoted 7 lines · Shuah Khan
I analyzed all calls to
dma_map_single()anddma_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
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()anddma_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) listsdrivers/usb/core/usb.cand 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
Re: [RFC] DMA mapping error check analysis
In reply to Shuah KhanQuoted 7 lines · Shuah Khan
I analyzed all calls to
dma_map_single()anddma_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
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 callfw_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
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 BrokenIn
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 callfw_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.cto Good status and leftcore-iso.cin Unmap Broken in caseioctl_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/
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 BrokenIn
ohci.c,ar_context_release()takes care of cleanup.In
core-iso.c, on failure, the callers are responsible to callfw_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.cto Good status and leftcore-iso.cin Unmap Broken in caseioctl_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
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_ callfw_iso_buffer_destroy()but takes care to not change the cdev's state in any other way. Soioctl_create_iso_context()can be called again and will then callfw_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/