[PATCH] ath10k: prevent sta pointer rcu violation
michal.kazior at tieto.com
Thu Jan 12 08:18:57 PST 2017
On 12 January 2017 at 16:46, Johannes Berg <johannes at sipsolutions.net> wrote:
> On Thu, 2017-01-12 at 16:14 +0100, Michal Kazior wrote:
>> Station pointers are RCU protected so driver must
>> be extra careful if it tries to store them
>> internally for later use outside of the RCU
>> section it obtained it in.
>> It was possible for station teardown to race with
>> some htt events. The possible outcome could be a
>> use-after-free and a crash.
>> Only peer-flow-control capable firmware was
>> affected (so hardware-wise qca99x0 and qca4019).
>> This could be done in sta_state() itself via
>> explicit synchronize_net() call but there's
>> already a convenient sta_pre_rcu_remove() op that
>> can be hooked up to avoid extra rcu stall.
> I don't think this makes sense. You're not using RCU-protected pointers
> to the stations yourself, all accesses to them are locked under the
> &ar->data_lock. As a consequence, you can't have any need for waiting
> for a grace period. Since you also remove the pointer (under lock) when
> the station gets removed, I don't think RCU can be the problem?
Unless you then continue to use that sta pointer after you release
data_lock. Consider this:
> CPU0 CPU1
> 1 synchronize_net()
> 2 drv_sta_state()
> 3 htt_fetch_ind(pid,tid) called
> 4 rcu_read_lock()
> 5 get(data_lock)
> 6 txq=peers[pid]->sta->txq[tid]
> 7 put(data_lock)
> 8 get(data_lock)
> 9 peer->sta=0
> 10 put(data_lock)
> 11 kfree(sta)
> 12 ieee80211_tx_dequeue(txq)
Even though there's no code like (9) per se you can think of it as
anything that tries to "remove" the peer--sta association (ath10k_peer
is removed implicitly via wmi peer delete command and waiting for htt
Holding data_lock for the entire duration of handling fetch indication
isn't really good for performance so it's better to fix RCU handling.
More information about the ath10k