[#3870] bfin_mac: RX from DMA to skb payload bug if cache policy is WB

Submitted By: Bryan Wu

Open Date

2008-01-28 21:16:35 Close Date

2008-01-31 00:32:52

Priority:

Medium Assignee:

Bryan Wu

Status:

Closed Fixed In Release:

N/A

Found In Release:

N/A Release:

Category:

N/A Board:

N/A

Processor:

N/A Silicon Revision:

Is this bug repeatable?:

Yes Resolution:

Fixed

Uboot version or rev.:

Toolchain version or rev.:

App binary format:

N/A

Summary: bfin_mac: RX from DMA to skb payload bug if cache policy is WB

Details:

Reported by: Alexey Demin <bf53x@ya.ru> in the forum

Hi all!

I guess there is a bug in the receiving path of bfin_mac driver when cache policy is WriteBack.

DMA Rx use skb payload area to receive ethernet packets.

When skb has been destroyed with kfree_skb, the kernel can reuse this payload area

for another packet.

If this payload has been changed(for example, ip forwarding with ttl decrement)

the changes can be stored in cache.

When dma transfers data of new packet to the payload area(skb->data) and

the cache of deleted packet with the same payload area flushes its changes simultaneously, the data in payload of new packet can be replaced with data of old packet.

So we must invalidate cache of payload before dma will be able

to send new data to this payload.

Here is the diffs with checking for bug and its fixing.

Checking bug presence:

uncomment #define CHECK_CACHE_BUG

make sure CONFIG_BKFIN_WB cache policy is ON.

ping -i 0 or ping -f from any host to this box.

Fixing bug:

uncomment #define FIX_CACHE_BUG"

With best regards, Lex.

==================================

--- bfin_mac.c&revision=4101 2008-01-21 20:17:20.000000000 +0300

+++ bfin_mac.c&revision=4101_fixed 2008-01-25 14:54:21.000000000 +0300

@@ -636,6 +636,17 @@

return 0;

}

+#define FIX_CACHE_BUG

+#define CHECK_CACHE_BUG

+#if defined(FIX_CACHE_BUG) && defined(CONFIG_BLKFIN_WT)

+#error There is nothing to fix when write through (WT) cache policy is ON

+#endif

+#ifdef CHECK_CACHE_BUG

+#include <linux/ip.h> /* struct iphdr */

+#endif

static void bf537mac_rx(struct net_device *dev)

{

struct sk_buff *skb, *new_skb;

@@ -653,6 +664,14 @@

}

/* reserve 2 bytes for RXDWA padding */

skb_reserve(new_skb, 2);

+#ifdef FIX_CACHE_BUG

+ /*Important!!! We must do it to cancel flushing of changes from cache

+ * back to payload filling with new data from DMA

+ */

+ blackfin_dcache_invalidate_range((unsigned long)new_skb->head, (unsigned long)new_skb->end);

+#endif

current_rx_ptr->skb = new_skb;

current_rx_ptr->desc_a.start_addr = (unsigned long)new_skb->data - 2;

@@ -669,6 +688,28 @@

skb->ip_summed = CHECKSUM_COMPLETE;

#endif

+#ifdef CHECK_CACHE_BUG

+ if (skb->protocol == htons(ETH_P_IP))

+ {

+ struct iphdr* iph = (struct iphdr*) skb->data;

+ printk(KERN_DEBUG "bfin_mac rx_pkt: skb=x%p(x%p-x%p), id=%d, pkt_len=%d, ttl=%d\n",

+ skb,skb->head, skb->end,

+ ntohs(iph->id),

+ len,

+ iph->ttl

+ );

+ if (iph->ttl == 55)

+ printk("ooooops, new packet has been corrupted with old data!!!!\n");

+ iph->ttl = 55;

+ iph->check = 0;

+ iph->check = ip_fast_csum((unsigned char *)iph, iph->ihl);

+ }

+#endif

netif_rx(skb);

lp->stats.rx_packets++;

lp->stats.rx_bytes += len;

=============================

Follow-ups

--- Bryan Wu 2008-01-29 06:37:00

it was fixed in 08R1 and trunk.

in the driver:

1. old_skb->data filled with DMA TTL=64, then you modified it to TTL=55.

Noticed this TTL=55 is in data cache before write to memory.

2. old_skb send to upper level, finally it was free by kfree_skb. Here in data

cache should be different from real memory if not write to memory.

3. bf537mac_rx() call dev_alloc_skb() to get a new_skb. As you point out, this

new_skb->data might be the same as old_skb->data.

4. Soon, DMA will fill this new_skb->data with new data TTL=64 (in real

memory), while in data cache it is TTL=55.

5. Maybe soon, data cache was written to the memory and overwrite the data,

TTL=55.

6. So we got this bug.

With you patch, the new_skb->data cache was invalidated, so there is no

further data cache writing operation to memory any more. DMA data will not be

overwritten finally.

Thanks, I will apply this to our svn and 08R1 release, then send it to upstream

mainline.

One suggestion is maybe this cache invalidate operation should be added to

alloc_skb(), other architecture can share this protection.

-Bryan

--- Bryan Wu 2008-01-31 02:37:57

It fixed. so close it.

-Bryan

Files

Changes

Commits

Dependencies

Duplicates

Associations