[v5.7,8/8] iwlwifi: mvm: don't call iwl_mvm_free_inactive_queue() under RCU

Commit Message

Luca Coelho April 3, 2020, 8:29 a.m. UTC

From: Johannes Berg <[e-mail]>

iwl_mvm_free_inactive_queue() will sleep in synchronize_net() under
some circumstances, so don't call it under RCU. There doesn't appear
to be a need for RCU protection around this particular call.

Cc: [e-mail] # v5.4+
Signed-off-by: Johannes Berg <[e-mail]>
Signed-off-by: Luca Coelho <[e-mail]>
---
 drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Mark Asselstine April 3, 2020, 2:28 p.m. UTC | #1

I was looking into this as part of
https://bugzilla.kernel.org/show_bug.cgi?id=206971 and had a similar
fix in flight. My concern was that queue_owner being used outside of
the RCU might be an issue as now you have no guaranty that the
eventual use of sta->txq[tid] in iwl_mvm_free_inactive_queue() is
going to be valid. The only way to work around this is instead of
storing queue_owner, store mvmtxq = iwl_mvm_txq_from_tid(sta, i), then
adjust iwl_mvm_free_inactive_queue(), iwl_mvm_disable_txq() and
whatnot to take struct iwl_mvm_txq * instead of struct ieee80211_sta
*. If you open the bug you will see the latest version of my work as
the attached patch. I am not an RCU expert so I am curious to hear
your thoughts.


On Fri, Apr 3, 2020 at 4:31 AM Luca Coelho <[e-mail]> wrote:
>
> From: Johannes Berg <[e-mail]>
>
> iwl_mvm_free_inactive_queue() will sleep in synchronize_net() under
> some circumstances, so don't call it under RCU. There doesn't appear
> to be a need for RCU protection around this particular call.
>
> Cc: [e-mail] # v5.4+
> Signed-off-by: Johannes Berg <[e-mail]>
> Signed-off-by: Luca Coelho <[e-mail]>
> ---
>  drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> index 56ae72debb96..9ca433fdf634 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> @@ -1184,17 +1184,15 @@ static int iwl_mvm_inactivity_check(struct iwl_mvm *mvm, u8 alloc_for_sta)
>         for_each_set_bit(i, &changetid_queues, IWL_MAX_HW_QUEUES)
>                 iwl_mvm_change_queue_tid(mvm, i);
>
> +       rcu_read_unlock();
> +
>         if (free_queue >= 0 && alloc_for_sta != IWL_MVM_INVALID_STA) {
>                 ret = iwl_mvm_free_inactive_queue(mvm, free_queue, queue_owner,
>                                                   alloc_for_sta);
> -               if (ret) {
> -                       rcu_read_unlock();
> +               if (ret)
>                         return ret;
> -               }
>         }
>
> -       rcu_read_unlock();
> -
>         return free_queue;
>  }
>
> --
> 2.25.1
>

Luca Coelho April 17, 2020, 6:42 a.m. UTC | #2

On Fri, 2020-04-03 at 10:28 -0400, Mark Asselstine wrote:
> I was looking into this as part of
> https://bugzilla.kernel.org/show_bug.cgi?id=206971 and had a similar
> fix in flight. My concern was that queue_owner being used outside of
> the RCU might be an issue as now you have no guaranty that the
> eventual use of sta->txq[tid] in iwl_mvm_free_inactive_queue() is
> going to be valid. The only way to work around this is instead of
> storing queue_owner, store mvmtxq = iwl_mvm_txq_from_tid(sta, i), then
> adjust iwl_mvm_free_inactive_queue(), iwl_mvm_disable_txq() and
> whatnot to take struct iwl_mvm_txq * instead of struct ieee80211_sta
> *. If you open the bug you will see the latest version of my work as
> the attached patch. I am not an RCU expert so I am curious to hear
> your thoughts.

I asked Johannes to check your comment.  For now, I'm going to drop it
from v2 of this patchset, so we can go ahead with the other patches.

--
Cheers,
Luca.

Johannes Berg April 17, 2020, 7:52 a.m. UTC | #3

Sorry, missed your comment.

On Fri, 2020-04-03 at 10:28 -0400, Mark Asselstine wrote:
> I was looking into this as part of
> https://bugzilla.kernel.org/show_bug.cgi?id=206971 and had a similar
> fix in flight. My concern was that queue_owner being used outside of
> the RCU might be an issue

Yes, that does *look* questionable, and I missed it completely.

However, that's only because the code makes weak assumptions when it's
under much stronger guarantees. There's no reason for it to use RCU here
for the station lookup, since it's holding the write-side lock (which is
the mvm->mutex).

IOW, we could just change

sta = rcu_dereference(mvm->fw_id_to_mac_id[sta_id]);

to

sta = rcu_dereference_protected(mvm->fw_id_to_mac_id[sta_id], ...);

and then it's clear that there's no issue.

> as now you have no guaranty that the
> eventual use of sta->txq[tid] in iwl_mvm_free_inactive_queue() is
> going to be valid. The only way to work around this is instead of
> storing queue_owner, store mvmtxq = iwl_mvm_txq_from_tid(sta, i), then
> adjust iwl_mvm_free_inactive_queue(), iwl_mvm_disable_txq() and
> whatnot to take struct iwl_mvm_txq * instead of struct ieee80211_sta
> *. If you open the bug you will see the latest version of my work as
> the attached patch. I am not an RCU expert so I am curious to hear
> your thoughts.

You could do that too, but it seems overly complex to me.

johannes

Kalle Valo June 15, 2020, 2:10 p.m. UTC | #4

Luca Coelho <[e-mail]> wrote:

> From: Johannes Berg <[e-mail]>
> 
> iwl_mvm_free_inactive_queue() will sleep in synchronize_net() under
> some circumstances, so don't call it under RCU. There doesn't appear
> to be a need for RCU protection around this particular call.
> 
> Cc: [e-mail] # v5.4+
> Signed-off-by: Johannes Berg <[e-mail]>
> Signed-off-by: Luca Coelho <[e-mail]>

I'll queue this for v5.8.

Kalle Valo June 23, 2020, 8:25 a.m. UTC | #5

Luca Coelho <[e-mail]> wrote:

> From: Johannes Berg <[e-mail]>
> 
> iwl_mvm_free_inactive_queue() will sleep in synchronize_net() under
> some circumstances, so don't call it under RCU. There doesn't appear
> to be a need for RCU protection around this particular call.
> 
> Cc: [e-mail] # v5.4+
> Signed-off-by: Johannes Berg <[e-mail]>
> Signed-off-by: Luca Coelho <[e-mail]>

Patch applied to wireless-drivers.git, thanks.

fbb1461ad1d6 iwlwifi: mvm: don't call iwl_mvm_free_inactive_queue() under RCU

Patch

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
index 56ae72debb96..9ca433fdf634 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
@@ -1184,17 +1184,15 @@  static int iwl_mvm_inactivity_check(struct iwl_mvm *mvm, u8 alloc_for_sta)
 	for_each_set_bit(i, &changetid_queues, IWL_MAX_HW_QUEUES)
 		iwl_mvm_change_queue_tid(mvm, i);
 
+	rcu_read_unlock();
+
 	if (free_queue >= 0 && alloc_for_sta != IWL_MVM_INVALID_STA) {
 		ret = iwl_mvm_free_inactive_queue(mvm, free_queue, queue_owner,
 						  alloc_for_sta);
-		if (ret) {
-			rcu_read_unlock();
+		if (ret)
 			return ret;
-		}
 	}
 
-	rcu_read_unlock();
-
 	return free_queue;
 }