Fix a couple of more bugs in the L2 padding code:
1. Compute the amount of L2 padding correctly (in 3 places).
2. Trim the skb correctly when the L2 padding has been applied.
Signed-off-by: Gertjan van Wingerde <[email protected]>
---
v2:
- Fix bug detect by Benoit Papillault.
---
drivers/net/wireless/rt2x00/rt2x00queue.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
index b8f0954..9082fad 100644
--- a/drivers/net/wireless/rt2x00/rt2x00queue.c
+++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
@@ -181,7 +181,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
unsigned int frame_length = skb->len;
unsigned int header_align = ALIGN_SIZE(skb, 0);
unsigned int payload_align = ALIGN_SIZE(skb, header_length);
- unsigned int l2pad = 4 - (payload_align - header_align);
+ unsigned int l2pad = (4 - (header_length & 3)) & 3;
if (header_align == payload_align) {
/*
@@ -216,6 +216,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
memmove(skb->data + header_length + l2pad,
skb->data + header_length + l2pad + payload_align,
frame_length - header_length);
+ skb_trim(skb, frame_length + l2pad);
skbdesc->flags |= SKBDESC_L2_PADDED;
}
}
@@ -223,7 +224,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
void rt2x00queue_remove_l2pad(struct sk_buff *skb, unsigned int header_length)
{
struct skb_frame_desc *skbdesc = get_skb_frame_desc(skb);
- unsigned int l2pad = 4 - (header_length & 3);
+ unsigned int l2pad = (4 - (header_length & 3)) & 3;
if (!l2pad || (skbdesc->flags & SKBDESC_L2_PADDED))
return;
@@ -346,7 +347,7 @@ static void rt2x00queue_create_tx_descriptor(struct queue_entry *entry,
* Header and alignment information.
*/
txdesc->header_length = ieee80211_get_hdrlen_from_skb(entry->skb);
- txdesc->l2pad = ALIGN_SIZE(entry->skb, txdesc->header_length);
+ txdesc->l2pad = (4 - (txdesc->header_length & 3)) & 3;
/*
* Check whether this frame is to be acked.
--
1.6.5.3
Gertjan van Wingerde a ?crit :
>> I'll sent a patch for unpadding on the RX path that I have tested with
>> rt2870 usb (I'm still working on the TX path). My feeling is that we are
>> not doing the correct computation.
>
> The current code indeed isn't correct, as it doesn't work for header lengths that don't
> need L2 padding. However see above on the problem with your suggested formula.
Indeed. 802.11 header length is always even (10, 16, 24, 26, 30, 32 ..).
We can keep the exact formula anyway since it's correct.
>
>> l2pad is the number of padding bytes added by the HW. It is computed
>> here based on the header_length. However, doing so, we missed two points :
>>
>> 1. ACK frame is 10 bytes. So header_length will be 10 and then l2pad is
>> 2. However, ACK frames do not need padding, only DATA frames needs
>> padding. The above formula is then wrong.
>
> I gathered as much as well, after a bit of rethinking. However, this is true
> because the frame doesn't have a payload at all. That is the part we have to
> take into account, not the fact that it is an ACK frame.
Correct. I just use ACK frame as an example.
>
>> 2. If we receive a QoS DATA frame whose header_length is 26 according to
>> frame_control, but skb->len = 20, then ieee80211_get_hdrlen_from_skb()
>> will returns hdrlen = 0 based on the value of skb->len. However, this
>> function does not know if skb->len includes said padding or not, or some
>> other padding! (On RX, rt2870 usb frames are also padded at the end!).
>
> But, in that case we have received an invalid frame anyway, so the padding
> doesn't really matter.
I fully disagree here. It's a bit of chicken-egg problem. I'm using
monitor mode to debug other wireless drivers, so I need a tool that
gives me the frame as it appears on the radio medium, be it invalid or
not. And I do see lots of invalid 802.11 frames in real life that are
generated by bogus drivers or intended to be bogus in order to crash
wireless drivers.
>
>> For those reason, I think it's better to have a common function that
>> computes the padding position (based on frame_control). Once we know the
>> padding position, we can compute padding size (= padding position & 3,
>> like above) and we can also compare with the real frame size (contained
>> in RXWI_W0_MPDU_TOTAL_BYTE_COUNT for instance).
>>
>
> See above, I don't think we need to have this detailed knowledge of the frame
> formats in the driver.
Futher testing will tell us. So far, I don't need it either.
Regards,
Benoit
On 11/29/09 18:55, Benoit PAPILLAULT wrote:
> Gertjan van Wingerde a ?crit :
>>> I fully disagree here. It's a bit of chicken-egg problem. I'm using
>>> monitor mode to debug other wireless drivers, so I need a tool that
>>> gives me the frame as it appears on the radio medium, be it invalid or
>>> not. And I do see lots of invalid 802.11 frames in real life that are
>>> generated by bogus drivers or intended to be bogus in order to crash
>>> wireless drivers.
>>
>> So, what do you suggest we do here?
>>
>> If we don't know what kind of data is given (clearly even the ieee80211 header
>> is malformed), then how can we detect what padding has been added by the hardware.
>> We know that the hardware puts padding between the header and the payload, but in
>> this case we don't even have a full header.
>> The only sane thing to do here is to assume that no padding has been applied at all.
>>
>> Also, do we know how mac80211 reacts to these kinds of frames, so is it safe to
>> pass it to mac80211?
>
> Here is my feeling :
>
> - for padding, we need to understand how the hardware behaves. My test
> shows that hardware uses the frame_control field to computes L2PAD flag,
> even if the header is malformed. Padding is always applied after 802.11
> header, if the frame is long enough. Otherwise, no padding of course.
> I've tested on ath9k where ath9k provides FCS field and here FCS can be
> used to check we did proper unpadding. Using ath9k, I then found how
> rt2800 works.
>
OK. This starts to make sense to me now. Basically what we have to do inside rt2x00
is to refrain from removing L2 padding if the header is malformed (to be detected
by checking whether header_length is 0, as ieee80211_get_hdrlen_from_skb will
return that value for a malformed header) and to refrain from removing L2 padding
when the header consumes the whole frame.
I guess this is best done outside the rt2x00queue_remove_l2pad function, so that
that function can still apply to both the TX and RX cases.
> - regarding invalid frames (yes, they do exists), they must be passed to
> upper layers (here mac80211). If mac80211 crashes on invalid frame, then
> mac80211 should be fixed. A quick check in ieee80211_rx() shows that
> ieee80211_rx_monitor() does this work already (ie, invalid frames are
> forwarded to monitor interfaces and then ignored by other interfaces).
>
OK. Good to know.
---
Gertjan.
On 11/29/09 15:32, Benoit PAPILLAULT wrote:
> Ivo Van Doorn a ?crit :
>> On Sun, Nov 29, 2009 at 12:47 PM, Gertjan van Wingerde
>> <[email protected]> wrote:
>>> Fix a couple of more bugs in the L2 padding code:
>>> 1. Compute the amount of L2 padding correctly (in 3 places).
>>> 2. Trim the skb correctly when the L2 padding has been applied.
>>>
>>> Signed-off-by: Gertjan van Wingerde <[email protected]>
>>> ---
>>>
>>> v2:
>>> - Fix bug detect by Benoit Papillault.
>>>
>>> ---
>>> drivers/net/wireless/rt2x00/rt2x00queue.c | 7 ++++---
>>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
>>> index b8f0954..9082fad 100644
>>> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
>>> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
>>> @@ -181,7 +181,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
>>> unsigned int frame_length = skb->len;
>>> unsigned int header_align = ALIGN_SIZE(skb, 0);
>>> unsigned int payload_align = ALIGN_SIZE(skb, header_length);
>>> - unsigned int l2pad = 4 - (payload_align - header_align);
>>> + unsigned int l2pad = (4 - (header_length & 3)) & 3;
>>
>> We already have this calculation on multiple locations, so perhaps it
>> is better to have
>> a L2PAD_SIZE macro in rt2x00.h (where ALIGN_SIZE() is also defined).
>>
>> Ivo
>
> The formula can be simplified to l2pad = header_length & 3.
>
Nope. This formula is not correct. It work okay in most cases as header lengths seem
to be even most of the time (maybe even always), and then it doesn't matter, but if
header lengths become uneven, then the formula computes the number of bytes the header
takes in the last 4-byte block, but not the number of padding bytes.
> I'll sent a patch for unpadding on the RX path that I have tested with
> rt2870 usb (I'm still working on the TX path). My feeling is that we are
> not doing the correct computation.
The current code indeed isn't correct, as it doesn't work for header lengths that don't
need L2 padding. However see above on the problem with your suggested formula.
>
> l2pad is the number of padding bytes added by the HW. It is computed
> here based on the header_length. However, doing so, we missed two points :
>
> 1. ACK frame is 10 bytes. So header_length will be 10 and then l2pad is
> 2. However, ACK frames do not need padding, only DATA frames needs
> padding. The above formula is then wrong.
I gathered as much as well, after a bit of rethinking. However, this is true
because the frame doesn't have a payload at all. That is the part we have to
take into account, not the fact that it is an ACK frame.
>
> 2. If we receive a QoS DATA frame whose header_length is 26 according to
> frame_control, but skb->len = 20, then ieee80211_get_hdrlen_from_skb()
> will returns hdrlen = 0 based on the value of skb->len. However, this
> function does not know if skb->len includes said padding or not, or some
> other padding! (On RX, rt2870 usb frames are also padded at the end!).
But, in that case we have received an invalid frame anyway, so the padding
doesn't really matter.
>
> For those reason, I think it's better to have a common function that
> computes the padding position (based on frame_control). Once we know the
> padding position, we can compute padding size (= padding position & 3,
> like above) and we can also compare with the real frame size (contained
> in RXWI_W0_MPDU_TOTAL_BYTE_COUNT for instance).
>
See above, I don't think we need to have this detailed knowledge of the frame
formats in the driver.
---
Gertjan.
On 11/29/09 18:21, Benoit PAPILLAULT wrote:
> Gertjan van Wingerde a ?crit :
>>> 2. If we receive a QoS DATA frame whose header_length is 26 according to
>>> frame_control, but skb->len = 20, then ieee80211_get_hdrlen_from_skb()
>>> will returns hdrlen = 0 based on the value of skb->len. However, this
>>> function does not know if skb->len includes said padding or not, or some
>>> other padding! (On RX, rt2870 usb frames are also padded at the end!).
>>
>> But, in that case we have received an invalid frame anyway, so the padding
>> doesn't really matter.
>
> I fully disagree here. It's a bit of chicken-egg problem. I'm using
> monitor mode to debug other wireless drivers, so I need a tool that
> gives me the frame as it appears on the radio medium, be it invalid or
> not. And I do see lots of invalid 802.11 frames in real life that are
> generated by bogus drivers or intended to be bogus in order to crash
> wireless drivers.
So, what do you suggest we do here?
If we don't know what kind of data is given (clearly even the ieee80211 header
is malformed), then how can we detect what padding has been added by the hardware.
We know that the hardware puts padding between the header and the payload, but in
this case we don't even have a full header.
The only sane thing to do here is to assume that no padding has been applied at all.
Also, do we know how mac80211 reacts to these kinds of frames, so is it safe to
pass it to mac80211?
---
Gertjan.
Gertjan van Wingerde a ?crit :
>> I fully disagree here. It's a bit of chicken-egg problem. I'm using
>> monitor mode to debug other wireless drivers, so I need a tool that
>> gives me the frame as it appears on the radio medium, be it invalid or
>> not. And I do see lots of invalid 802.11 frames in real life that are
>> generated by bogus drivers or intended to be bogus in order to crash
>> wireless drivers.
>
> So, what do you suggest we do here?
>
> If we don't know what kind of data is given (clearly even the ieee80211 header
> is malformed), then how can we detect what padding has been added by the hardware.
> We know that the hardware puts padding between the header and the payload, but in
> this case we don't even have a full header.
> The only sane thing to do here is to assume that no padding has been applied at all.
>
> Also, do we know how mac80211 reacts to these kinds of frames, so is it safe to
> pass it to mac80211?
Here is my feeling :
- for padding, we need to understand how the hardware behaves. My test
shows that hardware uses the frame_control field to computes L2PAD flag,
even if the header is malformed. Padding is always applied after 802.11
header, if the frame is long enough. Otherwise, no padding of course.
I've tested on ath9k where ath9k provides FCS field and here FCS can be
used to check we did proper unpadding. Using ath9k, I then found how
rt2800 works.
- regarding invalid frames (yes, they do exists), they must be passed to
upper layers (here mac80211). If mac80211 crashes on invalid frame, then
mac80211 should be fixed. A quick check in ieee80211_rx() shows that
ieee80211_rx_monitor() does this work already (ie, invalid frames are
forwarded to monitor interfaces and then ignored by other interfaces).
Regards,
Benoit
On Sun, Nov 29, 2009 at 12:47 PM, Gertjan van Wingerde
<[email protected]> wrote:
> Fix a couple of more bugs in the L2 padding code:
> 1. Compute the amount of L2 padding correctly (in 3 places).
> 2. Trim the skb correctly when the L2 padding has been applied.
>
> Signed-off-by: Gertjan van Wingerde <[email protected]>
> ---
>
> v2:
> - Fix bug detect by Benoit Papillault.
>
> ---
> ?drivers/net/wireless/rt2x00/rt2x00queue.c | ? ?7 ++++---
> ?1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
> index b8f0954..9082fad 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
> @@ -181,7 +181,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
> ? ? ? ?unsigned int frame_length = skb->len;
> ? ? ? ?unsigned int header_align = ALIGN_SIZE(skb, 0);
> ? ? ? ?unsigned int payload_align = ALIGN_SIZE(skb, header_length);
> - ? ? ? unsigned int l2pad = 4 - (payload_align - header_align);
> + ? ? ? unsigned int l2pad = (4 - (header_length & 3)) & 3;
We already have this calculation on multiple locations, so perhaps it
is better to have
a L2PAD_SIZE macro in rt2x00.h (where ALIGN_SIZE() is also defined).
Ivo
Ivo Van Doorn a ?crit :
> On Sun, Nov 29, 2009 at 12:47 PM, Gertjan van Wingerde
> <[email protected]> wrote:
>> Fix a couple of more bugs in the L2 padding code:
>> 1. Compute the amount of L2 padding correctly (in 3 places).
>> 2. Trim the skb correctly when the L2 padding has been applied.
>>
>> Signed-off-by: Gertjan van Wingerde <[email protected]>
>> ---
>>
>> v2:
>> - Fix bug detect by Benoit Papillault.
>>
>> ---
>> drivers/net/wireless/rt2x00/rt2x00queue.c | 7 ++++---
>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
>> index b8f0954..9082fad 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
>> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
>> @@ -181,7 +181,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
>> unsigned int frame_length = skb->len;
>> unsigned int header_align = ALIGN_SIZE(skb, 0);
>> unsigned int payload_align = ALIGN_SIZE(skb, header_length);
>> - unsigned int l2pad = 4 - (payload_align - header_align);
>> + unsigned int l2pad = (4 - (header_length & 3)) & 3;
>
> We already have this calculation on multiple locations, so perhaps it
> is better to have
> a L2PAD_SIZE macro in rt2x00.h (where ALIGN_SIZE() is also defined).
>
> Ivo
The formula can be simplified to l2pad = header_length & 3.
I'll sent a patch for unpadding on the RX path that I have tested with
rt2870 usb (I'm still working on the TX path). My feeling is that we are
not doing the correct computation.
l2pad is the number of padding bytes added by the HW. It is computed
here based on the header_length. However, doing so, we missed two points :
1. ACK frame is 10 bytes. So header_length will be 10 and then l2pad is
2. However, ACK frames do not need padding, only DATA frames needs
padding. The above formula is then wrong.
2. If we receive a QoS DATA frame whose header_length is 26 according to
frame_control, but skb->len = 20, then ieee80211_get_hdrlen_from_skb()
will returns hdrlen = 0 based on the value of skb->len. However, this
function does not know if skb->len includes said padding or not, or some
other padding! (On RX, rt2870 usb frames are also padded at the end!).
For those reason, I think it's better to have a common function that
computes the padding position (based on frame_control). Once we know the
padding position, we can compute padding size (= padding position & 3,
like above) and we can also compare with the real frame size (contained
in RXWI_W0_MPDU_TOTAL_BYTE_COUNT for instance).
Regards,
Benoit