[PATCHv3 1/2] arm64: Check for NULL device before getting the coherent_dma_mask

[Laura Abbott]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation