[PATCH] mac80211: Don't inspect Sequence Control field on control frames

[PATCH] mac80211: Don't inspect Sequence Control field on control frames

[Javier Cardona]

Per IEEE Std. 802.11-2012, Sec 8.2.4.4.1, the sequence Control field is
not present in control frames.  We noticed this problem when processing
Block Ack Requests.

Signed-off-by: Javier Cardona <javier@cozybit.com>
Signed-off-by: Javier Lopez <jlopex@cozybit.com>
---
 net/mac80211/rx.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index f975f64..bf54336 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1467,6 +1467,10 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
 
 	hdr = (struct ieee80211_hdr *)rx->skb->data;
 	fc = hdr->frame_control;
+
+	if (ieee80211_is_ctl(fc))
+		return RX_CONTINUE;
+
 	sc = le16_to_cpu(hdr->seq_ctrl);
 	frag = sc & IEEE80211_SCTL_FRAG;
 
-- 
1.7.5.4

Re: [PATCH] mac80211: Don't inspect Sequence Control field on control frames

[Christian Lamparter]

On Thursday, October 25, 2012 08:10:18 PM Javier Cardona wrote:
> Per IEEE Std. 802.11-2012, Sec 8.2.4.4.1, the sequence Control field is
> not present in control frames.  We noticed this problem when processing
> Block Ack Requests.
> 
> Signed-off-by: Javier Cardona <javier@cozybit.com>
> Signed-off-by: Javier Lopez <jlopex@cozybit.com>
> ---
>  net/mac80211/rx.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index f975f64..bf54336 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -1467,6 +1467,10 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
>  
>  	hdr = (struct ieee80211_hdr *)rx->skb->data;
>  	fc = hdr->frame_control;
> +
> +	if (ieee80211_is_ctl(fc))
> +		return RX_CONTINUE;
> +
>  	sc = le16_to_cpu(hdr->seq_ctrl);
>  	frag = sc & IEEE80211_SCTL_FRAG;
>  
hmm, I see this function also calls skb_linearize() on said
skb... Does anybody know of any possible side effects? Not
that control frames (In fact, just BlockACK Requests come
to my mind) usually so large...

Regards,
	Chr 

Re: [PATCH] mac80211: Don't inspect Sequence Control field on control frames

[Javier Cardona]

Christian,

On Thu, Oct 25, 2012 at 11:48 AM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> On Thursday, October 25, 2012 08:10:18 PM Javier Cardona wrote:
>> Per IEEE Std. 802.11-2012, Sec 8.2.4.4.1, the sequence Control field is
>> not present in control frames.  We noticed this problem when processing
>> Block Ack Requests.
>>
>> Signed-off-by: Javier Cardona <javier@cozybit.com>
>> Signed-off-by: Javier Lopez <jlopex@cozybit.com>
>> ---
>>  net/mac80211/rx.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
>> index f975f64..bf54336 100644
>> --- a/net/mac80211/rx.c
>> +++ b/net/mac80211/rx.c
>> @@ -1467,6 +1467,10 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
>>
>>       hdr = (struct ieee80211_hdr *)rx->skb->data;
>>       fc = hdr->frame_control;
>> +
>> +     if (ieee80211_is_ctl(fc))
>> +             return RX_CONTINUE;
>> +
>>       sc = le16_to_cpu(hdr->seq_ctrl);
>>       frag = sc & IEEE80211_SCTL_FRAG;
>>
> hmm, I see this function also calls skb_linearize() on said
> skb... Does anybody know of any possible side effects? Not
> that control frames (In fact, just BlockACK Requests come
> to my mind) usually so large...

skb_linearize() is only called on fragmented frames, which is how
regular BlockAckRequests were being processed before.
We are setting new flags introduced in 11aa, which is what caused
these new BARs to be mistakenly processed as fragments.

With our patch "regular" BARs (which are the only type of control
frames that hit mac80211) continue to be processed in the same way.

Cheers,

Javier

-- 
Javier Cardona
cozybit Inc.
http://www.cozybit.com

Re: [PATCH] mac80211: Don't inspect Sequence Control field on control frames

[Christian Lamparter]

On Thursday, October 25, 2012 09:03:42 PM Javier Cardona wrote:
> Christian,
> 
> On Thu, Oct 25, 2012 at 11:48 AM, Christian Lamparter
> <chunkeey@googlemail.com> wrote:
> > On Thursday, October 25, 2012 08:10:18 PM Javier Cardona wrote:
> >> Per IEEE Std. 802.11-2012, Sec 8.2.4.4.1, the sequence Control field is
> >> not present in control frames.  We noticed this problem when processing
> >> Block Ack Requests.
> >>
> >> Signed-off-by: Javier Cardona <javier@cozybit.com>
> >> Signed-off-by: Javier Lopez <jlopex@cozybit.com>
> >> ---
> >>  net/mac80211/rx.c |    4 ++++
> >>  1 files changed, 4 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> >> index f975f64..bf54336 100644
> >> --- a/net/mac80211/rx.c
> >> +++ b/net/mac80211/rx.c
> >> @@ -1467,6 +1467,10 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
> >>
> >>       hdr = (struct ieee80211_hdr *)rx->skb->data;
> >>       fc = hdr->frame_control;
> >> +
> >> +     if (ieee80211_is_ctl(fc))
> >> +             return RX_CONTINUE;
> >> +
> >>       sc = le16_to_cpu(hdr->seq_ctrl);
> >>       frag = sc & IEEE80211_SCTL_FRAG;
> >>
> > hmm, I see this function also calls skb_linearize() on said
> > skb... Does anybody know of any possible side effects? Not
> > that control frames (In fact, just BlockACK Requests come
> > to my mind) usually so large...
> 
> skb_linearize() is only called on fragmented frames, which is how
> regular BlockAckRequests were being processed before.
Actually, I checked ieee80211_rx_h_ctrl and the back_req handler uses
skb_copy_bits so it doesn't need a linearized skb to start with ;).

Regards,
	Chr

Re: [PATCH] mac80211: Don't inspect Sequence Control field on control frames

[Johannes Berg]

On Thu, 2012-10-25 at 11:10 -0700, Javier Cardona wrote:
> Per IEEE Std. 802.11-2012, Sec 8.2.4.4.1, the sequence Control field is
> not present in control frames.  We noticed this problem when processing
> Block Ack Requests.
> 

Cc stable?

>  	hdr = (struct ieee80211_hdr *)rx->skb->data;
>  	fc = hdr->frame_control;
> +
> +	if (ieee80211_is_ctl(fc))
> +		return RX_CONTINUE;

Shouldn't that be "goto out"? And it seems it should also incorporate
the skb->len check here, rather than accessing the field before checking
that it's present?

johannes

Re: [PATCH] mac80211: Don't inspect Sequence Control field on control frames

[Javier Cardona]

Johannes,

On Thu, Oct 25, 2012 at 12:03 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2012-10-25 at 11:10 -0700, Javier Cardona wrote:
>> Per IEEE Std. 802.11-2012, Sec 8.2.4.4.1, the sequence Control field is
>> not present in control frames.  We noticed this problem when processing
>> Block Ack Requests.
>>
>
> Cc stable?

Sure.

>>       hdr = (struct ieee80211_hdr *)rx->skb->data;
>>       fc = hdr->frame_control;
>> +
>> +     if (ieee80211_is_ctl(fc))
>> +             return RX_CONTINUE;
>
> Shouldn't that be "goto out"?

If we goto out, we'll increment the rx_packets counter, which
according to sta_info.h should only count MSDUs.

> And it seems it should also incorporate the skb->len check here, rather than accessing the field before checking
> that it's present?

ieee80211_rx_h_check() zaps all skbs with len < 16, so I don't think
it's needed, no?

Javier

-- 
Javier Cardona
cozybit Inc.
http://www.cozybit.com

Re: [PATCH] mac80211: Don't inspect Sequence Control field on control frames

[Johannes Berg]

On Thu, 2012-10-25 at 12:12 -0700, Javier Cardona wrote:

> >>       hdr = (struct ieee80211_hdr *)rx->skb->data;
> >>       fc = hdr->frame_control;
> >> +
> >> +     if (ieee80211_is_ctl(fc))
> >> +             return RX_CONTINUE;
> >
> > Shouldn't that be "goto out"?
> 
> If we goto out, we'll increment the rx_packets counter, which
> according to sta_info.h should only count MSDUs.

Ok.

> > And it seems it should also incorporate the skb->len check here, rather than accessing the field before checking
> > that it's present?
> 
> ieee80211_rx_h_check() zaps all skbs with len < 16, so I don't think
> it's needed, no?

But the sequence control field is at offset 22, I think?

johannes

Re: [PATCH] mac80211: Don't inspect Sequence Control field on control frames

[Johannes Berg]

On Thu, 2012-10-25 at 11:10 -0700, Javier Cardona wrote:
> Per IEEE Std. 802.11-2012, Sec 8.2.4.4.1, the sequence Control field is
> not present in control frames.  We noticed this problem when processing
> Block Ack Requests.
> 
> Signed-off-by: Javier Cardona <javier@cozybit.com>
> Signed-off-by: Javier Lopez <jlopex@cozybit.com>
> ---
>  net/mac80211/rx.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index f975f64..bf54336 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -1467,6 +1467,10 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
>  
>  	hdr = (struct ieee80211_hdr *)rx->skb->data;
>  	fc = hdr->frame_control;
> +
> +	if (ieee80211_is_ctl(fc))

Different question -- why check _is_ctl rather than !_is_data?

johannes

Re: [PATCH] mac80211: Don't inspect Sequence Control field on control frames

[Javier Cardona]

On Thu, Oct 25, 2012 at 12:43 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2012-10-25 at 11:10 -0700, Javier Cardona wrote:
>> Per IEEE Std. 802.11-2012, Sec 8.2.4.4.1, the sequence Control field is
>> not present in control frames.  We noticed this problem when processing
>> Block Ack Requests.
>>
>> Signed-off-by: Javier Cardona <javier@cozybit.com>
>> Signed-off-by: Javier Lopez <jlopex@cozybit.com>
>> ---
>>  net/mac80211/rx.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
>> index f975f64..bf54336 100644
>> --- a/net/mac80211/rx.c
>> +++ b/net/mac80211/rx.c
>> @@ -1467,6 +1467,10 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
>>
>>       hdr = (struct ieee80211_hdr *)rx->skb->data;
>>       fc = hdr->frame_control;
>> +
>> +     if (ieee80211_is_ctl(fc))
>
> Different question -- why check _is_ctl rather than !_is_data?

Per the Std, management frames can also be fragmented (not that I've
ever seen one).  The standard only excludes control frames.

Javier


-- 
Javier Cardona
cozybit Inc.
http://www.cozybit.com

Re: [PATCH] mac80211: Don't inspect Sequence Control field on control frames

[Johannes Berg]

On Thu, 2012-10-25 at 12:44 -0700, Javier Cardona wrote:

> >> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> >> index f975f64..bf54336 100644
> >> --- a/net/mac80211/rx.c
> >> +++ b/net/mac80211/rx.c
> >> @@ -1467,6 +1467,10 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
> >>
> >>       hdr = (struct ieee80211_hdr *)rx->skb->data;
> >>       fc = hdr->frame_control;
> >> +
> >> +     if (ieee80211_is_ctl(fc))
> >
> > Different question -- why check _is_ctl rather than !_is_data?
> 
> Per the Std, management frames can also be fragmented (not that I've
> ever seen one).  The standard only excludes control frames.

Oh, really? Ok. Applied.

johannes