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

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

[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

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 <shuah.khan@hp.com>
---
 Documentation/DMA-API.txt |    7 +++++++
 lib/dma-debug.c           |   26 +++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index 66bd97a..ee10a11 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -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
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 66ce414..8596114 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -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)
-- 
1.7.9.5

Re: [PATCH] dma-debug: Add dma map/unmap error tracking support

[Konrad Rzeszutek Wilk]

On Sun, Sep 02, 2012 at 08:14:17AM -0600, Shuah Khan wrote:
> 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?

> 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 <shuah.khan@hp.com>
> ---
>  Documentation/DMA-API.txt |    7 +++++++
>  lib/dma-debug.c           |   26 +++++++++++++++++++++++++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
> index 66bd97a..ee10a11 100644
> --- a/Documentation/DMA-API.txt
> +++ b/Documentation/DMA-API.txt
> @@ -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
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index 66ce414..8596114 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -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;
> +	}

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?

>  
>  	entry = dma_entry_alloc();
>  	if (!entry)
> -- 
> 1.7.9.5
> 
> 

Re: [PATCH] dma-debug: Add dma map/unmap error tracking support

[Shuah Khan]

On Tue, 2012-09-04 at 17:05 -0400, Konrad Rzeszutek Wilk wrote:
> On Sun, Sep 02, 2012 at 08:14:17AM -0600, Shuah Khan wrote:
> > 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?

> 
> > 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 <shuah.khan@hp.com>
> > ---
> >  Documentation/DMA-API.txt |    7 +++++++
> >  lib/dma-debug.c           |   26 +++++++++++++++++++++++++-
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
> > index 66bd97a..ee10a11 100644
> > --- a/Documentation/DMA-API.txt
> > +++ b/Documentation/DMA-API.txt
> > @@ -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
> > diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> > index 66ce414..8596114 100644
> > --- a/lib/dma-debug.c
> > +++ b/lib/dma-debug.c
> > @@ -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;
> > +	}
> 
> So this will print if the dma_map_page failed (which can happen).

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

> 
> 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?

> 
> >  
> >  	entry = dma_entry_alloc();
> >  	if (!entry)
> > -- 
> > 1.7.9.5
> > 
> > 

Re: [PATCH] dma-debug: Add dma map/unmap error tracking support

[Konrad Rzeszutek Wilk]

On Tue, Sep 04, 2012 at 04:57:56PM -0600, Shuah Khan wrote:
> On Tue, 2012-09-04 at 17:05 -0400, Konrad Rzeszutek Wilk wrote:
> > On Sun, Sep 02, 2012 at 08:14:17AM -0600, Shuah Khan wrote:
> > > 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..
> > 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.

Re: [PATCH] dma-debug: Add dma map/unmap error tracking support

[Shuah Khan]

On Wed, 2012-09-05 at 07:57 -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Sep 04, 2012 at 04:57:56PM -0600, Shuah Khan wrote:
> > On Tue, 2012-09-04 at 17:05 -0400, Konrad Rzeszutek Wilk wrote:
> > > On Sun, Sep 02, 2012 at 08:14:17AM -0600, Shuah Khan wrote:
> > > > 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..

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?

> > > 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.

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

Re: [PATCH] dma-debug: Add dma map/unmap error tracking support

[Shuah Khan]

On Wed, 2012-09-05 at 08:34 -0600, Shuah Khan wrote:
> On Wed, 2012-09-05 at 07:57 -0400, Konrad Rzeszutek Wilk wrote:
> > 
> > > 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

[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

Re: [RFC] DMA mapping error check analysis

[Alan Stern]

On Fri, 7 Sep 2012, Shuah Khan wrote:

> 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.

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

[Shuah Khan]

On Fri, 2012-09-07 at 12:20 -0400, Alan Stern wrote:
> On Fri, 7 Sep 2012, Shuah Khan wrote:
> 
> > 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.
> 
> 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

Re: [RFC] DMA mapping error check analysis

[Clemens Ladisch]

Shuah Khan wrote:
> 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

> 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

[Shuah Khan]

> >
> > 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.)
> 

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

[Stefan Richter]

On Sep 10 Shuah Khan wrote:
> 
> > >
> > > 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.)
> > 
> 
> 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.)
-- 
Stefan Richter
-=====-===-- =--= -=-=-
http://arcgraph.de/sr/

Re: [RFC] DMA mapping error check analysis

[Clemens Ladisch]

Stefan Richter wrote:
> On Sep 10 Shuah Khan wrote:
>>>> 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.)
>>
>> 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

Re: [RFC] DMA mapping error check analysis

[Stefan Richter]

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.
-- 
Stefan Richter
-=====-===-- =--= -=-=-
http://arcgraph.de/sr/