On 13 January 2017 at 09:16, Johannes Berg <johan...@sipsolutions.net> wrote:
> On Thu, 2017-01-12 at 15:28 +0100, Michal Kazior wrote:
>> Station structure is considered as not uploaded
>> (to driver) until drv_sta_state() finishes. This
>> call is however done after the structure is
>> attached to mac80211 internal lists and hashes.
>> This means mac80211 can lookup (and use) station
>> structure before it is uploaded to a driver.
>> If this happens (structure exists, but
>> sta->uploaded is false) fast_tx path can still be
>> taken. Deep in the fastpath call the sta->uploaded
>> is checked against to derive "pubsta" argument for
>> ieee80211_get_txq(). If sta->uploaded is false
>> (and sta is actually non-NULL) ieee80211_get_txq()
>> effectively downgraded to vif->txq.
>> At first glance this may look innocent but coerces
>> mac80211 into a state that is almost guaranteed
>> (codel may drop offending skb) to crash because a
>> station-oriented skb gets queued up on
>> vif-oriented txq. The ieee80211_tx_dequeue() ends
>> up looking at info->control.flags and tries to use
>> txq->sta which in the fail case is NULL.
>> It's probably pointless to pretend one can
>> downgrade skb from sta-txq to vif-txq.
> Ok. I understand things until this point, more or less.
> What I don't understand - and you haven't really described - is how the
> changes fix it? Could you resend with a paragraph added that explains
"Since downgrading unicast traffic to vif->txq must not be done
there's no txq to put a frame on if sta->uploaded is false. Therefore
the code is made to fall back to regular tx() op path if the described
condition is hit. " -- is this sufficient?
> Also, you're adding a test:
>> if (sta && !sta->uploaded)
> but couldn't do move that into the existing "if (sta)" block?
> Everything before that only ever returns NULL anyway.
Good point. It makes more sense to put the sta->uploaded check in if
(sta) block. I'll move it.