| Message ID | 1386711816-20270-2-git-send-email-lauraa@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 10, 2013 at 09:43:35PM +0000, Laura Abbott wrote: > The device passed in to dma_alloc may be NULL. Check for this before > trying to get the coherent_dma_mask. > > Cc: Will Deacon <will.deacon@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Laura Abbott <lauraa@codeaurora.org> > --- > arch/arm64/mm/dma-mapping.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c > index 4bd7579..4134212 100644 > --- a/arch/arm64/mm/dma-mapping.c > +++ b/arch/arm64/mm/dma-mapping.c > @@ -33,7 +33,7 @@ static void *arm64_swiotlb_alloc_coherent(struct device *dev, size_t size, > dma_addr_t *dma_handle, gfp_t flags, > struct dma_attrs *attrs) > { > - if (IS_ENABLED(CONFIG_ZONE_DMA32) && > + if (dev && IS_ENABLED(CONFIG_ZONE_DMA32) && > dev->coherent_dma_mask <= DMA_BIT_MASK(32)) > flags |= GFP_DMA32; > return swiotlb_alloc_coherent(dev, size, dma_handle, flags); Unless I'm misreading the code, it looks like there are paths through swiotlb_alloc_coherent that will dereference the dev parameter without a NULL check. Are you sure we should allow for NULL devices here? Will
On 12/11/2013 2:42 AM, Will Deacon wrote: > On Tue, Dec 10, 2013 at 09:43:35PM +0000, Laura Abbott wrote: >> The device passed in to dma_alloc may be NULL. Check for this before >> trying to get the coherent_dma_mask. >> >> Cc: Will Deacon <will.deacon@arm.com> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Marek Szyprowski <m.szyprowski@samsung.com> >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org> >> --- >> arch/arm64/mm/dma-mapping.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c >> index 4bd7579..4134212 100644 >> --- a/arch/arm64/mm/dma-mapping.c >> +++ b/arch/arm64/mm/dma-mapping.c >> @@ -33,7 +33,7 @@ static void *arm64_swiotlb_alloc_coherent(struct device *dev, size_t size, >> dma_addr_t *dma_handle, gfp_t flags, >> struct dma_attrs *attrs) >> { >> - if (IS_ENABLED(CONFIG_ZONE_DMA32) && >> + if (dev && IS_ENABLED(CONFIG_ZONE_DMA32) && >> dev->coherent_dma_mask <= DMA_BIT_MASK(32)) >> flags |= GFP_DMA32; >> return swiotlb_alloc_coherent(dev, size, dma_handle, flags); > > Unless I'm misreading the code, it looks like there are paths through > swiotlb_alloc_coherent that will dereference the dev parameter without a > NULL check. Are you sure we should allow for NULL devices here? > The current ARM code allows for NULL devices so that would be a difference in behavior between arm and arm64. We're also relying on this behavior in some code. Where exactly in swiotlb_alloc_coherent does this dereference happen? The only one I see is checked with 'if (hwdev && hwdev->coherent_dma_mask)' > Will > Thanks, Laura
On Wed, Dec 11, 2013 at 05:48:10PM +0000, Laura Abbott wrote: > On 12/11/2013 2:42 AM, Will Deacon wrote: > > On Tue, Dec 10, 2013 at 09:43:35PM +0000, Laura Abbott wrote: > >> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c > >> index 4bd7579..4134212 100644 > >> --- a/arch/arm64/mm/dma-mapping.c > >> +++ b/arch/arm64/mm/dma-mapping.c > >> @@ -33,7 +33,7 @@ static void *arm64_swiotlb_alloc_coherent(struct device *dev, size_t size, > >> dma_addr_t *dma_handle, gfp_t flags, > >> struct dma_attrs *attrs) > >> { > >> - if (IS_ENABLED(CONFIG_ZONE_DMA32) && > >> + if (dev && IS_ENABLED(CONFIG_ZONE_DMA32) && > >> dev->coherent_dma_mask <= DMA_BIT_MASK(32)) > >> flags |= GFP_DMA32; > >> return swiotlb_alloc_coherent(dev, size, dma_handle, flags); > > > > Unless I'm misreading the code, it looks like there are paths through > > swiotlb_alloc_coherent that will dereference the dev parameter without a > > NULL check. Are you sure we should allow for NULL devices here? > > > > The current ARM code allows for NULL devices so that would be a > difference in behavior between arm and arm64. We're also relying on this > behavior in some code. Where exactly in swiotlb_alloc_coherent does this > dereference happen? The only one I see is checked with 'if (hwdev && > hwdev->coherent_dma_mask)' phys_to_dma could, but doesn't. The one I spotted was buried down in: map_single -> swiotlb_tbl_map_single -> dma_get_seg_boundary Will
On 12/11/2013 9:51 AM, Will Deacon wrote: > On Wed, Dec 11, 2013 at 05:48:10PM +0000, Laura Abbott wrote: >> On 12/11/2013 2:42 AM, Will Deacon wrote: >>> On Tue, Dec 10, 2013 at 09:43:35PM +0000, Laura Abbott wrote: >>>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c >>>> index 4bd7579..4134212 100644 >>>> --- a/arch/arm64/mm/dma-mapping.c >>>> +++ b/arch/arm64/mm/dma-mapping.c >>>> @@ -33,7 +33,7 @@ static void *arm64_swiotlb_alloc_coherent(struct device *dev, size_t size, >>>> dma_addr_t *dma_handle, gfp_t flags, >>>> struct dma_attrs *attrs) >>>> { >>>> - if (IS_ENABLED(CONFIG_ZONE_DMA32) && >>>> + if (dev && IS_ENABLED(CONFIG_ZONE_DMA32) && >>>> dev->coherent_dma_mask <= DMA_BIT_MASK(32)) >>>> flags |= GFP_DMA32; >>>> return swiotlb_alloc_coherent(dev, size, dma_handle, flags); >>> >>> Unless I'm misreading the code, it looks like there are paths through >>> swiotlb_alloc_coherent that will dereference the dev parameter without a >>> NULL check. Are you sure we should allow for NULL devices here? >>> >> >> The current ARM code allows for NULL devices so that would be a >> difference in behavior between arm and arm64. We're also relying on this >> behavior in some code. Where exactly in swiotlb_alloc_coherent does this >> dereference happen? The only one I see is checked with 'if (hwdev && >> hwdev->coherent_dma_mask)' > > phys_to_dma could, but doesn't. The one I spotted was buried down in: > > map_single -> swiotlb_tbl_map_single -> dma_get_seg_boundary > Ah yes I see that now. I guess the question still stands though, should we fixup the parts of the code to fully support the NULL devices or do we tell clients to fix their code and fail the allocation with a warning not to pass NULL? > Will > Laura
On Wed, Dec 11, 2013 at 06:10:59PM +0000, Laura Abbott wrote: > On 12/11/2013 9:51 AM, Will Deacon wrote: > > On Wed, Dec 11, 2013 at 05:48:10PM +0000, Laura Abbott wrote: > >> On 12/11/2013 2:42 AM, Will Deacon wrote: > >>> On Tue, Dec 10, 2013 at 09:43:35PM +0000, Laura Abbott wrote: > >>>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c > >>>> index 4bd7579..4134212 100644 > >>>> --- a/arch/arm64/mm/dma-mapping.c > >>>> +++ b/arch/arm64/mm/dma-mapping.c > >>>> @@ -33,7 +33,7 @@ static void *arm64_swiotlb_alloc_coherent(struct device *dev, size_t size, > >>>> dma_addr_t *dma_handle, gfp_t flags, > >>>> struct dma_attrs *attrs) > >>>> { > >>>> - if (IS_ENABLED(CONFIG_ZONE_DMA32) && > >>>> + if (dev && IS_ENABLED(CONFIG_ZONE_DMA32) && > >>>> dev->coherent_dma_mask <= DMA_BIT_MASK(32)) > >>>> flags |= GFP_DMA32; > >>>> return swiotlb_alloc_coherent(dev, size, dma_handle, flags); > >>> > >>> Unless I'm misreading the code, it looks like there are paths through > >>> swiotlb_alloc_coherent that will dereference the dev parameter without a > >>> NULL check. Are you sure we should allow for NULL devices here? > >>> > >> > >> The current ARM code allows for NULL devices so that would be a > >> difference in behavior between arm and arm64. We're also relying on this > >> behavior in some code. Where exactly in swiotlb_alloc_coherent does this > >> dereference happen? The only one I see is checked with 'if (hwdev && > >> hwdev->coherent_dma_mask)' > > > > phys_to_dma could, but doesn't. The one I spotted was buried down in: > > > > map_single -> swiotlb_tbl_map_single -> dma_get_seg_boundary > > > > Ah yes I see that now. > > I guess the question still stands though, should we fixup the parts of > the code to fully support the NULL devices or do we tell clients to fix > their code and fail the allocation with a warning not to pass NULL? Assuming it's always possible to pass in a non-NULL device, I'd be tempted to fix the callers. Can you think of a valid use-case where the caller doesn't have a device handle to pass in? Will
On 12/12/2013 4:18 AM, Will Deacon wrote: > On Wed, Dec 11, 2013 at 06:10:59PM +0000, Laura Abbott wrote: >> On 12/11/2013 9:51 AM, Will Deacon wrote: >>> On Wed, Dec 11, 2013 at 05:48:10PM +0000, Laura Abbott wrote: >>>> On 12/11/2013 2:42 AM, Will Deacon wrote: >>>>> On Tue, Dec 10, 2013 at 09:43:35PM +0000, Laura Abbott wrote: >>>>>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c >>>>>> index 4bd7579..4134212 100644 >>>>>> --- a/arch/arm64/mm/dma-mapping.c >>>>>> +++ b/arch/arm64/mm/dma-mapping.c >>>>>> @@ -33,7 +33,7 @@ static void *arm64_swiotlb_alloc_coherent(struct device *dev, size_t size, >>>>>> dma_addr_t *dma_handle, gfp_t flags, >>>>>> struct dma_attrs *attrs) >>>>>> { >>>>>> - if (IS_ENABLED(CONFIG_ZONE_DMA32) && >>>>>> + if (dev && IS_ENABLED(CONFIG_ZONE_DMA32) && >>>>>> dev->coherent_dma_mask <= DMA_BIT_MASK(32)) >>>>>> flags |= GFP_DMA32; >>>>>> return swiotlb_alloc_coherent(dev, size, dma_handle, flags); >>>>> >>>>> Unless I'm misreading the code, it looks like there are paths through >>>>> swiotlb_alloc_coherent that will dereference the dev parameter without a >>>>> NULL check. Are you sure we should allow for NULL devices here? >>>>> >>>> >>>> The current ARM code allows for NULL devices so that would be a >>>> difference in behavior between arm and arm64. We're also relying on this >>>> behavior in some code. Where exactly in swiotlb_alloc_coherent does this >>>> dereference happen? The only one I see is checked with 'if (hwdev && >>>> hwdev->coherent_dma_mask)' >>> >>> phys_to_dma could, but doesn't. The one I spotted was buried down in: >>> >>> map_single -> swiotlb_tbl_map_single -> dma_get_seg_boundary >>> >> >> Ah yes I see that now. >> >> I guess the question still stands though, should we fixup the parts of >> the code to fully support the NULL devices or do we tell clients to fix >> their code and fail the allocation with a warning not to pass NULL? > > Assuming it's always possible to pass in a non-NULL device, I'd be tempted > to fix the callers. Can you think of a valid use-case where the caller > doesn't have a device handle to pass in? > Depends on your definition of 'valid' ;) The only cases we've been able to come up with are places where we've been abusing the DMA APIs slightly to get chunks of contiguous memory through CMA for miscellaneous debugging/testing purposes. I only stumbled across this problem at all while writing a quick test case for CMA. I'd argue that if there is a real use case for non-device large contiguous allocations there should be a new dedicated API that doesn't break the dma abstraction. I suggest for now we add a big WARN when a NULL device is passed in and see how many people complain. Laura
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index 4bd7579..4134212 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -33,7 +33,7 @@ static void *arm64_swiotlb_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle, gfp_t flags, struct dma_attrs *attrs) { - if (IS_ENABLED(CONFIG_ZONE_DMA32) && + if (dev && IS_ENABLED(CONFIG_ZONE_DMA32) && dev->coherent_dma_mask <= DMA_BIT_MASK(32)) flags |= GFP_DMA32; return swiotlb_alloc_coherent(dev, size, dma_handle, flags);
The device passed in to dma_alloc may be NULL. Check for this before trying to get the coherent_dma_mask. Cc: Will Deacon <will.deacon@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Marek Szyprowski <m.szyprowski@samsung.com> Signed-off-by: Laura Abbott <lauraa@codeaurora.org> --- arch/arm64/mm/dma-mapping.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)