| Message ID | 1353671082-7775-1-git-send-email-arend@broadcom.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Nov 23, 2012 at 12:44:42PM +0100, Arend van Spriel wrote: > From: Piotr Haber <phaber@broadcom.com> > > In the event that tx packet can not be queued by the driver > the packet is dropped. Propagate that information to the .tx() > callback to make sure the freed packet is not accessed after > that. > > This has happened causing slab corruptions as reported by > Stanislaw Gruszka. > > Bug #47721: https://bugzilla.kernel.org/show_bug.cgi?id=47721 > > Reported-by: Stanislaw Gruszka <stf_xl@wp.pl> > Reviewed-by: Arend van Spriel <arend@broadcom.com> > Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com> > Signed-off-by: Piotr Haber <phaber@broadcom.com> > Signed-off-by: Arend van Spriel <arend@broadcom.com> > --- > Fixing a kernel bug so based on the wireless repository. The > fix for wireless-next will be posted separately as the patches > differ. So this patch does not need to be merged to the > wireless-next tree. Let me know if I can be of help in resolving the conflicts. Fwiw the fix looks like it ought to be easy to make on top of wireless-next, but I do have a couple of comments. > diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c > index be5bcfb..a6605b1 100644 > --- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c > +++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c > @@ -901,7 +901,7 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, > struct ieee80211_hdr *h; > u16 seq, start_seq = 0, bindex, index, mcl; > u8 mcs = 0; > - bool ba_recd = false, ack_recd = false; > + bool ba_recd = false, ack_recd = false, last_packet = false; > u8 suc_mpdu = 0, tot_mpdu = 0; > uint supr_status; > bool update_rate = true, retry = true, tx_error = false; > @@ -1010,6 +1010,8 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, > > index = TX_SEQ_TO_INDEX(seq); > ack_recd = false; > + last_packet = (((mcl & TXC_AMPDU_MASK) >> TXC_AMPDU_SHIFT) == > + TXC_AMPDU_LAST); > if (ba_recd) { > bindex = MODSUB_POW2(seq, start_seq, SEQNUM_MAX); > BCMMSG(wiphy, > @@ -1074,8 +1076,7 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, > tot_mpdu++; > > /* break out if last packet of ampdu */ > - if (((mcl & TXC_AMPDU_MASK) >> TXC_AMPDU_SHIFT) == > - TXC_AMPDU_LAST) > + if (last_packet) > break; > > p = dma_getnexttxp(wlc->hw->di[queue], DMA_RANGE_TRANSMITTED); These changes are effectively a no-op and don't really seem to have anything to do with fixing the bug. > @@ -7288,10 +7290,12 @@ void brcms_c_sendpkt_mac80211(struct brcms_c_info *wlc, struct sk_buff *sdu, > prio = ieee80211_is_data(d11_header->frame_control) ? sdu->priority : > MAXPRIO; > fifo = prio2fifo[prio]; > - if (brcms_c_d11hdrs_mac80211(wlc, hw, sdu, scb, 0, 1, fifo, 0)) > - return; > - brcms_c_txq_enq(wlc, scb, sdu, BRCMS_PRIO_TO_PREC(prio)); > + brcms_c_d11hdrs_mac80211(wlc, hw, sdu, scb, 0, 1, fifo, 0); Maybe brcms_c_d11hdrs_mac80211() should return void? I've never understood what its return value was supposed to represent. Cheers, Seth -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 26, 2012 at 08:55:11AM -0600, Seth Forshee wrote: > On Fri, Nov 23, 2012 at 12:44:42PM +0100, Arend van Spriel wrote: > > From: Piotr Haber <phaber@broadcom.com> > > > > In the event that tx packet can not be queued by the driver > > the packet is dropped. Propagate that information to the .tx() > > callback to make sure the freed packet is not accessed after > > that. > > > > This has happened causing slab corruptions as reported by > > Stanislaw Gruszka. > > > > Bug #47721: https://bugzilla.kernel.org/show_bug.cgi?id=47721 > > > > Reported-by: Stanislaw Gruszka <stf_xl@wp.pl> > > Reviewed-by: Arend van Spriel <arend@broadcom.com> > > Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com> > > Signed-off-by: Piotr Haber <phaber@broadcom.com> > > Signed-off-by: Arend van Spriel <arend@broadcom.com> > > --- > > Fixing a kernel bug so based on the wireless repository. The > > fix for wireless-next will be posted separately as the patches > > differ. So this patch does not need to be merged to the > > wireless-next tree. > > Let me know if I can be of help in resolving the conflicts. Fwiw the fix > looks like it ought to be easy to make on top of wireless-next, but I do > have a couple of comments. > > > diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c > > index be5bcfb..a6605b1 100644 > > --- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c > > +++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c > > @@ -901,7 +901,7 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, > > struct ieee80211_hdr *h; > > u16 seq, start_seq = 0, bindex, index, mcl; > > u8 mcs = 0; > > - bool ba_recd = false, ack_recd = false; > > + bool ba_recd = false, ack_recd = false, last_packet = false; > > u8 suc_mpdu = 0, tot_mpdu = 0; > > uint supr_status; > > bool update_rate = true, retry = true, tx_error = false; > > @@ -1010,6 +1010,8 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, > > > > index = TX_SEQ_TO_INDEX(seq); > > ack_recd = false; > > + last_packet = (((mcl & TXC_AMPDU_MASK) >> TXC_AMPDU_SHIFT) == > > + TXC_AMPDU_LAST); > > if (ba_recd) { > > bindex = MODSUB_POW2(seq, start_seq, SEQNUM_MAX); > > BCMMSG(wiphy, > > @@ -1074,8 +1076,7 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, > > tot_mpdu++; > > > > /* break out if last packet of ampdu */ > > - if (((mcl & TXC_AMPDU_MASK) >> TXC_AMPDU_SHIFT) == > > - TXC_AMPDU_LAST) > > + if (last_packet) > > break; > > > > p = dma_getnexttxp(wlc->hw->di[queue], DMA_RANGE_TRANSMITTED); > > These changes are effectively a no-op and don't really seem to have > anything to do with fixing the bug. I concur -- could you remove these bits for the 3.7 fix? > > > @@ -7288,10 +7290,12 @@ void brcms_c_sendpkt_mac80211(struct brcms_c_info *wlc, struct sk_buff *sdu, > > prio = ieee80211_is_data(d11_header->frame_control) ? sdu->priority : > > MAXPRIO; > > fifo = prio2fifo[prio]; > > - if (brcms_c_d11hdrs_mac80211(wlc, hw, sdu, scb, 0, 1, fifo, 0)) > > - return; > > - brcms_c_txq_enq(wlc, scb, sdu, BRCMS_PRIO_TO_PREC(prio)); > > + brcms_c_d11hdrs_mac80211(wlc, hw, sdu, scb, 0, 1, fifo, 0); > > Maybe brcms_c_d11hdrs_mac80211() should return void? I've never > understood what its return value was supposed to represent. > > Cheers, > Seth > >
On 11/26/2012 08:20 PM, John W. Linville wrote: > On Mon, Nov 26, 2012 at 08:55:11AM -0600, Seth Forshee wrote: >> On Fri, Nov 23, 2012 at 12:44:42PM +0100, Arend van Spriel wrote: >>> From: Piotr Haber <phaber@broadcom.com> >>> >>> In the event that tx packet can not be queued by the driver >>> the packet is dropped. Propagate that information to the .tx() >>> callback to make sure the freed packet is not accessed after >>> that. >>> >>> This has happened causing slab corruptions as reported by >>> Stanislaw Gruszka. >>> >>> Bug #47721: https://bugzilla.kernel.org/show_bug.cgi?id=47721 >>> >>> Reported-by: Stanislaw Gruszka <stf_xl@wp.pl> >>> Reviewed-by: Arend van Spriel <arend@broadcom.com> >>> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com> >>> Signed-off-by: Piotr Haber <phaber@broadcom.com> >>> Signed-off-by: Arend van Spriel <arend@broadcom.com> >>> --- >>> Fixing a kernel bug so based on the wireless repository. The >>> fix for wireless-next will be posted separately as the patches >>> differ. So this patch does not need to be merged to the >>> wireless-next tree. >> >> Let me know if I can be of help in resolving the conflicts. Fwiw the fix >> looks like it ought to be easy to make on top of wireless-next, but I do >> have a couple of comments. >> >>> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c >>> index be5bcfb..a6605b1 100644 >>> --- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c >>> +++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c >>> @@ -901,7 +901,7 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, >>> struct ieee80211_hdr *h; >>> u16 seq, start_seq = 0, bindex, index, mcl; >>> u8 mcs = 0; >>> - bool ba_recd = false, ack_recd = false; >>> + bool ba_recd = false, ack_recd = false, last_packet = false; >>> u8 suc_mpdu = 0, tot_mpdu = 0; >>> uint supr_status; >>> bool update_rate = true, retry = true, tx_error = false; >>> @@ -1010,6 +1010,8 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, >>> >>> index = TX_SEQ_TO_INDEX(seq); >>> ack_recd = false; >>> + last_packet = (((mcl & TXC_AMPDU_MASK) >> TXC_AMPDU_SHIFT) == >>> + TXC_AMPDU_LAST); >>> if (ba_recd) { >>> bindex = MODSUB_POW2(seq, start_seq, SEQNUM_MAX); >>> BCMMSG(wiphy, >>> @@ -1074,8 +1076,7 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, >>> tot_mpdu++; >>> >>> /* break out if last packet of ampdu */ >>> - if (((mcl & TXC_AMPDU_MASK) >> TXC_AMPDU_SHIFT) == >>> - TXC_AMPDU_LAST) >>> + if (last_packet) >>> break; >>> >>> p = dma_getnexttxp(wlc->hw->di[queue], DMA_RANGE_TRANSMITTED); >> >> These changes are effectively a no-op and don't really seem to have >> anything to do with fixing the bug. > > I concur -- could you remove these bits for the 3.7 fix? > True. Will resend it. Gr. AvS -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/26/2012 03:55 PM, Seth Forshee wrote: > On Fri, Nov 23, 2012 at 12:44:42PM +0100, Arend van Spriel wrote: >> From: Piotr Haber <phaber@broadcom.com> >> >> In the event that tx packet can not be queued by the driver >> the packet is dropped. Propagate that information to the .tx() >> callback to make sure the freed packet is not accessed after >> that. >> >> This has happened causing slab corruptions as reported by >> Stanislaw Gruszka. >> >> Bug #47721: https://bugzilla.kernel.org/show_bug.cgi?id=47721 >> >> Reported-by: Stanislaw Gruszka <stf_xl@wp.pl> >> Reviewed-by: Arend van Spriel <arend@broadcom.com> >> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com> >> Signed-off-by: Piotr Haber <phaber@broadcom.com> >> Signed-off-by: Arend van Spriel <arend@broadcom.com> >> --- >> Fixing a kernel bug so based on the wireless repository. The >> fix for wireless-next will be posted separately as the patches >> differ. So this patch does not need to be merged to the >> wireless-next tree. > > Let me know if I can be of help in resolving the conflicts. Fwiw the fix > looks like it ought to be easy to make on top of wireless-next, but I do > have a couple of comments. I have fix for wireless-next available already. Will post it later today. >> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c >> index be5bcfb..a6605b1 100644 >> --- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c >> +++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c >> @@ -901,7 +901,7 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, >> struct ieee80211_hdr *h; >> u16 seq, start_seq = 0, bindex, index, mcl; >> u8 mcs = 0; >> - bool ba_recd = false, ack_recd = false; >> + bool ba_recd = false, ack_recd = false, last_packet = false; >> u8 suc_mpdu = 0, tot_mpdu = 0; >> uint supr_status; >> bool update_rate = true, retry = true, tx_error = false; >> @@ -1010,6 +1010,8 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, >> >> index = TX_SEQ_TO_INDEX(seq); >> ack_recd = false; >> + last_packet = (((mcl & TXC_AMPDU_MASK) >> TXC_AMPDU_SHIFT) == >> + TXC_AMPDU_LAST); >> if (ba_recd) { >> bindex = MODSUB_POW2(seq, start_seq, SEQNUM_MAX); >> BCMMSG(wiphy, >> @@ -1074,8 +1076,7 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, >> tot_mpdu++; >> >> /* break out if last packet of ampdu */ >> - if (((mcl & TXC_AMPDU_MASK) >> TXC_AMPDU_SHIFT) == >> - TXC_AMPDU_LAST) >> + if (last_packet) >> break; >> >> p = dma_getnexttxp(wlc->hw->di[queue], DMA_RANGE_TRANSMITTED); > > These changes are effectively a no-op and don't really seem to have > anything to do with fixing the bug. Agree. I noticed that as well and was in doubt whether I should keep it or not. The commit message does not cover this so I resubmit the patch (already did). >> @@ -7288,10 +7290,12 @@ void brcms_c_sendpkt_mac80211(struct brcms_c_info *wlc, struct sk_buff *sdu, >> prio = ieee80211_is_data(d11_header->frame_control) ? sdu->priority : >> MAXPRIO; >> fifo = prio2fifo[prio]; >> - if (brcms_c_d11hdrs_mac80211(wlc, hw, sdu, scb, 0, 1, fifo, 0)) >> - return; >> - brcms_c_txq_enq(wlc, scb, sdu, BRCMS_PRIO_TO_PREC(prio)); >> + brcms_c_d11hdrs_mac80211(wlc, hw, sdu, scb, 0, 1, fifo, 0); > > Maybe brcms_c_d11hdrs_mac80211() should return void? I've never > understood what its return value was supposed to represent. All code patch in the function return 0 so it could be made void. Gr. AvS -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c index be5bcfb..a6605b1 100644 --- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c +++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c @@ -901,7 +901,7 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, struct ieee80211_hdr *h; u16 seq, start_seq = 0, bindex, index, mcl; u8 mcs = 0; - bool ba_recd = false, ack_recd = false; + bool ba_recd = false, ack_recd = false, last_packet = false; u8 suc_mpdu = 0, tot_mpdu = 0; uint supr_status; bool update_rate = true, retry = true, tx_error = false; @@ -1010,6 +1010,8 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, index = TX_SEQ_TO_INDEX(seq); ack_recd = false; + last_packet = (((mcl & TXC_AMPDU_MASK) >> TXC_AMPDU_SHIFT) == + TXC_AMPDU_LAST); if (ba_recd) { bindex = MODSUB_POW2(seq, start_seq, SEQNUM_MAX); BCMMSG(wiphy, @@ -1074,8 +1076,7 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb, tot_mpdu++; /* break out if last packet of ampdu */ - if (((mcl & TXC_AMPDU_MASK) >> TXC_AMPDU_SHIFT) == - TXC_AMPDU_LAST) + if (last_packet) break; p = dma_getnexttxp(wlc->hw->di[queue], DMA_RANGE_TRANSMITTED); diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c index a744ea5..5590499 100644 --- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c +++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c @@ -280,8 +280,8 @@ static void brcms_ops_tx(struct ieee80211_hw *hw, kfree_skb(skb); goto done; } - brcms_c_sendpkt_mac80211(wl->wlc, skb, hw); - tx_info->rate_driver_data[0] = control->sta; + if (brcms_c_sendpkt_mac80211(wl->wlc, skb, hw)) + tx_info->rate_driver_data[0] = control->sta; done: spin_unlock_bh(&wl->lock); } diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c index 75086b3..9fb0a4c9 100644 --- a/drivers/net/wireless/brcm80211/brcmsmac/main.c +++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c @@ -6095,7 +6095,7 @@ static bool brcms_c_prec_enq(struct brcms_c_info *wlc, struct pktq *q, return brcms_c_prec_enq_head(wlc, q, pkt, prec, false); } -void brcms_c_txq_enq(struct brcms_c_info *wlc, struct scb *scb, +bool brcms_c_txq_enq(struct brcms_c_info *wlc, struct scb *scb, struct sk_buff *sdu, uint prec) { struct brcms_txq_info *qi = wlc->pkt_queue; /* Check me */ @@ -6110,7 +6110,9 @@ void brcms_c_txq_enq(struct brcms_c_info *wlc, struct scb *scb, * packet flooding from mac80211 stack */ brcmu_pkt_buf_free_skb(sdu); + return false; } + return true; } /* @@ -7273,7 +7275,7 @@ brcms_c_d11hdrs_mac80211(struct brcms_c_info *wlc, struct ieee80211_hw *hw, return 0; } -void brcms_c_sendpkt_mac80211(struct brcms_c_info *wlc, struct sk_buff *sdu, +bool brcms_c_sendpkt_mac80211(struct brcms_c_info *wlc, struct sk_buff *sdu, struct ieee80211_hw *hw) { u8 prio; @@ -7288,10 +7290,12 @@ void brcms_c_sendpkt_mac80211(struct brcms_c_info *wlc, struct sk_buff *sdu, prio = ieee80211_is_data(d11_header->frame_control) ? sdu->priority : MAXPRIO; fifo = prio2fifo[prio]; - if (brcms_c_d11hdrs_mac80211(wlc, hw, sdu, scb, 0, 1, fifo, 0)) - return; - brcms_c_txq_enq(wlc, scb, sdu, BRCMS_PRIO_TO_PREC(prio)); + brcms_c_d11hdrs_mac80211(wlc, hw, sdu, scb, 0, 1, fifo, 0); + if (!brcms_c_txq_enq(wlc, scb, sdu, BRCMS_PRIO_TO_PREC(prio))) + return false; brcms_c_send_q(wlc); + + return true; } void brcms_c_send_q(struct brcms_c_info *wlc) diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.h b/drivers/net/wireless/brcm80211/brcmsmac/main.h index 8debc74..b44725c 100644 --- a/drivers/net/wireless/brcm80211/brcmsmac/main.h +++ b/drivers/net/wireless/brcm80211/brcmsmac/main.h @@ -642,7 +642,7 @@ extern void brcms_c_txfifo(struct brcms_c_info *wlc, uint fifo, bool commit, s8 txpktpend); extern void brcms_c_txfifo_complete(struct brcms_c_info *wlc, uint fifo, s8 txpktpend); -extern void brcms_c_txq_enq(struct brcms_c_info *wlc, struct scb *scb, +extern bool brcms_c_txq_enq(struct brcms_c_info *wlc, struct scb *scb, struct sk_buff *sdu, uint prec); extern void brcms_c_print_txstatus(struct tx_status *txs); extern int brcms_b_xmtfifo_sz_get(struct brcms_hardware *wlc_hw, uint fifo, diff --git a/drivers/net/wireless/brcm80211/brcmsmac/pub.h b/drivers/net/wireless/brcm80211/brcmsmac/pub.h index 5855f4f..bfa2630 100644 --- a/drivers/net/wireless/brcm80211/brcmsmac/pub.h +++ b/drivers/net/wireless/brcm80211/brcmsmac/pub.h @@ -321,7 +321,7 @@ extern void brcms_c_intrsrestore(struct brcms_c_info *wlc, u32 macintmask); extern bool brcms_c_intrsupd(struct brcms_c_info *wlc); extern bool brcms_c_isr(struct brcms_c_info *wlc, bool *wantdpc); extern bool brcms_c_dpc(struct brcms_c_info *wlc, bool bounded); -extern void brcms_c_sendpkt_mac80211(struct brcms_c_info *wlc, +extern bool brcms_c_sendpkt_mac80211(struct brcms_c_info *wlc, struct sk_buff *sdu, struct ieee80211_hw *hw); extern bool brcms_c_aggregatable(struct brcms_c_info *wlc, u8 tid);