netdev - Re: [PATCHv2 net-next 05/16] net: mvpp2: introduce PPv2.2 HW descriptors and adapt accessors

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]

Message-ID: <113811b6-79a4-9c66-d302-add9fb0c5b1a@arm.com>
Date:   Fri, 6 Jan 2017 14:44:56 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
Cc:     Russell King - ARM Linux <linux@...linux.org.uk>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, Yehuda Yitschak <yehuday@...vell.com>,
        Jason Cooper <jason@...edaemon.net>,
        Pawel Moll <pawel.moll@....com>,
        Ian Campbell <ijc+devicetree@...lion.org.uk>,
        netdev@...r.kernel.org, Hanna Hawa <hannah@...vell.com>,
        Nadav Haklai <nadavh@...vell.com>,
        Rob Herring <robh+dt@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Kumar Gala <galak@...eaurora.org>,
        Gregory Clement <gregory.clement@...e-electrons.com>,
        Stefan Chulski <stefanc@...vell.com>,
        Marcin Wojtas <mw@...ihalf.com>,
        "David S. Miller" <davem@...emloft.net>,
        linux-arm-kernel@...ts.infradead.org,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Subject: Re: [PATCHv2 net-next 05/16] net: mvpp2: introduce PPv2.2 HW
 descriptors and adapt accessors

On 06/01/17 14:29, Russell King - ARM Linux wrote:
> On Wed, Dec 28, 2016 at 05:46:21PM +0100, Thomas Petazzoni wrote:
>> This commit adds the definition of the PPv2.2 HW descriptors, adjusts
>> the mvpp2_tx_desc and mvpp2_rx_desc structures accordingly, and adapts
>> the accessors to work on both PPv2.1 and PPv2.2.
>>
>> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
> ...
>> +		/* On PPv2.2, the situation is more complicated,
>> +		 * because there is only 40 bits to store the virtual
>> +		 * address, which is not sufficient. So on 64 bits
>> +		 * systems, we use phys_to_virt() to get the virtual
>> +		 * address from the physical address, which is fine
>> +		 * because the kernel linear mapping includes the
>> +		 * entire 40 bits physical address space. On 32 bits
>> +		 * systems however, we can't use phys_to_virt(), but
>> +		 * since virtual addresses are 32 bits only, there is
>> +		 * enough space in the RX descriptor for the full
>> +		 * virtual address.
>> +		 */
>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>> +		dma_addr_t dma_addr =
>> +			rx_desc->pp22.buf_phys_addr_key_hash & DMA_BIT_MASK(40);
>> +		phys_addr_t phys_addr =
>> +			dma_to_phys(port->dev->dev.parent, dma_addr);

Ugh, this looks bogus. dma_to_phys(), in the arm64 case at least, is
essentially a SWIOTLB internal helper function which has to be
implemented in architecture code because reasons. Calling it from a
driver is almost certainly wrong (it doesn't even exist on most
architectures). Besides, if this is really a genuine dma_addr_t obtained
from a DMA API call, you cannot infer it to be related to a CPU physical
address, or convertible to one at all.

>> +
>> +		return (unsigned long)phys_to_virt(phys_addr);
>> +#else
>> +		return rx_desc->pp22.buf_cookie_misc & DMA_BIT_MASK(40);
>> +#endif
> 
> I'm not sure that's the best way of selecting the difference.

Given that CONFIG_ARCH_DMA_ADDR_T_64BIT could be enabled on 32-bit LPAE
systems, indeed it definitely isn't.

Robin.

>  It seems
> that the issue here is the size of the virtual address, so why not test
> the size of a virtual address pointer?
> 
> 		if (8 * sizeof(rx_desc) > 40) {
> 			/* do phys addr dance */
> 		} else {
> 			return rx_desc->pp22.buf_cookie_misc & DMA_BIT_MASK(40);
> 		}
> 
> It also means that we get compile coverage over both sides of the
> conditional.
>