build_skb() and data corruption

build_skb() and data corruption

[Jonas Jensen]

Please help,

I think I see memory corruption with a driver recently added to linux-next.

The following error occur downloading a large file with wget (or
ncftp): "read error: Bad address"
wget exits and leaves the file unfinished.

The error goes away when build_skb() is patched out, in this case by
allocating pages directly in RX loop.

I currently have two branches with code placed under ifdef USE_BUILD_SKB:
https://bitbucket.org/Kasreyn/linux-next/src/faa7c408a655fdfd7c383f79259031da5a05d61e/drivers/net/ethernet/moxa/moxart_ether.c#cl-472

If build_skb() is the cause, is there something the driver can do about it?

A quick search on "build_skb memory corruption" reveals the following
commit, "igb: Revert support for build_skb in igb"
f9d40f6a9921cc7d9385f64362314054e22152bd:

"The reason for reverting this patch is that it can lead to data corruption.
The following flow was pointed out by Ben Hutchings:
1. skb is forwarded to another device
2. Packet headers are modified and it's put into a queue
3. Second packet is received into the other half of this page
4. Page cannot be reused, so is DMA-unmapped
5. The DMA mapping was non-coherent, so unmap copies or invalidates
cache
The headers added in step 2 get trashed in step 5."


This is extra interesting, errors only happen on a locally mounted
ext3 filesystem, never on tmpfs e.g.:

# mount
/dev/mmcblk0p1 on / type ext3
(rw,relatime,errors=continue,barrier=1,data=ordered)
tmpfs on /dev/shm type tmpfs (rw,relatime,mode=777)
tmpfs on /tmp type tmpfs (rw,relatime)

#cd /tmp
# wget -c ftp://149.20.4.69/pub/linux/kernel/v2.6/linux-2.6.11.11.tar.gz
Connecting to 149.20.4.69 (149.20.4.69:21)
linux-2.6.11.11.tar.  25% |*******                        | 11374k
0:01:36 ETAwget: short write
[  153.560000] wget (383) used greatest stack depth: 4776 bytes left
# rm linux-2.6.11.11.tar.gz
# wget -c ftp://149.20.4.69/pub/linux/kernel/v2.6/linux-2.6.11.11.tar.gz
Connecting to 149.20.4.69 (149.20.4.69:21)
linux-2.6.11.11.tar.  25% |*******                        | 11315k
0:01:34 ETAwget: short write
# rm linux-2.6.11.11.tar.gz
# wget -c ftp://149.20.4.69/pub/linux/kernel/v2.6/linux-2.6.11.11.tar.gz
Connecting to 149.20.4.69 (149.20.4.69:21)
linux-2.6.11.11.tar.  25% |*******                        | 11473k
0:01:38 ETAwget: short write
[  237.300000] wget (387) used greatest stack depth: 4752 bytes left

# cd /root/
# wget -c ftp://149.20.4.69/pub/linux/kernel/v2.6/linux-2.6.11.11.tar.gz
Connecting to 149.20.4.69 (149.20.4.69:21)
linux-2.6.11.11.tar.   3% |*                              |  1647k  0:03:02 ETA
wget: read error: Bad address


Regards,
Jonas

Re: build_skb() and data corruption

[Arnd Bergmann]

On Monday 13 January 2014, Jonas Jensen wrote:
> Please help,
> 
> I think I see memory corruption with a driver recently added to linux-next.
> 
> The following error occur downloading a large file with wget (or
> ncftp): "read error: Bad address"
> wget exits and leaves the file unfinished.
> 
> The error goes away when build_skb() is patched out, in this case by
> allocating pages directly in RX loop.
> 
> I currently have two branches with code placed under ifdef USE_BUILD_SKB:
> https://bitbucket.org/Kasreyn/linux-next/src/faa7c408a655fdfd7c383f79259031da5a05d61e/drivers/net/ethernet/moxa/moxart_ether.c#cl-472
> 

The problem appears to be incorrect coherency management of the
dma buffers. You are using the streaming DMA mapping API on a
platform that is not cache coherent, and you are reusing buffers.

The solution to this problem is to add the appropriate
dma_sync_single_for_device() and dma_sync_single_for_cpu()
calls at the point where buffer ownership changes between the
dma master and the CPU. This will flush the caches after the
filling them on the tx path and invalidate caches when the
dma master fills them on rx.

See Documentation/DMA-API-HOWTO.txt for details.

	Arnd

Re: build_skb() and data corruption

[Ben Hutchings]

On Mon, 2014-01-13 at 12:47 +0100, Jonas Jensen wrote:
> Please help,
> 
> I think I see memory corruption with a driver recently added to linux-next.
> 
> The following error occur downloading a large file with wget (or
> ncftp): "read error: Bad address"
> wget exits and leaves the file unfinished.
> 
> The error goes away when build_skb() is patched out, in this case by
> allocating pages directly in RX loop.

The problem is you're assuming build_skb() does rather more than it
does.

> I currently have two branches with code placed under ifdef USE_BUILD_SKB:
> https://bitbucket.org/Kasreyn/linux-next/src/faa7c408a655fdfd7c383f79259031da5a05d61e/drivers/net/ethernet/moxa/moxart_ether.c#cl-472

Quoting from there:

> #define USE_BUILD_SKB
> #ifdef USE_BUILD_SKB
> 
> 
> 	skb = build_skb(page_address(priv->rx_pages[priv->rx_head]), priv->rx_buf_size);
> 
> 
> 	if (unlikely(!skb)) {
> 		net_info_ratelimited("RX build_skb failed\n");

Don't log; increment the rx_dropped counter.

> 		moxart_rx_drop_packet(ndev, desc0);
> 		return true;
> 	}
>
> 
> 	if (likely(skb))

Redundant condition.

> 		get_page(priv->rx_pages[priv->rx_head]);
> 
> 
> 	skb_put(skb, len);
> 
> 
> #else
> 
> 
> 	dma_unmap_page(&ndev->dev, priv->rx_buf_phys[priv->rx_head], RX_BUF_SIZE, DMA_FROM_DEVICE);

dma_unmap_page() is always needed, so move this above the #ifdef
USE_BUILD_SKB.

> 
> 	skb = netdev_alloc_skb_ip_align(ndev, 128);

Missing check for NULL.

> 	skb_fill_page_desc(skb, 0, priv->rx_pages[priv->rx_head], 0, len);
> 	skb->len += len;
> 	skb->data_len += len;
> 
> 
> 	if (len > 128) {
> 		skb->truesize += PAGE_SIZE;
> 		__pskb_pull_tail(skb, ETH_HLEN);
> 	} else {
> 		__pskb_pull_tail(skb, len);
> 	}
> 
> 
> 	p = priv->rx_pages[priv->rx_head];

Missing check for NULL.

> 	moxart_alloc_rx_page(ndev, priv->rx_head);

New buffer is always required, so move this under the #endif... well,
except that if moxart_alloc_rx_page() fails then things go horribly
wrong as you have.

Best practice is to try allocating the next buffer before passing up the
current one; if that fails then you recycle the current buffer and
increment rx_dropped.

> 
> 
> #endif
[end quoted code]

> If build_skb() is the cause, is there something the driver can do about it?
> 
> A quick search on "build_skb memory corruption" reveals the following
> commit, "igb: Revert support for build_skb in igb"
> f9d40f6a9921cc7d9385f64362314054e22152bd:
[...]

The problem I identified in igb occurs only when splitting a page into
multiple buffers (or when recycling DMA-mapped pages).  build_skb() is
fine to use whenever each RX buffer is a single page (except that that's
memory-inefficient) that's unmapped on completion.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: build_skb() and data corruption

[Jonas Jensen]

Thanks for the replies, you led me to a new solution,


I now think build_skb() is not the right choice, my motivation for
using it in the first place, that I thought it meant getting away with
not copying memory.

build_skb() is replaced by netdev_alloc_skb_ip_align() and memcpy()
(derived from drivers/net/ethernet/realtek/r8169.c).

Read errors are gone, even without syncing DMA. Is it a good idea to
do it anyway, i.e. leave calls to dma_sync_single_* in?

Also, without build_skb(), kmalloc memory can be used (frag_size > 0),
memory is used more efficiently, at least half of a page times 64
previously wasted.

New code here:
https://bitbucket.org/Kasreyn/linux-next/src/92f5aeb3b58f8d8dffd9178b6b5ff46217156caa/drivers/net/ethernet/moxa/moxart_ether.c#cl-424


Thanks,
Jonas

Re: build_skb() and data corruption

[Arnd Bergmann]

On Tuesday 14 January 2014, Jonas Jensen wrote:
> Thanks for the replies, you led me to a new solution,
> 
> 
> I now think build_skb() is not the right choice, my motivation for
> using it in the first place, that I thought it meant getting away with
> not copying memory.
> 
> build_skb() is replaced by netdev_alloc_skb_ip_align() and memcpy()
> (derived from drivers/net/ethernet/realtek/r8169.c).
> 
> Read errors are gone, even without syncing DMA. Is it a good idea to
> do it anyway, i.e. leave calls to dma_sync_single_* in?

The calls to dma_sync_single_* in the moxart_rx() function are needed.
The call to arm_dma_ops.sync_single_for_device() in
moxart_mac_setup_desc_ring() is wrong, because the buffer is already
owned by the device at that point (just after dma_map_single), and
because you should use the official dma_* api rather than using
the arm_dma_ops struct.

	Arnd