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
