linux-kernel - Re: [PATCH 4/5] dma-direct: implement complete bus_dma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 Sep 2018 17:14:56 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Christoph Hellwig <hch@....de>
Cc:     iommu@...ts.linux-foundation.org,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH 4/5] dma-direct: implement complete bus_dma_mask handling

On 27/09/18 16:32, Christoph Hellwig wrote:
> On Thu, Sep 27, 2018 at 03:58:04PM +0100, Robin Murphy wrote:
>>>    }
>>>    #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */
>>>    diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
>>> index 3c404e33d946..64466b7ef67b 100644
>>> --- a/kernel/dma/direct.c
>>> +++ b/kernel/dma/direct.c
>>> @@ -43,10 +43,11 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size,
>>>    			return false;
>>>    		}
>>>    -		if (*dev->dma_mask >= DMA_BIT_MASK(32)) {
>>> +		if (*dev->dma_mask >= DMA_BIT_MASK(32) || dev->bus_dma_mask) {
>>
>> Hmm... say *dev->dma_mask is 31 bits and dev->bus_dma_mask is 40 bits due
>> to a global DT property, we'll now scream where we didn't before even
>> though the bus mask is almost certainly irrelevant - is that desirable?
> 
> This is just the reporting in the error case - we'll only hit this
> IFF dma_capable already returned false.  But if you don't want a message
> here we can probably drop this hunk.

It was only a question of consistency - whatever the reason was to avoid 
warning for small device masks before, it's not obvious why it wouldn't 
still apply in the presence of nonzero but larger bus mask. The fact 
that the current check is there at all implied to me that we're 
expecting less-capable device to hit this often and thus wanted to avoid 
being noisy. If that's not the case then it's fine as-is AFAIC.

>>> @@ -65,12 +66,18 @@ u64 dma_direct_get_required_mask(struct device *dev)
>>>    {
>>>    	u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT);
>>>    +	if (dev->bus_dma_mask && dev->bus_dma_mask < max_dma)
>>> +		max_dma = dev->bus_dma_mask;
>>
>> Again, I think we could just do another min_not_zero() here. A device wired
>> to address only one single page of RAM isn't a realistic prospect (and we
>> could just flip the -1 and the shift in the max_dma calculation if we
>> *really* wanted to support such things).
> 
> This just seemed more readable to me than min_not_zero, but if others
> prefer min_not_zero I can switch.

Nah, just checking whether there were any intentionally different 
assumptions compared to the couple of other places in the patch where 
min_not_zero() *is* used. If it's purely a style thing then no worries 
(personally I'd have written it yet another way anyway).

Robin.