Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758839AbYGPQMT (ORCPT ); Wed, 16 Jul 2008 12:12:19 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754652AbYGPQMF (ORCPT ); Wed, 16 Jul 2008 12:12:05 -0400 Received: from qw-out-2122.google.com ([74.125.92.24]:47082 "EHLO qw-out-2122.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752686AbYGPQMD (ORCPT ); Wed, 16 Jul 2008 12:12:03 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:cc:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references; b=jf/3AV3q4NFsJjUkIDQm3AYN/nFhsvumVswEl29sqm6cfZ1E2zK/2s19jNm1LCqai4 G57H4+7cNVtNcdcuECP3vYL7m4l2GBDZU5MKH8Yp7uen9DH5e/6lpX7JMxujGNV5+18O zVh4JA3sl4cLIkS8mHPA60YXBTofd1i59mBvE= Message-ID: <40f31dec0807160912h6ce88550w770b8d221b6d5579@mail.gmail.com> Date: Wed, 16 Jul 2008 19:12:01 +0300 From: "Nick Kossifidis" To: "Jiri Slaby" Subject: Re: [PATCH 1/5] Ath5k: fix memory corruption Cc: linville@tuxdriver.com, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, "Luis R. Rodriguez" In-Reply-To: <1216136661-10930-1-git-send-email-jirislaby@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Disposition: inline References: <1216136661-10930-1-git-send-email-jirislaby@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by alpha.home.local id m6GGCNfq024963 Content-Length: 6943 Lines: 5 2008/7/15 Jiri Slaby :> When signal is noisy, hardware can use all RX buffers and since the last> entry in the list is self-linked, it overwrites the entry until we link> new buffers.>> Ensure that we don't free this last one until we are 100% sure that it> is not used by the hardware anymore to not cause memory curruption as> can be seen below.>> This is done by checking next buffer in the list. Even after that we> know that the hardware refetched the new link and proceeded further> (the next buffer is ready) we can finally free the overwritten buffer.>> We discard it since the status in its descriptor is overwritten (OR-ed> by new status) too.>> =============================================================================> BUG kmalloc-4096: Poison overwritten> ----------------------------------------------------------------------------->> INFO: 0xffff810067419060-0xffff810067419667. First byte 0x8 instead of 0x6b> INFO: Allocated in dev_alloc_skb+0x18/0x30 age=1118 cpu=1 pid=0> INFO: Freed in skb_release_data+0x85/0xd0 age=1105 cpu=1 pid=3718> INFO: Slab 0xffffe200019d0600 objects=7 used=0 fp=0xffff810067419048 flags=0x40000000000020c3> INFO: Object 0xffff810067419048 @offset=4168 fp=0xffff81006741c120>> Bytes b4 0xffff810067419038: 4f 0b 02 00 01 00 00 00 5a 5a 5a 5a 5a 5a 5a 5a O.......ZZZZZZZZ> Object 0xffff810067419048: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk> Object 0xffff810067419058: 6b 6b 6b 6b 6b 6b 6b 6b 08 42 30 00 00 0b 6b 80 kkkkkkkk.B0...k.> Object 0xffff810067419068: f0 5d 00 4f 62 08 a3 64 00 0c 42 16 52 e4 f0 5a 360].Ob.243d..B.R344360Z> Object 0xffff810067419078: 68 81 00 00 7b a5 b4 be 7d 3b 8f 53 cd d5 de 12 h...{245264276};.S315325336.> Object 0xffff810067419088: 96 10 0b 89 48 54 23 41 0f 4e 2d b9 37 c3 cb 29 ....HT#A.N-2717303313)> Object 0xffff810067419098: d1 e0 de 14 8a 57 2a cc 3b 44 0d 78 7a 19 12 15 321340336..W*314;D.xz...> Object 0xffff8100674190a8: a9 ec d4 35 a8 10 ec 8c 40 a7 06 0a 51 a7 48 bb 2513543245250.354.@247..Q247H273> Object 0xffff8100674190b8: 3e cf a1 c7 38 60 63 3f 51 15 c7 20 eb ba 65 30 >ϡ3078`c?Q.307.353272e0> Redzone 0xffff81006741a048: bb bb bb bb bb bb bb bb 273273273273273273273273> Padding 0xffff81006741a088: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ> Pid: 3297, comm: ath5k_pci Not tainted 2.6.26-rc8-mm1_64 #427>> Call Trace:> [] print_trailer+0xf6/0x150> [] check_bytes_and_report+0x125/0x180> [] check_object+0xac/0x260> [] __slab_alloc+0x368/0x6d0> [] ? wireless_send_event+0x142/0x310> [] ? __alloc_skb+0x44/0x150> [] ? wireless_send_event+0x142/0x310> [] __kmalloc_track_caller+0xc3/0xf0> [] __alloc_skb+0x6e/0x150> [... stack snipped]>> FIX kmalloc-4096: Restoring 0xffff810067419060-0xffff810067419667=0x6b>> FIX kmalloc-4096: Marking all objects used>> Signed-off-by: Jiri Slaby > Cc: Nick Kossifidis > Cc: Luis R. Rodriguez > ---> drivers/net/wireless/ath5k/base.c | 32 +++++++++++++++++++++++++-------> drivers/net/wireless/ath5k/base.h | 2 +-> 2 files changed, 26 insertions(+), 8 deletions(-)>> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c> index 12a9443..e9ec284 100644> --- a/drivers/net/wireless/ath5k/base.c> +++ b/drivers/net/wireless/ath5k/base.c> @@ -1683,20 +1683,21 @@ ath5k_tasklet_rx(unsigned long data)> struct ath5k_rx_status rs = {};> struct sk_buff *skb;> struct ath5k_softc *sc = (void *)data;> - struct ath5k_buf *bf;> + struct ath5k_buf *bf, *bf_last;> struct ath5k_desc *ds;> int ret;> int hdrlen;> int pad;>> spin_lock(&sc->rxbuflock);> + if (list_empty(&sc->rxbuf)) {> + ATH5K_WARN(sc, "empty rx buf pool\n");> + goto unlock;> + }> + bf_last = list_entry(sc->rxbuf.prev, struct ath5k_buf, list);> do {> rxs.flag = 0;>> - if (unlikely(list_empty(&sc->rxbuf))) {> - ATH5K_WARN(sc, "empty rx buf pool\n");> - break;> - }> bf = list_first_entry(&sc->rxbuf, struct ath5k_buf, list);> BUG_ON(bf->skb == NULL);> skb = bf->skb;> @@ -1706,8 +1707,24 @@ ath5k_tasklet_rx(unsigned long data)> pci_dma_sync_single_for_cpu(sc->pdev, sc->desc_daddr,> sc->desc_len, PCI_DMA_FROMDEVICE);>> - if (unlikely(ds->ds_link == bf->daddr)) /* this is the end */> - break;> + /*> + * last buffer must not be freed to ensure proper hardware> + * function. When the hardware finishes also a packet next to> + * it, we are sure, it doesn't use it anymore and we can go on.> + */> + if (bf_last == bf)> + bf->flags |= 1;> + if (bf->flags) {> + struct ath5k_buf *bf_next = list_entry(bf->list.next,> + struct ath5k_buf, list);> + ret = sc->ah->ah_proc_rx_desc(sc->ah, bf_next->desc,> + &rs);> + if (ret)> + break;> + bf->flags &= ~1;> + /* skip the overwritten one (even status is martian) */> + goto next;> + }>> ret = sc->ah->ah_proc_rx_desc(sc->ah, ds, &rs);> if (unlikely(ret == -EINPROGRESS))> @@ -1817,6 +1834,7 @@ accept:> next:> list_move_tail(&bf->list, &sc->rxbuf);> } while (ath5k_rxbuf_setup(sc, bf) == 0);> +unlock:> spin_unlock(&sc->rxbuflock);> }>> diff --git a/drivers/net/wireless/ath5k/base.h b/drivers/net/wireless/ath5k/base.h> index 47f414b..d7e03e6 100644> --- a/drivers/net/wireless/ath5k/base.h> +++ b/drivers/net/wireless/ath5k/base.h> @@ -56,7 +56,7 @@>> struct ath5k_buf {> struct list_head list;> - unsigned int flags; /* tx descriptor flags */> + unsigned int flags; /* rx descriptor flags */> struct ath5k_desc *desc; /* virtual addr of desc */> dma_addr_t daddr; /* physical addr of desc */> struct sk_buff *skb; /* skbuff for buf */> --> 1.5.6.2>> Nice catch ;-) Acked-by: Nick Kossifidis -- GPG ID: 0xD21DB2DBAs you read this post global entropy rises. Have Fun ;-)Nick????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?