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

[lauraa@codeaurora.org (Laura Abbott)]

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


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