netdev - Re: [ath9k-devel] [PATCH v2 07/46] net/wireless: ath9k: fix DMA API usage

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:	Tue, 12 Jul 2011 22:53:16 +0200
From:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:	Ralf Baechle <ralf@...ux-mips.org>
Cc:	Felix Fietkau <nbd@...nwrt.org>, netdev@...r.kernel.org,
	linux-wireless@...r.kernel.org,
	Jouni Malinen <jmalinen@...eros.com>,
	Senthil Balasubramanian <senthilkumar@...eros.com>,
	ath9k-devel@...ema.h4ckr.net,
	Vasanthakumar Thiagarajan <vasanth@...eros.com>
Subject: Re: [ath9k-devel] [PATCH v2 07/46] net/wireless: ath9k: fix DMA
 API usage

On Tue, Jul 12, 2011 at 08:32:04PM +0100, Ralf Baechle wrote:
> On Tue, Jul 12, 2011 at 12:36:06PM +0800, Felix Fietkau wrote:
> 
> > >diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
> > >index 70dc8ec..c5f46d5 100644
> > >--- a/drivers/net/wireless/ath/ath9k/recv.c
> > >+++ b/drivers/net/wireless/ath/ath9k/recv.c
> > >@@ -684,15 +684,11 @@ static bool ath_edma_get_buffers(struct ath_softc *sc,
> > >  	BUG_ON(!bf);
> > >
> > >  	dma_sync_single_for_cpu(sc->dev, bf->bf_buf_addr,
> > >-				common->rx_bufsize, DMA_FROM_DEVICE);
> > >+				common->rx_bufsize, DMA_BIDIRECTIONAL);
> > >
> > >  	ret = ath9k_hw_process_rxdesc_edma(ah, NULL, skb->data);
> > >-	if (ret == -EINPROGRESS) {
> > >-		/*let device gain the buffer again*/
> > >-		dma_sync_single_for_device(sc->dev, bf->bf_buf_addr,
> > >-				common->rx_bufsize, DMA_FROM_DEVICE);
> > >+	if (ret == -EINPROGRESS)
> > >  		return false;
> > >-	}
> > >
> > >  	__skb_unlink(skb,&rx_edma->rx_fifo);
> > >  	if (ret == -EINVAL) {
> > I have strong doubts about this change. On most MIPS devices,
> > dma_sync_single_for_cpu is a no-op, whereas
> > dma_sync_single_for_device flushes the cache range. With this
> > change, the CPU could cache the DMA status part behind skb->data and
> > that cache entry would not be flushed inbetween calls to this
> > functions on the same buffer, likely leading to rx stalls.
> 
> The code was already broken before.  By the time dma_sync_single_for_cpu
> and ath9k_hw_process_rxdesc_edma are called, the DMA engine may still be
> active in the buffer,  yet the driver is looking at it.
> 
> dma_sync_single_for_cpu() is part of changing the buffer ownership from
> the device to the CPU.  When it is being called, DMA into the buffer should
> already have been completed ...  or else the shit may hit the jet engine.

Let's get rid of the "buffer ownership" misunderstanding from the picture.
Ownership is about who is expected to be writing (or is assured the data
is not being changed under his foot). This has nothing to do with DMA API.

DMA API is there for two purposes: to make part of memory visible to
both CPU and device (map/unmap) and to ensure memory consistency in
presence of caches (sync; implicitly done in map/unmap).

In the case we're analysing, the ownership is on the device's side
until it stops writing the buffer. sync_to_cpu doesn't change that.
It only allows the CPU to see more recent data (in case the CPU cached
something earlier).

> Imagine what would happen on a hypothetic cache architecture which does not
> have a dirty bit, that is which would have to write back every cache line -
> even clean lines - to memory in order to evict it.  Corruption.

dma_map_whatever() would mark the memory uncachable on such an architecture.
Otherwise this would violate assumptions on DMA_FROM_DEVICE mappings (or
"device owned buffers") that the CPU does not write to the mapped memory.

Best Regards,
Michał Mirosław
--
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