| Message ID | 1351188618-11155-1-git-send-email-javier@cozybit.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;