[PATCH] bcm43xx: fix DMA TX skb freeing in case of fragmented packets.

It seems to me that the today's wireless-2.6 git contains bcm43xx which
does not free txb's correctly, if I understand it right.

Consider a situation where a txb with two skb's is sent down.
The dma_tx_fragment will save the pointer to meta->txb of the first
fragment. If fragments are freed in order, ieee80211_txb_free frees both
skb's when the first fragment is processed. This may result in reuse
of the second skb's memory.

This danger is rather remote, but it seems to me that the patch
below not only fixes the problem, but also makes the code simpler,
which is good, right?

Signed-off-by: Michael Buesch <mbuesch@freenet.de>
Signed-off-by: John W. Linville <linville@tuxdriver.com>

diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_dma.c b/drivers/net/wireless/bcm43xx/bcm43xx_dma.c
index e20fbaf29e0b5190612159f1d7ce5257383ad876..0cd292847954439113fb48e8d36b57f10d5e055f 100644 (file)
--- a/drivers/net/wireless/bcm43xx/bcm43xx_dma.c
+++ b/drivers/net/wireless/bcm43xx/bcm43xx_dma.c
@@ -170,19 +170,6 @@ void sync_descbuffer_for_device(struct bcm43xx_dmaring *ring,
                                   addr, len, DMA_FROM_DEVICE);
 }
 
-static inline
-void mark_skb_mustfree(struct sk_buff *skb,
-                      char mustfree)
-{
-       skb->cb[0] = mustfree;
-}
-
-static inline
-int skb_mustfree(struct sk_buff *skb)
-{
-       return (skb->cb[0] != 0);
-}
-
 /* Unmap and free a descriptor buffer. */
 static inline
 void free_descriptor_buffer(struct bcm43xx_dmaring *ring,
@@ -191,17 +178,11 @@ void free_descriptor_buffer(struct bcm43xx_dmaring *ring,
                            int irq_context)
 {
        assert(meta->skb);
-       if (skb_mustfree(meta->skb)) {
-               if (irq_context)
-                       dev_kfree_skb_irq(meta->skb);
-               else
-                       dev_kfree_skb(meta->skb);
-       }
+       if (irq_context)
+               dev_kfree_skb_irq(meta->skb);
+       else
+               dev_kfree_skb(meta->skb);
        meta->skb = NULL;
-       if (meta->txb) {
-               ieee80211_txb_free(meta->txb);
-               meta->txb = NULL;
-       }
 }
 
 static int alloc_ringmemory(struct bcm43xx_dmaring *ring)
@@ -334,7 +315,6 @@ static int setup_rx_descbuffer(struct bcm43xx_dmaring *ring,
        meta->skb = skb;
        meta->dmaaddr = dmaaddr;
        skb->dev = ring->bcm->net_dev;
-       mark_skb_mustfree(skb, 1);
        desc_addr = (u32)(dmaaddr + ring->memoffset);
        desc_ctl = (BCM43xx_DMADTOR_BYTECNT_MASK &
                    (u32)(ring->rx_buffersize - ring->frameoffset));
@@ -457,7 +437,6 @@ static void free_all_descbuffers(struct bcm43xx_dmaring *ring)
 
                if (!meta->skb) {
                        assert(ring->tx);
-                       assert(!meta->txb);
                        continue;
                }
                if (ring->tx) {
@@ -726,7 +705,6 @@ static void dmacontroller_poke_tx(struct bcm43xx_dmaring *ring,
 
 static int dma_tx_fragment(struct bcm43xx_dmaring *ring,
                           struct sk_buff *skb,
-                          struct ieee80211_txb *txb,
                           u8 cur_frag)
 {
        int slot;
@@ -741,11 +719,6 @@ static int dma_tx_fragment(struct bcm43xx_dmaring *ring,
        desc = ring->vbase + slot;
        meta = ring->meta + slot;
 
-       if (cur_frag == 0) {
-               /* Save the txb pointer for freeing in xmitstatus IRQ */
-               meta->txb = txb;
-       }
-
        /* Add a device specific TX header. */
        assert(skb_headroom(skb) >= sizeof(struct bcm43xx_txhdr));
        /* Reserve enough headroom for the device tx header. */
@@ -810,13 +783,12 @@ int bcm43xx_dma_tx(struct bcm43xx_private *bcm,
 
        for (i = 0; i < txb->nr_frags; i++) {
                skb = txb->fragments[i];
-               /* We do not free the skb, as it is freed as
-                * part of the txb freeing.
-                */
-               mark_skb_mustfree(skb, 0);
-               dma_tx_fragment(ring, skb, txb, i);
+               /* Take skb from ieee80211_txb_free */
+               txb->fragments[i] = NULL;
+               dma_tx_fragment(ring, skb, i);
                //TODO: handle failure of dma_tx_fragment
        }
+       ieee80211_txb_free(txb);
 
        return 0;
 }

diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_dma.h b/drivers/net/wireless/bcm43xx/bcm43xx_dma.h
index 88ad34dff2f23b07b30e6accd18ecdc8be0ba208..cab8e2ba4c7efc0ed752f4ae8eb83ee3c7abb1ac 100644 (file)
--- a/drivers/net/wireless/bcm43xx/bcm43xx_dma.h
+++ b/drivers/net/wireless/bcm43xx/bcm43xx_dma.h
@@ -119,10 +119,6 @@ struct bcm43xx_dmadesc_meta {
        struct sk_buff *skb;
        /* DMA base bus-address of the descriptor buffer. */
        dma_addr_t dmaaddr;
-       /* Pointer to our txb (can be NULL).
-        * This should be freed in completion IRQ.
-        */
-       struct ieee80211_txb *txb;
 };
 
 struct bcm43xx_dmaring {