Re: [RFC] DMA mapping error check analysis
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/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
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 1Unmap Broken drivers/firewire/ohci.c 1Unmap 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
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 1Unmap Broken > > > drivers/firewire/ohci.c 1Unmap 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/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
> > > > http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis > > > File Name # of calls Status > > drivers/firewire/core-iso.c 1Unmap Broken > > drivers/firewire/ohci.c 1Unmap 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
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 1Unmap Broken > drivers/firewire/ohci.c 1Unmap 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
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 1Unmap Broken drivers/firewire/ohci.c 1Unmap 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis File Name # of calls Status drivers/firewire/core-iso.c 1Unmap Broken drivers/firewire/ohci.c 1Unmap 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
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 1Unmap Broken drivers/firewire/ohci.c 1Unmap 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/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
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 1Unmap Broken drivers/firewire/ohci.c 1Unmap 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
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/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC] DMA mapping error check analysis
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC] DMA mapping error check analysis
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] DMA mapping error check analysis
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
