Re: build_skb() and data corruption
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: build_skb() and data corruption
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: build_skb() and data corruption
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: build_skb() and data corruption
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: build_skb() and data corruption
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(>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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: build_skb() and data corruption
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
build_skb() and data corruption
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.56] 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.30] 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
build_skb() and data corruption
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.56] 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.30] 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: build_skb() and data corruption
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: build_skb() and data corruption
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
