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

From: Mark Asselstine <asselsm@gmail.com>
To: Luca Coelho <luca@coelho.fi>
Cc: kvalo@codeaurora.org, linux-wireless <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v5.7 8/8] iwlwifi: mvm: don't call iwl_mvm_free_inactive_queue() under RCU
Date: Fri, 3 Apr 2020 10:28:37 -0400	[thread overview]
Message-ID: <CAPuovE+_jWPjesRZ0GFk0zjf-jLmyyqOGxwyZGcawg5DL9CbXg@mail.gmail.com> (raw)
In-Reply-To: <iwlwifi.20200403112332.0f49448c133d.I17fd308bc4a9491859c9b112f4eb5d2c3fc18d7d@changeid>

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 <luca@coelho.fi> wrote:
>
> From: Johannes Berg <johannes.berg@intel.com>
>
> 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: stable@vger.kernel.org # v5.4+
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  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
>