[PATCH V2 v3.7] brcmsmac: handle packet drop on enqueuing correctly

[PATCH V2 v3.7] brcmsmac: handle packet drop on enqueuing correctly

[Arend van Spriel]

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>
---
 drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c |    4 ++--
 drivers/net/wireless/brcm80211/brcmsmac/main.c        |   14 +++++++++-----
 drivers/net/wireless/brcm80211/brcmsmac/main.h        |    2 +-
 drivers/net/wireless/brcm80211/brcmsmac/pub.h         |    2 +-
 4 files changed, 13 insertions(+), 9 deletions(-)

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);
-- 
1.7.10.4

Re: [PATCH V2 v3.7] brcmsmac: handle packet drop on enqueuing correctly

[Arend van Spriel]

On 11/26/2012 09:23 PM, 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>
> ---
>  drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c |    4 ++--
>  drivers/net/wireless/brcm80211/brcmsmac/main.c        |   14 +++++++++-----
>  drivers/net/wireless/brcm80211/brcmsmac/main.h        |    2 +-
>  drivers/net/wireless/brcm80211/brcmsmac/pub.h         |    2 +-
>  4 files changed, 13 insertions(+), 9 deletions(-)

Hi, John

What is keeping you from picking up this patch? Anything I should do?
This V2 removed the no-op changes in ampdu.c that Seth indicated so...
please let me know.

> 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;

This is where the slab corruption used to occur, because txinfo is part
of a possibly released sk_buff.

Regards,
Arend

>   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);
> 

Re: [PATCH V2 v3.7] brcmsmac: handle packet drop on enqueuing correctly

[Kalle Valo]

Hi Arend,

"Arend van Spriel" <arend@broadcom.com> writes:

> What is keeping you from picking up this patch? Anything I should do?
> This V2 removed the no-op changes in ampdu.c that Seth indicated so...

Most likely it's too late. Dave already told that that John's previous
pull request might not make it to 3.7.

-- 
Kalle Valo

Re: [PATCH V2 v3.7] brcmsmac: handle packet drop on enqueuing correctly

[John W. Linville]

On Mon, Dec 03, 2012 at 04:24:31PM +0200, Kalle Valo wrote:
> Hi Arend,
> 
> "Arend van Spriel" <arend@broadcom.com> writes:
> 
> > What is keeping you from picking up this patch? Anything I should do?
> > This V2 removed the no-op changes in ampdu.c that Seth indicated so...
> 
> Most likely it's too late. Dave already told that that John's previous
> pull request might not make it to 3.7.

Yes, that.  Also, given the merge difficulties this creates and the
fact that Seth's patch is already queued for 3.8, maybe you should
consider sending this for the 3.7 stable series when it opens?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH V2 v3.7] brcmsmac: handle packet drop on enqueuing correctly

[Arend van Spriel]

On 12/03/2012 07:39 PM, John W. Linville wrote:
> On Mon, Dec 03, 2012 at 04:24:31PM +0200, Kalle Valo wrote:
>> Hi Arend,
>>
>> "Arend van Spriel" <arend@broadcom.com> writes:
>>
>>> What is keeping you from picking up this patch? Anything I should do?
>>> This V2 removed the no-op changes in ampdu.c that Seth indicated so...
>>
>> Most likely it's too late. Dave already told that that John's previous
>> pull request might not make it to 3.7.
> 
> Yes, that.  Also, given the merge difficulties this creates and the
> fact that Seth's patch is already queued for 3.8, maybe you should
> consider sending this for the 3.7 stable series when it opens?
> 
> John
> 

That was my plan when it would not get in for 3.7.0. I wanted it to go
to stable anyway as the fix is also needed for 3.6.x stable. Just wanted
to get it upstream fixed first.

For 3.8 the fix has been reworked on top of Seth patches and is in
wireless-next (c4dea35e brcmsmac: handle packet drop during transmit
correctly). Should I refer to that commit when submitting to stable
(although it is a slightly different patch).

Gr. AvS

Re: [PATCH V2 v3.7] brcmsmac: handle packet drop on enqueuing correctly

[John W. Linville]

On Mon, Dec 03, 2012 at 07:54:54PM +0100, Arend van Spriel wrote:
> On 12/03/2012 07:39 PM, John W. Linville wrote:
> > On Mon, Dec 03, 2012 at 04:24:31PM +0200, Kalle Valo wrote:
> >> Hi Arend,
> >>
> >> "Arend van Spriel" <arend@broadcom.com> writes:
> >>
> >>> What is keeping you from picking up this patch? Anything I should do?
> >>> This V2 removed the no-op changes in ampdu.c that Seth indicated so...
> >>
> >> Most likely it's too late. Dave already told that that John's previous
> >> pull request might not make it to 3.7.
> > 
> > Yes, that.  Also, given the merge difficulties this creates and the
> > fact that Seth's patch is already queued for 3.8, maybe you should
> > consider sending this for the 3.7 stable series when it opens?
> > 
> > John
> > 
> 
> That was my plan when it would not get in for 3.7.0. I wanted it to go
> to stable anyway as the fix is also needed for 3.6.x stable. Just wanted
> to get it upstream fixed first.
> 
> For 3.8 the fix has been reworked on top of Seth patches and is in
> wireless-next (c4dea35e brcmsmac: handle packet drop during transmit
> correctly). Should I refer to that commit when submitting to stable
> (although it is a slightly different patch).

Yes, I think the stable team will appreciate that.

Thanks,

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH V2 v3.7] brcmsmac: handle packet drop on enqueuing correctly

[Arend van Spriel]

On 12/03/2012 07:59 PM, John W. Linville wrote:
> On Mon, Dec 03, 2012 at 07:54:54PM +0100, Arend van Spriel wrote:
>> On 12/03/2012 07:39 PM, John W. Linville wrote:
>>> On Mon, Dec 03, 2012 at 04:24:31PM +0200, Kalle Valo wrote:
>>>> Hi Arend,
>>>>
>>>> "Arend van Spriel" <arend@broadcom.com> writes:
>>>>
>>>>> What is keeping you from picking up this patch? Anything I should do?
>>>>> This V2 removed the no-op changes in ampdu.c that Seth indicated so...
>>>>
>>>> Most likely it's too late. Dave already told that that John's previous
>>>> pull request might not make it to 3.7.
>>>
>>> Yes, that.  Also, given the merge difficulties this creates and the
>>> fact that Seth's patch is already queued for 3.8, maybe you should
>>> consider sending this for the 3.7 stable series when it opens?
>>>
>>> John
>>>
>>
>> That was my plan when it would not get in for 3.7.0. I wanted it to go
>> to stable anyway as the fix is also needed for 3.6.x stable. Just wanted
>> to get it upstream fixed first.
>>
>> For 3.8 the fix has been reworked on top of Seth patches and is in
>> wireless-next (c4dea35e brcmsmac: handle packet drop during transmit
>> correctly). Should I refer to that commit when submitting to stable
>> (although it is a slightly different patch).
> 
> Yes, I think the stable team will appreciate that.
> 
> Thanks,
> 
> John
> 

Hi John,

Not keeping a close eye on LKML, but I noticed 3.7-rc8 is out. Would you
reconsider taking this patch for 3.7? Would be nice to be rid of slab
corruption in v3.7.0.

Gr. AvS