lkml.org 
[lkml]   [2014]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: build_skb() and data corruption
From
Date
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.



\
 
 \ /
  Last update: 2014-01-13 15:01    [W:0.401 / U:0.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site