netdev - Re: [PATCH] net: ep93xx_eth: drop GFP

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: <20110529103825.GZ24876@n2100.arm.linux.org.uk>
Date:	Sun, 29 May 2011 11:38:25 +0100
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Mika Westerberg <mika.westerberg@....fi>
Cc:	netdev@...r.kernel.org, hsweeten@...ionengravers.com,
	ryan@...ewatersys.com, kernel@...tstofly.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] net: ep93xx_eth: drop GFP_DMA from memory allocations

On Sun, May 29, 2011 at 12:03:57PM +0300, Mika Westerberg wrote:
> diff --git a/drivers/net/arm/ep93xx_eth.c b/drivers/net/arm/ep93xx_eth.c
> index 5a77001..e495f76 100644
> --- a/drivers/net/arm/ep93xx_eth.c
> +++ b/drivers/net/arm/ep93xx_eth.c
> @@ -494,7 +494,7 @@ static int ep93xx_alloc_buffers(struct ep93xx_priv *ep)
>  	int i;
>  
>  	ep->descs = dma_alloc_coherent(NULL, sizeof(struct ep93xx_descs),
> -				&ep->descs_dma_addr, GFP_KERNEL | GFP_DMA);
> +				&ep->descs_dma_addr, GFP_KERNEL);

This one is correct anyway - whether the allocation comes from the DMA
zone or not is something which should be controlled by the DMA mask and
not the driver passing random GFP flags to dma_alloc_coherent.

However, because it is passing a NULL device to dma_alloc_coherent(), it
assumes that it is for an old ISA device, and so will continue to perform
a GFP_DMA allocation.  The solution - pass a struct device and set the
DMA masks correctly.

> @@ -525,7 +525,7 @@ static int ep93xx_alloc_buffers(struct ep93xx_priv *ep)
>  		void *page;
>  		dma_addr_t d;
>  
> -		page = (void *)__get_free_page(GFP_KERNEL | GFP_DMA);
> +		page = (void *)__get_free_page(GFP_KERNEL);
>  		if (page == NULL)
>  			goto err;
>  

Wow, just looked at what this driver is doing with the DMA buffer handling,
and I'm wondering how come its soo broken.

So, to summarize what its doing:

1. It allocates buffers for rx and tx.
2. It maps them with dma_map_single().
	This transfers ownership of the buffer to the DMA device.
3. In ep93xx_xmit,
3a. It copies the data into the buffer with skb_copy_and_csum_dev()
	This violates the DMA buffer ownership rules - the CPU should
	not be writing to this buffer while it is (in principle) owned
	by the DMA device.
3b. It then calls dma_sync_single_for_cpu() for the buffer.
	This transfers ownership of the buffer to the CPU, which surely
	is the wrong direction.
4. In ep93xx_rx,
4a. It calls dma_sync_single_for_cpu() for the buffer.
	This at least transfers the DMA buffer ownership to the CPU
	before the CPU reads the buffer
4b. It then uses skb_copy_to_linear_data() to copy the data out.
	At no point does it transfer ownership back to the DMA device.
5. When the driver is removed, it dma_unmap_single()'s the buffer.
	This transfers ownership of the buffer to the CPU.
6. It frees the buffer.

While it may work on ep93xx, it's not respecting the DMA API rules,
and with DMA debugging enabled it will probably encounter quite a few
warnings.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html