[PATCH 0/3] iwlwifi: fixes intended for 4.13 2017-08-05

[PATCH 0/3] iwlwifi: fixes intended for 4.13 2017-08-05

[Luca Coelho]

From: Luca Coelho <luciano.coelho@intel.com>

Hi,

This is the third set of fixes for 4.13.

The issues solved here are:

* Fix a memory leak in the SAR code;
* Fix a stuck queue case in AP mode;
* Convert a WARN to a simple debug in a legitimate race case (from
  which we can recover);

As usual, I'm pushing this to a pending branch, for kbuild bot, and
will send a pull-request later.

Please review.

Cheers,
Luca.


Avraham Stern (1):
  iwlwifi: mvm: start mac queues when deferred tx frames are purged

Christophe Jaillet (1):
  iwlwifi: mvm: Fix a memory leak in an error handling path in
    'iwl_mvm_sar_get_wgds_table()'

Emmanuel Grumbach (1):
  iwlwifi: mvm: don't WARN when a legit race happens in A-MPDU

 drivers/net/wireless/intel/iwlwifi/mvm/fw.c       |  6 ++++--
 drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c | 12 +++++++++++-
 drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c     | 10 ++++++----
 3 files changed, 21 insertions(+), 7 deletions(-)

-- 
2.13.2

[PATCH 1/3] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'

[Luca Coelho]

From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>

We should free 'wgds.pointer' here as done a few lines above in another
error handling path.
It was allocated within 'acpi_evaluate_object()'.

Fixes: c52030a01ccc ("iwlwifi: mvm: add GEO_TX_POWER_LIMIT cmd for geographic tx power table")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
index 79e7a7a285dc..82863e9273eb 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
@@ -1275,8 +1275,10 @@ static int iwl_mvm_sar_get_wgds_table(struct iwl_mvm *mvm)
 
 			entry = &wifi_pkg->package.elements[idx++];
 			if ((entry->type != ACPI_TYPE_INTEGER) ||
-			    (entry->integer.value > U8_MAX))
-				return -EINVAL;
+			    (entry->integer.value > U8_MAX)) {
+				ret = -EINVAL;
+				goto out_free;
+			}
 
 			mvm->geo_profiles[i].values[j] = entry->integer.value;
 		}
-- 
2.13.2

Re: [PATCH 1/3] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'

[Kalle Valo]

Luca Coelho <luca@coelho.fi> writes:

> From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
>
> We should free 'wgds.pointer' here as done a few lines above in another
> error handling path.
> It was allocated within 'acpi_evaluate_object()'.
>
> Fixes: c52030a01ccc ("iwlwifi: mvm: add GEO_TX_POWER_LIMIT cmd for geographic tx power table")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> index 79e7a7a285dc..82863e9273eb 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> @@ -1275,8 +1275,10 @@ static int iwl_mvm_sar_get_wgds_table(struct iwl_mvm *mvm)
>  
>  			entry = &wifi_pkg->package.elements[idx++];
>  			if ((entry->type != ACPI_TYPE_INTEGER) ||
> -			    (entry->integer.value > U8_MAX))
> -				return -EINVAL;
> +			    (entry->integer.value > U8_MAX)) {
> +				ret = -EINVAL;
> +				goto out_free;
> +			}

How likely is this leak to happen in real world? To me it looks like
more like a theoretical issue and could have easily waited for 4.14. But
it's fine this time, just something to keep in mind in the future.

-- 
Kalle Valo

Re: [PATCH 1/3] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'

[Luca Coelho]

On Mon, 2017-08-07 at 15:57 +0300, Kalle Valo wrote:
> Luca Coelho <luca@coelho.fi> writes:
> 
> > From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
> > 
> > We should free 'wgds.pointer' here as done a few lines above in another
> > error handling path.
> > It was allocated within 'acpi_evaluate_object()'.
> > 
> > Fixes: c52030a01ccc ("iwlwifi: mvm: add GEO_TX_POWER_LIMIT cmd for geographic tx power table")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > ---
> >  drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > index 79e7a7a285dc..82863e9273eb 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > @@ -1275,8 +1275,10 @@ static int iwl_mvm_sar_get_wgds_table(struct iwl_mvm *mvm)
> >  
> >  			entry = &wifi_pkg->package.elements[idx++];
> >  			if ((entry->type != ACPI_TYPE_INTEGER) ||
> > -			    (entry->integer.value > U8_MAX))
> > -				return -EINVAL;
> > +			    (entry->integer.value > U8_MAX)) {
> > +				ret = -EINVAL;
> > +				goto out_free;
> > +			}
> 
> How likely is this leak to happen in real world? To me it looks like
> more like a theoretical issue and could have easily waited for 4.14. But
> it's fine this time, just something to keep in mind in the future.

This is a one-liner fix and I consider memory leaks serious enough to
deserve a fix for rc5.  This bug can happen with broken ACPI tables and,
trust me, broken ACPI tables are not that hard to find.

But you rule here, feel free to NACK my patches whenever you see fit! :)

--
Cheers,
Luca.

Re: [PATCH 1/3] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'

[Kalle Valo]

Luca Coelho <luca@coelho.fi> writes:

> On Mon, 2017-08-07 at 15:57 +0300, Kalle Valo wrote:
>> Luca Coelho <luca@coelho.fi> writes:
>> 
>> > From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
>> > 
>> > We should free 'wgds.pointer' here as done a few lines above in another
>> > error handling path.
>> > It was allocated within 'acpi_evaluate_object()'.
>> > 
>> > Fixes: c52030a01ccc ("iwlwifi: mvm: add GEO_TX_POWER_LIMIT cmd for geographic tx power table")
>> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
>> > ---
>> >  drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 6 ++++--
>> >  1 file changed, 4 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
>> > index 79e7a7a285dc..82863e9273eb 100644
>> > --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
>> > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
>> > @@ -1275,8 +1275,10 @@ static int iwl_mvm_sar_get_wgds_table(struct iwl_mvm *mvm)
>> >  
>> >  			entry = &wifi_pkg->package.elements[idx++];
>> >  			if ((entry->type != ACPI_TYPE_INTEGER) ||
>> > -			    (entry->integer.value > U8_MAX))
>> > -				return -EINVAL;
>> > +			    (entry->integer.value > U8_MAX)) {
>> > +				ret = -EINVAL;
>> > +				goto out_free;
>> > +			}
>> 
>> How likely is this leak to happen in real world? To me it looks like
>> more like a theoretical issue and could have easily waited for 4.14. But
>> it's fine this time, just something to keep in mind in the future.
>
> This is a one-liner fix and I consider memory leaks serious enough to
> deserve a fix for rc5.  This bug can happen with broken ACPI tables and,
> trust me, broken ACPI tables are not that hard to find.

Sure, anything's possible. But what I'm reading here this is still a
theoretical issue, not a leak which we _know_ will happen to thousands
of people.

> But you rule here, feel free to NACK my patches whenever you see fit! :)

I'm trying to minimise the numbers of patches going to wireless-drivers
and striving for only fixes which really matter and keep the theoretical
stuff for -next. The is mostly selfish reasons as wireless-drivers are a
lot more work, especially if there are conflicts.

But like I said in my previous mail, no need to drop this.

-- 
Kalle Valo

Re: [PATCH 1/3] iwlwifi: mvm: Fix a memory leak in an error handling path in 'iwl_mvm_sar_get_wgds_table()'

[Luca Coelho]

On Mon, 2017-08-07 at 16:37 +0300, Kalle Valo wrote:
> Luca Coelho <luca@coelho.fi> writes:
> 
> > On Mon, 2017-08-07 at 15:57 +0300, Kalle Valo wrote:
> > > Luca Coelho <luca@coelho.fi> writes:
> > > 
> > > > From: Christophe Jaillet <christophe.jaillet@wanadoo.fr>
> > > > 
> > > > We should free 'wgds.pointer' here as done a few lines above in another
> > > > error handling path.
> > > > It was allocated within 'acpi_evaluate_object()'.
> > > > 
> > > > Fixes: c52030a01ccc ("iwlwifi: mvm: add GEO_TX_POWER_LIMIT cmd for geographic tx power table")
> > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > > ---
> > > >  drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > > > index 79e7a7a285dc..82863e9273eb 100644
> > > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
> > > > @@ -1275,8 +1275,10 @@ static int iwl_mvm_sar_get_wgds_table(struct iwl_mvm *mvm)
> > > >  
> > > >  			entry = &wifi_pkg->package.elements[idx++];
> > > >  			if ((entry->type != ACPI_TYPE_INTEGER) ||
> > > > -			    (entry->integer.value > U8_MAX))
> > > > -				return -EINVAL;
> > > > +			    (entry->integer.value > U8_MAX)) {
> > > > +				ret = -EINVAL;
> > > > +				goto out_free;
> > > > +			}
> > > 
> > > How likely is this leak to happen in real world? To me it looks like
> > > more like a theoretical issue and could have easily waited for 4.14. But
> > > it's fine this time, just something to keep in mind in the future.
> > 
> > This is a one-liner fix and I consider memory leaks serious enough to
> > deserve a fix for rc5.  This bug can happen with broken ACPI tables and,
> > trust me, broken ACPI tables are not that hard to find.
> 
> Sure, anything's possible. But what I'm reading here this is still a
> theoretical issue, not a leak which we _know_ will happen to thousands
> of people.
> 
> > But you rule here, feel free to NACK my patches whenever you see fit! :)
> 
> I'm trying to minimise the numbers of patches going to wireless-drivers
> and striving for only fixes which really matter and keep the theoretical
> stuff for -next. The is mostly selfish reasons as wireless-drivers are a
> lot more work, especially if there are conflicts.

I totally understand.  It's a lot more work for me too, with all the
reordering I need to do and the conflicts these cause.


> But like I said in my previous mail, no need to drop this.

Okay, thanks!

--
Cheers,
Luca.

[PATCH 2/3] iwlwifi: mvm: start mac queues when deferred tx frames are purged

[Luca Coelho]

From: Avraham Stern <avraham.stern@intel.com>

Under DQA, when tx is deferred because a queue needs to be allocated,
the mac queue for that TID is stopped until the new stream is added.
If at this point the station that this stream belongs to is removed,
all the deferred tx frames are purged, but the mac queue is not
restarted. As a result, all following tx on this queue will not be
transmitted.

Fix this by starting the relevant mac queues when the deferred tx
frames are purged.

Fixes: 24afba7690e4 ("iwlwifi: mvm: support bss dynamic alloc/dealloc of queues")
Signed-off-by: Avraham Stern <avraham.stern@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
index c7b1e58e3384..ce901be5fba8 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
@@ -2597,8 +2597,18 @@ static void iwl_mvm_purge_deferred_tx_frames(struct iwl_mvm *mvm,
 	spin_lock_bh(&mvm_sta->lock);
 	for (i = 0; i <= IWL_MAX_TID_COUNT; i++) {
 		tid_data = &mvm_sta->tid_data[i];
-		while ((skb = __skb_dequeue(&tid_data->deferred_tx_frames)))
+
+		while ((skb = __skb_dequeue(&tid_data->deferred_tx_frames))) {
+			struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+
+			/*
+			 * The first deferred frame should've stopped the MAC
+			 * queues, so we should never get a second deferred
+			 * frame for the RA/TID.
+			 */
+			iwl_mvm_start_mac_queues(mvm, info->hw_queue);
 			ieee80211_free_txskb(mvm->hw, skb);
+		}
 	}
 	spin_unlock_bh(&mvm_sta->lock);
 }
-- 
2.13.2

Re: [PATCH 2/3] iwlwifi: mvm: start mac queues when deferred tx frames are purged

[Kalle Valo]

Luca Coelho <luca@coelho.fi> writes:

> From: Avraham Stern <avraham.stern@intel.com>
>
> Under DQA, when tx is deferred because a queue needs to be allocated,
> the mac queue for that TID is stopped until the new stream is added.
> If at this point the station that this stream belongs to is removed,
> all the deferred tx frames are purged, but the mac queue is not
> restarted. As a result, all following tx on this queue will not be
> transmitted.
>
> Fix this by starting the relevant mac queues when the deferred tx
> frames are purged.
>
> Fixes: 24afba7690e4 ("iwlwifi: mvm: support bss dynamic alloc/dealloc of queues")
> Signed-off-by: Avraham Stern <avraham.stern@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>

The commit log is not really explaining the bug from user's point of
view.

-- 
Kalle Valo

[PATCH 3/3] iwlwifi: mvm: don't WARN when a legit race happens in A-MPDU

[Luca Coelho]

From: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

When we start an Rx A-MPDU session, we first get the AddBA
request, then we send an ADD_STA command to the firmware
that will reply with a BAID which is a hardware resource
that tracks the BA session.
This BAID will appear on each and every frame that we get
from the firwmare until the A-MPDU session is torn down.
In the Rx path, we look at this BAID to manage the
reordering buffer.

This flow is inherently racy since the hardware will start
to put the BAID in the frames it receives even if the
firmware hasn't sent the response to the ADD_STA command.
This basically means that the driver can get frames with
a valid BAID that it doesn't know yet.
When that happens, the driver used to WARN.
Fix this by simply not WARN in this case. When the driver
will know abou the BAID, it will initialise the relevant
states and the next frame with a valid BAID will refresh
them.

Fixes: b915c10174fb ("iwlwifi: mvm: add reorder buffer per queue")
Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
index f3e608196369..71c8b800ffa9 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
@@ -636,9 +636,9 @@ static bool iwl_mvm_reorder(struct iwl_mvm *mvm,
 
 	baid_data = rcu_dereference(mvm->baid_map[baid]);
 	if (!baid_data) {
-		WARN(!(reorder & IWL_RX_MPDU_REORDER_BA_OLD_SN),
-		     "Received baid %d, but no data exists for this BAID\n",
-		     baid);
+		IWL_DEBUG_RX(mvm,
+			     "Got valid BAID but no baid allocated, bypass the re-ordering buffer. Baid %d reorder 0x%x\n",
+			      baid, reorder);
 		return false;
 	}
 
@@ -759,7 +759,9 @@ static void iwl_mvm_agg_rx_received(struct iwl_mvm *mvm,
 
 	data = rcu_dereference(mvm->baid_map[baid]);
 	if (!data) {
-		WARN_ON(!(reorder_data & IWL_RX_MPDU_REORDER_BA_OLD_SN));
+		IWL_DEBUG_RX(mvm,
+			     "Got valid BAID but no baid allocated, bypass the re-ordering buffer. Baid %d reorder 0x%x\n",
+			      baid, reorder_data);
 		goto out;
 	}
 
-- 
2.13.2