[PATCH v2] ath9k: decrypt_error flag issue

[PATCH v2] ath9k: decrypt_error flag issue

[Lorenzo Bianconi]

From: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>

After the hw reports a decryption error, the flag decrypt_error is set to
true in ath9k_rx_accept.
Since this flag is initialized to false just outside ath_rx_tasklet while cycle,
all subsequent frames are marked as corrupted until ath_rx_tasklet ends.
Fix the issue initializing decrypt_error flag at the begging of the cycle.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
---
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1044,7 +1044,6 @@
 	struct ieee80211_hw *hw = sc->hw;
 	struct ieee80211_hdr *hdr;
 	int retval;
-	bool decrypt_error = false;
 	struct ath_rx_status rs;
 	enum ath9k_rx_qtype qtype;
 	bool edma = !!(ah->caps.hw_caps & ATH9K_HW_CAP_EDMA);
@@ -1066,6 +1065,7 @@
 	tsf_lower = tsf & 0xffffffff;

 	do {
+		bool decrypt_error = false;
 		/* If handling rx interrupt and flush is in progress => exit */
 		if (test_bit(SC_OP_RXFLUSH, &sc->sc_flags) && (flush == 0))
 			break;

Re: [PATCH v2] ath9k: decrypt_error flag issue

[Luis R. Rodriguez]

On Thu, Aug 09, 2012 at 05:47:47PM +0200, Lorenzo Bianconi wrote:
> From: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
> 
> After the hw reports a decryption error, the flag decrypt_error is set to
> true in ath9k_rx_accept.
> Since this flag is initialized to false just outside ath_rx_tasklet while cycle,
> all subsequent frames are marked as corrupted until ath_rx_tasklet ends.
> Fix the issue initializing decrypt_error flag at the begging of the cycle.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>

Thanks! In practice what issues did you see ?

  Luis

Re: [PATCH v2] ath9k: decrypt_error flag issue

[Lorenzo Bianconi]

> On Thu, Aug 09, 2012 at 05:47:47PM +0200, Lorenzo Bianconi wrote:
>> From: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
>>
>> After the hw reports a decryption error, the flag decrypt_error is set to
>> true in ath9k_rx_accept.
>> Since this flag is initialized to false just outside ath_rx_tasklet while cycle,
>> all subsequent frames are marked as corrupted until ath_rx_tasklet ends.
>> Fix the issue initializing decrypt_error flag at the begging of the cycle.
>>
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
>
> Thanks! In practice what issues did you see ?
>
>   Luis

If the decrypt_error flag is set to true for correctly decrypted
packets, the ieee80211_rx_status flag is not marked with
RX_FLAG_DECRYPTED in ath9k_rx_skb_postprocess.
This behavior causes some issues. For example mac80211 attempts to
decrypt the frame in software even if the packet is already decrypted
in hw.

Regards

Lorenzo

Re: [PATCH v2] ath9k: decrypt_error flag issue

[Luis R. Rodriguez]

On Thu, Aug 9, 2012 at 11:06 AM, Lorenzo Bianconi
<lorenzo.bianconi83@gmail.com> wrote:
>> On Thu, Aug 09, 2012 at 05:47:47PM +0200, Lorenzo Bianconi wrote:
>>> From: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
>>>
>>> After the hw reports a decryption error, the flag decrypt_error is set to
>>> true in ath9k_rx_accept.
>>> Since this flag is initialized to false just outside ath_rx_tasklet while cycle,
>>> all subsequent frames are marked as corrupted until ath_rx_tasklet ends.
>>> Fix the issue initializing decrypt_error flag at the begging of the cycle.
>>>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
>>
>> Thanks! In practice what issues did you see ?
>>
>>   Luis
>
> If the decrypt_error flag is set to true for correctly decrypted
> packets, the ieee80211_rx_status flag is not marked with
> RX_FLAG_DECRYPTED in ath9k_rx_skb_postprocess.
> This behavior causes some issues. For example mac80211 attempts to
> decrypt the frame in software even if the packet is already decrypted
> in hw.

And this would lead to .. what? How did you realize this? Can you
please resend and add all this information to the commit log message?

  Luis

Re: [PATCH v2] ath9k: decrypt_error flag issue

[Pavel Roskin]

On Thu, 9 Aug 2012 11:10:38 -0700
"Luis R. Rodriguez" <rodrigue@qca.qualcomm.com> wrote:

> And this would lead to .. what? How did you realize this? Can you
> please resend and add all this information to the commit log message?

Also please use a better subject.  For example:

ath9k: fix decrypt_error initialization in ath_rx_tasklet()

"issue" is too vague.

-- 
Regards,
Pavel Roskin

Re: [PATCH v2] ath9k: decrypt_error flag issue

[Luis R. Rodriguez]

On Thu, Aug 9, 2012 at 2:07 PM, Pavel Roskin <proski@gnu.org> wrote:
> On Thu, 9 Aug 2012 11:10:38 -0700
> "Luis R. Rodriguez" <rodrigue@qca.qualcomm.com> wrote:
>
>> And this would lead to .. what? How did you realize this? Can you
>> please resend and add all this information to the commit log message?
>
> Also please use a better subject.  For example:
>
> ath9k: fix decrypt_error initialization in ath_rx_tasklet()
>
> "issue" is too vague.

Also -- what I was getting at is to evaluate whether or not this is an
important fix or critical. To determine if its critical it helps to
understand exactly what negative behavior was observed. If its
critical it can go to stable but I have a feeling this is not
critical. If its not critical and only important although it won't go
to stable I'll still cherry pick it for the stable compat-wireless
releases.

  Luis

Re: [PATCH v2] ath9k: decrypt_error flag issue

[Felix Fietkau]

On 2012-08-09 11:21 PM, Luis R. Rodriguez wrote:
> On Thu, Aug 9, 2012 at 2:07 PM, Pavel Roskin <proski@gnu.org> wrote:
>> On Thu, 9 Aug 2012 11:10:38 -0700
>> "Luis R. Rodriguez" <rodrigue@qca.qualcomm.com> wrote:
>>
>>> And this would lead to .. what? How did you realize this? Can you
>>> please resend and add all this information to the commit log message?
>>
>> Also please use a better subject.  For example:
>>
>> ath9k: fix decrypt_error initialization in ath_rx_tasklet()
>>
>> "issue" is too vague.
> 
> Also -- what I was getting at is to evaluate whether or not this is an
> important fix or critical. To determine if its critical it helps to
> understand exactly what negative behavior was observed. If its
> critical it can go to stable but I have a feeling this is not
> critical. If its not critical and only important although it won't go
> to stable I'll still cherry pick it for the stable compat-wireless
> releases.
I think it's critical enough for stable. I think when using CCMP
encryption, high CPU load can get the connection stuck (because of CCMP
PN corruption when the issue occurs).

- Felix

Re: [PATCH v2] ath9k: decrypt_error flag issue

[Lorenzo Bianconi]

> On 2012-08-09 11:21 PM, Luis R. Rodriguez wrote:
>> On Thu, Aug 9, 2012 at 2:07 PM, Pavel Roskin <proski@gnu.org> wrote:
>>> On Thu, 9 Aug 2012 11:10:38 -0700
>>> "Luis R. Rodriguez" <rodrigue@qca.qualcomm.com> wrote:
>>>
>>>> And this would lead to .. what? How did you realize this? Can you
>>>> please resend and add all this information to the commit log message?
>>>
>>> Also please use a better subject.  For example:
>>>
>>> ath9k: fix decrypt_error initialization in ath_rx_tasklet()
>>>
>>> "issue" is too vague.
>>
>> Also -- what I was getting at is to evaluate whether or not this is an
>> important fix or critical. To determine if its critical it helps to
>> understand exactly what negative behavior was observed. If its
>> critical it can go to stable but I have a feeling this is not
>> critical. If its not critical and only important although it won't go
>> to stable I'll still cherry pick it for the stable compat-wireless
>> releases.
> I think it's critical enough for stable. I think when using CCMP
> encryption, high CPU load can get the connection stuck (because of CCMP
> PN corruption when the issue occurs).
>
> - Felix
>

 I was using a two hop network encrypted with AES CCMP.

STA <---f0---> Node A <---f0---> Node B   f0=2412GHz

Node A and node B were running latest OpenWRT trunk, AR9280 chipset.
I was injecting 30Mbps of UDP iperf traffic from STA to Node B.
I was facing a connection stuck issue on Node B side because it
reports a lot of decrypt errors
and since decrypt_error flag sets to true, it tries to decrypt an
already decrypted frame with ieee80211_aes_ccm_decrypt. Setting
decrypt_error to false at beginning of ath_rx_tasklet cycle
the connection is definitely more stable avoiding waste of CPU time.

Regards

Lorenzo

Re: [PATCH v2] ath9k: decrypt_error flag issue

[Luis R. Rodriguez]

On Thu, Aug 9, 2012 at 3:06 PM, Lorenzo Bianconi
<lorenzo.bianconi83@gmail.com> wrote:
>> On 2012-08-09 11:21 PM, Luis R. Rodriguez wrote:
>>> On Thu, Aug 9, 2012 at 2:07 PM, Pavel Roskin <proski@gnu.org> wrote:
>>>> On Thu, 9 Aug 2012 11:10:38 -0700
>>>> "Luis R. Rodriguez" <rodrigue@qca.qualcomm.com> wrote:
>>>>
>>>>> And this would lead to .. what? How did you realize this? Can you
>>>>> please resend and add all this information to the commit log message?
>>>>
>>>> Also please use a better subject.  For example:
>>>>
>>>> ath9k: fix decrypt_error initialization in ath_rx_tasklet()
>>>>
>>>> "issue" is too vague.
>>>
>>> Also -- what I was getting at is to evaluate whether or not this is an
>>> important fix or critical. To determine if its critical it helps to
>>> understand exactly what negative behavior was observed. If its
>>> critical it can go to stable but I have a feeling this is not
>>> critical. If its not critical and only important although it won't go
>>> to stable I'll still cherry pick it for the stable compat-wireless
>>> releases.
>> I think it's critical enough for stable. I think when using CCMP
>> encryption, high CPU load can get the connection stuck (because of CCMP
>> PN corruption when the issue occurs).
>>
>> - Felix
>>
>
>  I was using a two hop network encrypted with AES CCMP.
>
> STA <---f0---> Node A <---f0---> Node B   f0=2412GHz
>
> Node A and node B were running latest OpenWRT trunk, AR9280 chipset.
> I was injecting 30Mbps of UDP iperf traffic from STA to Node B.
> I was facing a connection stuck issue on Node B side because it
> reports a lot of decrypt errors
> and since decrypt_error flag sets to true, it tries to decrypt an
> already decrypted frame with ieee80211_aes_ccm_decrypt. Setting
> decrypt_error to false at beginning of ath_rx_tasklet cycle
> the connection is definitely more stable avoiding waste of CPU time.

Can you resubmit with all this information and also what Felix
mentioned and also also add:

Cc: stable@vger.kernel.org

as part of your commit log message (note: commit log, not e-mail).

  Luis