Re: [PATCH v8] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Johannes Berg]

> Well, the TXQ already adds a lot of other overhead (hashing on the
> packet header, for one), so my guess would be that this would be
> negligible compared to all that? 
> 
> > 
> > I suppose I don't have to care all that much about the TXQs, but
> > ...
> > 
> > Then again, adding a field in the skb->cb for the sake of this? No,
> > not really either.
> 
> So that's a "keep it", then? :)

Yeah I think so :)

> > > + ieee80211_xmit_fast_finish(sta->sdata, sta,
> > > pn_offs,
> > > +    info->control.hw_key, 
> > > skb);
> > 
> > I don't see how keeping the info->control.hw_key pointer across the
> > TXQ/FQ/Codel queueing isn't a potential bug? Probably one that
> > already exists in your code today, before this patch, of course.
> 
> You mean the key could get removed from the hardware while the packet
> was queued? Can certainly add a check for that. Under what conditions
> does that happen? Does it make sense to try to recover from it (I
> guess by calling tx_h_select_key), or is it rare enough that giving
> up and dropping the packet makes more sense?

Not just from the hardware, more importantly the whole key structure
can be kfree()d, leading to use-after-free here, no?

Fast-xmit solves this by invalidating the fast-xmit cache when the key
pointer changes/goes away and possibly punting some frames to the slow
path, but you've absolutely no protection on these pointers here within
the TXQs, afaict?

A similar situation occurs with other pointers, like stations and vifs,
but when those are removed then obviously the entire TXQs are flushed,
so they're not relevant.

With the key though, frames can be on the queue while a key is removed,
and even before this patch, drivers would consequently access an
invalid key pointer.

Mind you, as I just wrote I think that issue exists even before this
patch, so you should probably look at it separately. Felix might know
better too.

johannes

Re: [PATCH v8] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

>> +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx);
>> +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata,
>> +   struct sta_info *sta, u8 pn_offs,
>> +   struct ieee80211_key_conf *key_conf,
>> +   struct sk_buff *skb);
>> +
>
> I'm not very happy with this - I think you should do some
> refactoring/code move in a separate prior patch to avoid this.

Noted, will do.

>> +if (txq->sta && info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {
>>  struct sta_info *sta = container_of(txq->sta, struct sta_info,
>>  sta);
>> -struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
>> +u8 pn_offs = 0;
>>  
>> -hdr->seq_ctrl = ieee80211_tx_next_seq(sta, txq->tid);
>> -if (test_bit(IEEE80211_TXQ_AMPDU, >flags))
>> -info->flags |= IEEE80211_TX_CTL_AMPDU;
>> -else
>> -info->flags &= ~IEEE80211_TX_CTL_AMPDU;
>> +if (info->control.hw_key)
>> +pn_offs = ieee80211_hdrlen(hdr->frame_control);
>
> Not very happy with this either - the fast-xmit path explicitly tries
> to avoid all these calculations.

Well, the TXQ already adds a lot of other overhead (hashing on the
packet header, for one), so my guess would be that this would be
negligible compared to all that? 

> I suppose I don't have to care all that much about the TXQs, but ...
>
> Then again, adding a field in the skb->cb for the sake of this? No,
> not really either.

So that's a "keep it", then? :)

>> +ieee80211_xmit_fast_finish(sta->sdata, sta, pn_offs,
>> +   info->control.hw_key, skb);
>
> I don't see how keeping the info->control.hw_key pointer across the
> TXQ/FQ/Codel queueing isn't a potential bug? Probably one that already
> exists in your code today, before this patch, of course.

You mean the key could get removed from the hardware while the packet
was queued? Can certainly add a check for that. Under what conditions
does that happen? Does it make sense to try to recover from it (I guess
by calling tx_h_select_key), or is it rare enough that giving up and
dropping the packet makes more sense?

>> +} else {
>> +struct ieee80211_tx_data tx = { };
>> +
>> +__skb_queue_head_init();
>> +tx.local = local;
>> +tx.skb = skb;
>
> an empty initializer is weird - why not at least move local/skb
> initializations into it? Even txq->sta, I guess, since you can assign
> txq->sta either way.

Yup, makes sense. Noted.

>> -CALL_TXH(ieee80211_tx_h_select_key);
>> +
>>  if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
>>  CALL_TXH(ieee80211_tx_h_rate_ctrl);
> [...]
>>  if (unlikely(info->flags & IEEE80211_TX_INTFL_RETRANSMISSION)) {
>>  __skb_queue_tail(>skbs, tx->skb);
>>  tx->skb = NULL;
>>  goto txh_done;
>>  }
>> 
>> +CALL_TXH(ieee80211_tx_h_select_key);
>
> What happens for the IEEE80211_TX_INTFL_RETRANSMISSION packets wrt.
> key selection? Why is it OK to change this?

You're right, that's an oversight on my part. Will fix.

-Toke

Re: [PATCH v8] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Johannes Berg]

> +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx);
> +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata,
> +    struct sta_info *sta, u8 pn_offs,
> +    struct ieee80211_key_conf *key_conf,
> +    struct sk_buff *skb);
> +

I'm not very happy with this - I think you should do some
refactoring/code move in a separate prior patch to avoid this.

> + if (txq->sta && info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {
>   struct sta_info *sta = container_of(txq->sta, struct sta_info,
>   sta);
> - struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> + u8 pn_offs = 0;
>  
> - hdr->seq_ctrl = ieee80211_tx_next_seq(sta, txq->tid);
> - if (test_bit(IEEE80211_TXQ_AMPDU, >flags))
> - info->flags |= IEEE80211_TX_CTL_AMPDU;
> - else
> - info->flags &= ~IEEE80211_TX_CTL_AMPDU;
> + if (info->control.hw_key)
> + pn_offs = ieee80211_hdrlen(hdr->frame_control);

Not very happy with this either - the fast-xmit path explicitly tries
to avoid all these calculations.

I suppose I don't have to care all that much about the TXQs, but ...

Then again, adding a field in the skb->cb for the sake of this? No, not really 
either.


> + ieee80211_xmit_fast_finish(sta->sdata, sta, pn_offs,
> +    info->control.hw_key, skb);

I don't see how keeping the info->control.hw_key pointer across the
TXQ/FQ/Codel queueing isn't a potential bug? Probably one that already
exists in your code today, before this patch, of course.


> + } else {
> + struct ieee80211_tx_data tx = { };
> +
> + __skb_queue_head_init();
> + tx.local = local;
> + tx.skb = skb;

an empty initializer is weird - why not at least move local/skb
initializations into it? Even txq->sta, I guess, since you can assign
txq->sta either way.

> - CALL_TXH(ieee80211_tx_h_select_key);
> +
>   if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
>   CALL_TXH(ieee80211_tx_h_rate_ctrl);
[...]
>   if (unlikely(info->flags & IEEE80211_TX_INTFL_RETRANSMISSION)) {
>   __skb_queue_tail(>skbs, tx->skb);
>   tx->skb = NULL;
>   goto txh_done;
>   }
> 
> + CALL_TXH(ieee80211_tx_h_select_key);

What happens for the IEEE80211_TX_INTFL_RETRANSMISSION packets wrt. key
selection? Why is it OK to change this?

johannes

Re: [PATCH v8] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Felix Fietkau]

On 2016-09-06 13:44, Toke Høiland-Jørgensen wrote:
> The TXQ intermediate queues can cause packet reordering when more than
> one flow is active to a single station. Since some of the wifi-specific
> packet handling (notably sequence number and encryption handling) is
> sensitive to re-ordering, things break if they are applied before the
> TXQ.
> 
> This splits up the TX handlers and fast_xmit logic into two parts: An
> early part and a late part. The former is applied before TXQ enqueue,
> and the latter after dequeue. The non-TXQ path just applies both parts
> at once.
> 
> Because fragments shouldn't be split up or reordered, the fragmentation
> handler is run after dequeue. Any fragments are then kept in the TXQ and
> on subsequent dequeues they take precedence over dequeueing from the FQ
> structure.
> 
> This approach avoids having to scatter special cases for when TXQ is
> enabled, at the cost of making the fast_xmit and TX handler code
> slightly more complex.
> 
> Signed-off-by: Toke Høiland-Jørgensen 
Acked-by: Felix Fietkau