Re: memory clobber in rx path, maybe related to ath9k.

[Ben Greear <greearb@candelatech.com>]

On 10/12/2010 11:43 AM, Ben Greear wrote:
> On 10/12/2010 11:40 AM, Luis R. Rodriguez wrote:
>> On Tue, Oct 12, 2010 at 11:35 AM, Ben Greear<greearb@candelatech.com>
>> wrote:

>>> This code looks weird to me. One of the paprd branches
>>> deletes the skb, the other doesn't appear to. Neither
>>> null out bf->bf_mpdu, which would appear to leave a dangling
>>> pointer in at least the dev_kfree_skb_any() branch.
>>>
>>> ath_tx_complete frees it's skb in all cases, so another
>>> bf->bf_mpdu dangling pointer issue.
>>>
>>> Maybe at the least we should null out bf->bf_mpdu when
>>> skb is consumed?
>>
>> You're reading my mind, that was what I was going to test today. Still
>> doing e-mail sweep though.
>
> I'm trying to put together a patch now..but the paprd branch makes
> no sense at all.
>
> Shouldn't it also free the skb in the complete(&sc->paprd_complete) branch?
> I don't see anything that uses the skb, and nothing that frees it.
>
> if (bf->bf_state.bfs_paprd) {
> if (time_after(jiffies,
> bf->bf_state.bfs_paprd_timestamp +
> msecs_to_jiffies(ATH_PAPRD_TIMEOUT)))
> dev_kfree_skb_any(skb);
> else
> complete(&sc->paprd_complete);

I tried this patch, but the memory poision warnings continue.
Might still be a useful patch though...

[greearb@build-32 mac80211]$ git diff
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 8656491..f43a004 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -398,7 +398,7 @@ void ath_paprd_calibrate(struct work_struct *work)
                                   "Timeout waiting for paprd training on "
                                   "TX chain %d\n",
                                   chain);
-                       goto fail_paprd;
+                       break;
                 }

                 if (!ar9003_paprd_is_done(ah))
@@ -416,7 +416,6 @@ void ath_paprd_calibrate(struct work_struct *work)
                 ath_paprd_activate(sc);
         }

-fail_paprd:
         ath9k_ps_restore(sc);
  }

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 942be55..d39b4b5 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1919,17 +1919,17 @@ static void ath_tx_complete_buf(struct ath_softc *sc, struct ath_buf *bf,
         dma_unmap_single(sc->dev, bf->bf_dmacontext, skb->len, DMA_TO_DEVICE);

         if (bf->bf_state.bfs_paprd) {
-               if (time_after(jiffies,
-                              bf->bf_state.bfs_paprd_timestamp +
-                              msecs_to_jiffies(ATH_PAPRD_TIMEOUT)))
-                       dev_kfree_skb_any(skb);
-               else
-                       complete(&sc->paprd_complete);
+               /* ath_paprd_calibrate owns the skb. */
+               complete(&sc->paprd_complete);
         } else {
-               ath_tx_complete(sc, skb, bf->aphy, tx_flags);
+               /* stat_tx must be called first, it references skb. */
                 ath_debug_stat_tx(sc, txq, bf, ts);
+               ath_tx_complete(sc, skb, bf->aphy, tx_flags);
         }

+       /* At this point, skb is consumed one way or another */
+       bf->bf_mpdu = NULL;
+
         /*
          * Return the list of ath_buf of this mpdu to free queue
          */


Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com