Broken fragmentation

Broken fragmentation

[Tomas Winkler]

Johannes
Your patch bellow has broken the fragmentation. Can you please have a
look at this.

commit 49cfd9a35cd593e3b575b89c2dca148bdd4fbb57
Author: Johannes Berg <johannes@sipsolutions.net>
Date:   Thu May 15 12:55:28 2008 +0200

    mac80211: reorder some transmit handlers



static ieee80211_tx_handler ieee80211_tx_handlers[] =
{
	ieee80211_tx_h_check_assoc,
	ieee80211_tx_h_sequence,
	ieee80211_tx_h_ps_buf,
	ieee80211_tx_h_select_key,
	ieee80211_tx_h_michael_mic_add,
	ieee80211_tx_h_rate_ctrl,
	ieee80211_tx_h_misc,                                         -- this
one already assume fragmetation
	ieee80211_tx_h_fragment,
	/* handlers after fragment must be aware of tx info fragmentation! */
	ieee80211_tx_h_encrypt,
	ieee80211_tx_h_stats,
	NULL
};


static ieee80211_tx_result
ieee80211_tx_h_misc(struct ieee80211_tx_data *tx)
{


          /* Setup duration field for the first fragment of the frame. Duration
	 * for remaining fragments will be updated when they are being sent
	 * to low-level driver in ieee80211_tx(). */
	dur = ieee80211_duration(tx, is_multicast_ether_addr(hdr->addr1),
				 (tx->flags & IEEE80211_TX_FRAGMENTED) ?
				 tx->extra_frag[0]->len : 0);                                 --
NULL pointer


}


Thanks
Tomas

Re: Broken fragmentation

[Johannes Berg]

On Thu, 2008-06-19 at 12:20 +0300, Tomas Winkler wrote:
> Johannes
> Your patch bellow has broken the fragmentation. Can you please have a
> look at this.

>           /* Setup duration field for the first fragment of the frame. Duration
> 	 * for remaining fragments will be updated when they are being sent
> 	 * to low-level driver in ieee80211_tx(). */
> 	dur = ieee80211_duration(tx, is_multicast_ether_addr(hdr->addr1),
> 				 (tx->flags & IEEE80211_TX_FRAGMENTED) ?
> 				 tx->extra_frag[0]->len : 0);                                 --
> NULL pointer

Oops, you're right, I missed the part where it accesses the fragments,
the bulk of the function does txinfo updates which I didn't want to
apply to all fragments.

There is another bug too, this must come after encryption since
encryption can add to the length of the frame. What do you think about
the patch below? Untested so far because I have to go and wanted to get
it out to you.

johannes

---
 net/mac80211/tx.c |   66 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 31 deletions(-)

--- everything.orig/net/mac80211/tx.c	2008-06-19 11:25:12.000000000 +0200
+++ everything/net/mac80211/tx.c	2008-06-19 11:53:08.000000000 +0200
@@ -87,8 +87,8 @@ static inline void ieee80211_dump_frame(
 }
 #endif /* CONFIG_MAC80211_LOWTX_FRAME_DUMP */
 
-static u16 ieee80211_duration(struct ieee80211_tx_data *tx, int group_addr,
-			      int next_frag_len)
+static __le16 ieee80211_duration(struct ieee80211_tx_data *tx, int group_addr,
+				 int next_frag_len)
 {
 	int rate, mrate, erp, dur, i;
 	struct ieee80211_rate *txrate;
@@ -140,7 +140,7 @@ static u16 ieee80211_duration(struct iee
 
 	/* data/mgmt */
 	if (0 /* FIX: data/mgmt during CFP */)
-		return 32768;
+		return cpu_to_le16(32768);
 
 	if (group_addr) /* Group address as the destination - no ACK */
 		return 0;
@@ -210,7 +210,7 @@ static u16 ieee80211_duration(struct iee
 				tx->sdata->bss_conf.use_short_preamble);
 	}
 
-	return dur;
+	return cpu_to_le16(dur);
 }
 
 static int inline is_ieee80211_device(struct net_device *dev,
@@ -544,7 +544,6 @@ ieee80211_tx_h_misc(struct ieee80211_tx_
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) tx->skb->data;
 	u16 fc = le16_to_cpu(hdr->frame_control);
-	u16 dur;
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
 	struct ieee80211_supported_band *sband;
 
@@ -602,14 +601,6 @@ ieee80211_tx_h_misc(struct ieee80211_tx_
 		info->flags |= IEEE80211_TX_CTL_SHORT_PREAMBLE;
 	}
 
-	/* Setup duration field for the first fragment of the frame. Duration
-	 * for remaining fragments will be updated when they are being sent
-	 * to low-level driver in ieee80211_tx(). */
-	dur = ieee80211_duration(tx, is_multicast_ether_addr(hdr->addr1),
-				 (tx->flags & IEEE80211_TX_FRAGMENTED) ?
-				 tx->extra_frag[0]->len : 0);
-	hdr->duration_id = cpu_to_le16(dur);
-
 	if ((info->flags & IEEE80211_TX_CTL_USE_RTS_CTS) ||
 	    (info->flags & IEEE80211_TX_CTL_USE_CTS_PROTECT)) {
 		struct ieee80211_rate *rate;
@@ -755,6 +746,36 @@ ieee80211_tx_h_encrypt(struct ieee80211_
 }
 
 static ieee80211_tx_result
+ieee80211_tx_h_calculate_duration(struct ieee80211_tx_data *tx)
+{
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
+	int next_len, i;
+	int group_addr = is_multicast_ether_addr(hdr->addr1);
+
+	if (!(tx->flags & IEEE80211_TX_FRAGMENTED)) {
+		hdr->duration_id = ieee80211_duration(tx, group_addr, 0);
+		return TX_CONTINUE;
+	}
+
+	hdr->duration_id = ieee80211_duration(tx, group_addr,
+					      tx->extra_frag[0]->len);
+
+	for (i = 0; i < tx->num_extra_frag; i++) {
+		if (i + 1 < tx->num_extra_frag) {
+			next_len = tx->extra_frag[i + 1]->len;
+		} else {
+			next_len = 0;
+			tx->rate_idx = tx->last_frag_rate_idx;
+		}
+
+		hdr = (struct ieee80211_hdr *)tx->extra_frag[i]->data;
+		hdr->duration_id = ieee80211_duration(tx, 0, next_len);
+	}
+
+	return TX_CONTINUE;
+}
+
+static ieee80211_tx_result
 ieee80211_tx_h_stats(struct ieee80211_tx_data *tx)
 {
 	int i;
@@ -788,6 +809,7 @@ static ieee80211_tx_handler ieee80211_tx
 	ieee80211_tx_h_fragment,
 	/* handlers after fragment must be aware of tx info fragmentation! */
 	ieee80211_tx_h_encrypt,
+	ieee80211_tx_h_calculate_duration,
 	ieee80211_tx_h_stats,
 	NULL
 };
@@ -1139,24 +1161,6 @@ static int ieee80211_tx(struct net_devic
 		return 0;
 	}
 
-	if (tx.extra_frag) {
-		for (i = 0; i < tx.num_extra_frag; i++) {
-			int next_len, dur;
-			struct ieee80211_hdr *hdr =
-				(struct ieee80211_hdr *)
-				tx.extra_frag[i]->data;
-
-			if (i + 1 < tx.num_extra_frag) {
-				next_len = tx.extra_frag[i + 1]->len;
-			} else {
-				next_len = 0;
-				tx.rate_idx = tx.last_frag_rate_idx;
-			}
-			dur = ieee80211_duration(&tx, 0, next_len);
-			hdr->duration_id = cpu_to_le16(dur);
-		}
-	}
-
 retry:
 	ret = __ieee80211_tx(local, skb, &tx);
 	if (ret) {

Re: Broken fragmentation

[Tomas Winkler]

On Thu, Jun 19, 2008 at 12:56 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2008-06-19 at 12:20 +0300, Tomas Winkler wrote:
>> Johannes
>> Your patch bellow has broken the fragmentation. Can you please have a
>> look at this.
>
>>           /* Setup duration field for the first fragment of the frame. Duration
>>        * for remaining fragments will be updated when they are being sent
>>        * to low-level driver in ieee80211_tx(). */
>>       dur = ieee80211_duration(tx, is_multicast_ether_addr(hdr->addr1),
>>                                (tx->flags & IEEE80211_TX_FRAGMENTED) ?
>>                                tx->extra_frag[0]->len : 0);                                 --
>> NULL pointer
>
> Oops, you're right, I missed the part where it accesses the fragments,
> the bulk of the function does txinfo updates which I didn't want to
> apply to all fragments.
>
> There is another bug too, this must come after encryption since
> encryption can add to the length of the frame. What do you think about
> the patch below? Untested so far because I have to go and wanted to get
> it out to you.
>

Now it crashes in the driver...maybe the txinfo to cb changes. Looking into it.
Tomas

> johannes
>
> ---
>  net/mac80211/tx.c |   66 ++++++++++++++++++++++++++++--------------------------
>  1 file changed, 35 insertions(+), 31 deletions(-)
>
> --- everything.orig/net/mac80211/tx.c   2008-06-19 11:25:12.000000000 +0200
> +++ everything/net/mac80211/tx.c        2008-06-19 11:53:08.000000000 +0200
> @@ -87,8 +87,8 @@ static inline void ieee80211_dump_frame(
>  }
>  #endif /* CONFIG_MAC80211_LOWTX_FRAME_DUMP */
>
> -static u16 ieee80211_duration(struct ieee80211_tx_data *tx, int group_addr,
> -                             int next_frag_len)
> +static __le16 ieee80211_duration(struct ieee80211_tx_data *tx, int group_addr,
> +                                int next_frag_len)
>  {
>        int rate, mrate, erp, dur, i;
>        struct ieee80211_rate *txrate;
> @@ -140,7 +140,7 @@ static u16 ieee80211_duration(struct iee
>
>        /* data/mgmt */
>        if (0 /* FIX: data/mgmt during CFP */)
> -               return 32768;
> +               return cpu_to_le16(32768);
>
>        if (group_addr) /* Group address as the destination - no ACK */
>                return 0;
> @@ -210,7 +210,7 @@ static u16 ieee80211_duration(struct iee
>                                tx->sdata->bss_conf.use_short_preamble);
>        }
>
> -       return dur;
> +       return cpu_to_le16(dur);
>  }
>
>  static int inline is_ieee80211_device(struct net_device *dev,
> @@ -544,7 +544,6 @@ ieee80211_tx_h_misc(struct ieee80211_tx_
>  {
>        struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) tx->skb->data;
>        u16 fc = le16_to_cpu(hdr->frame_control);
> -       u16 dur;
>        struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
>        struct ieee80211_supported_band *sband;
>
> @@ -602,14 +601,6 @@ ieee80211_tx_h_misc(struct ieee80211_tx_
>                info->flags |= IEEE80211_TX_CTL_SHORT_PREAMBLE;
>        }
>
> -       /* Setup duration field for the first fragment of the frame. Duration
> -        * for remaining fragments will be updated when they are being sent
> -        * to low-level driver in ieee80211_tx(). */
> -       dur = ieee80211_duration(tx, is_multicast_ether_addr(hdr->addr1),
> -                                (tx->flags & IEEE80211_TX_FRAGMENTED) ?
> -                                tx->extra_frag[0]->len : 0);
> -       hdr->duration_id = cpu_to_le16(dur);
> -
>        if ((info->flags & IEEE80211_TX_CTL_USE_RTS_CTS) ||
>            (info->flags & IEEE80211_TX_CTL_USE_CTS_PROTECT)) {
>                struct ieee80211_rate *rate;
> @@ -755,6 +746,36 @@ ieee80211_tx_h_encrypt(struct ieee80211_
>  }
>
>  static ieee80211_tx_result
> +ieee80211_tx_h_calculate_duration(struct ieee80211_tx_data *tx)
> +{
> +       struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
> +       int next_len, i;
> +       int group_addr = is_multicast_ether_addr(hdr->addr1);
> +
> +       if (!(tx->flags & IEEE80211_TX_FRAGMENTED)) {
> +               hdr->duration_id = ieee80211_duration(tx, group_addr, 0);
> +               return TX_CONTINUE;
> +       }
> +
> +       hdr->duration_id = ieee80211_duration(tx, group_addr,
> +                                             tx->extra_frag[0]->len);
> +
> +       for (i = 0; i < tx->num_extra_frag; i++) {
> +               if (i + 1 < tx->num_extra_frag) {
> +                       next_len = tx->extra_frag[i + 1]->len;
> +               } else {
> +                       next_len = 0;
> +                       tx->rate_idx = tx->last_frag_rate_idx;
> +               }
> +
> +               hdr = (struct ieee80211_hdr *)tx->extra_frag[i]->data;
> +               hdr->duration_id = ieee80211_duration(tx, 0, next_len);
> +       }
> +
> +       return TX_CONTINUE;
> +}
> +
> +static ieee80211_tx_result
>  ieee80211_tx_h_stats(struct ieee80211_tx_data *tx)
>  {
>        int i;
> @@ -788,6 +809,7 @@ static ieee80211_tx_handler ieee80211_tx
>        ieee80211_tx_h_fragment,
>        /* handlers after fragment must be aware of tx info fragmentation! */
>        ieee80211_tx_h_encrypt,
> +       ieee80211_tx_h_calculate_duration,
>        ieee80211_tx_h_stats,
>        NULL
>  };
> @@ -1139,24 +1161,6 @@ static int ieee80211_tx(struct net_devic
>                return 0;
>        }
>
> -       if (tx.extra_frag) {
> -               for (i = 0; i < tx.num_extra_frag; i++) {
> -                       int next_len, dur;
> -                       struct ieee80211_hdr *hdr =
> -                               (struct ieee80211_hdr *)
> -                               tx.extra_frag[i]->data;
> -
> -                       if (i + 1 < tx.num_extra_frag) {
> -                               next_len = tx.extra_frag[i + 1]->len;
> -                       } else {
> -                               next_len = 0;
> -                               tx.rate_idx = tx.last_frag_rate_idx;
> -                       }
> -                       dur = ieee80211_duration(&tx, 0, next_len);
> -                       hdr->duration_id = cpu_to_le16(dur);
> -               }
> -       }
> -
>  retry:
>        ret = __ieee80211_tx(local, skb, &tx);
>        if (ret) {
>
>
>

Re: Broken fragmentation

[Tomas Winkler]

On Thu, Jun 19, 2008 at 2:46 PM, Tomas Winkler <tomasw@gmail.com> wrote:
> On Thu, Jun 19, 2008 at 12:56 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> On Thu, 2008-06-19 at 12:20 +0300, Tomas Winkler wrote:
>>> Johannes
>>> Your patch bellow has broken the fragmentation. Can you please have a
>>> look at this.
>>
>>>           /* Setup duration field for the first fragment of the frame. Duration
>>>        * for remaining fragments will be updated when they are being sent
>>>        * to low-level driver in ieee80211_tx(). */
>>>       dur = ieee80211_duration(tx, is_multicast_ether_addr(hdr->addr1),
>>>                                (tx->flags & IEEE80211_TX_FRAGMENTED) ?
>>>                                tx->extra_frag[0]->len : 0);                                 --
>>> NULL pointer
>>
>> Oops, you're right, I missed the part where it accesses the fragments,
>> the bulk of the function does txinfo updates which I didn't want to
>> apply to all fragments.
>>
>> There is another bug too, this must come after encryption since
>> encryption can add to the length of the frame. What do you think about
>> the patch below? Untested so far because I have to go and wanted to get
>> it out to you.
>>
>
> Now it crashes in the driver...maybe the txinfo to cb changes. Looking into it.
> Tomas
Looks like this is the second part keyconf is null
Tomas

>>
>> ---
>>  net/mac80211/tx.c |   66 ++++++++++++++++++++++++++++--------------------------
>>  1 file changed, 35 insertions(+), 31 deletions(-)
>>
>> --- everything.orig/net/mac80211/tx.c   2008-06-19 11:25:12.000000000 +0200
>> +++ everything/net/mac80211/tx.c        2008-06-19 11:53:08.000000000 +0200
>> @@ -87,8 +87,8 @@ static inline void ieee80211_dump_frame(
>>  }
>>  #endif /* CONFIG_MAC80211_LOWTX_FRAME_DUMP */
>>
>> -static u16 ieee80211_duration(struct ieee80211_tx_data *tx, int group_addr,
>> -                             int next_frag_len)
>> +static __le16 ieee80211_duration(struct ieee80211_tx_data *tx, int group_addr,
>> +                                int next_frag_len)
>>  {
>>        int rate, mrate, erp, dur, i;
>>        struct ieee80211_rate *txrate;
>> @@ -140,7 +140,7 @@ static u16 ieee80211_duration(struct iee
>>
>>        /* data/mgmt */
>>        if (0 /* FIX: data/mgmt during CFP */)
>> -               return 32768;
>> +               return cpu_to_le16(32768);
>>
>>        if (group_addr) /* Group address as the destination - no ACK */
>>                return 0;
>> @@ -210,7 +210,7 @@ static u16 ieee80211_duration(struct iee
>>                                tx->sdata->bss_conf.use_short_preamble);
>>        }
>>
>> -       return dur;
>> +       return cpu_to_le16(dur);
>>  }
>>
>>  static int inline is_ieee80211_device(struct net_device *dev,
>> @@ -544,7 +544,6 @@ ieee80211_tx_h_misc(struct ieee80211_tx_
>>  {
>>        struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) tx->skb->data;
>>        u16 fc = le16_to_cpu(hdr->frame_control);
>> -       u16 dur;
>>        struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
>>        struct ieee80211_supported_band *sband;
>>
>> @@ -602,14 +601,6 @@ ieee80211_tx_h_misc(struct ieee80211_tx_
>>                info->flags |= IEEE80211_TX_CTL_SHORT_PREAMBLE;
>>        }
>>
>> -       /* Setup duration field for the first fragment of the frame. Duration
>> -        * for remaining fragments will be updated when they are being sent
>> -        * to low-level driver in ieee80211_tx(). */
>> -       dur = ieee80211_duration(tx, is_multicast_ether_addr(hdr->addr1),
>> -                                (tx->flags & IEEE80211_TX_FRAGMENTED) ?
>> -                                tx->extra_frag[0]->len : 0);
>> -       hdr->duration_id = cpu_to_le16(dur);
>> -
>>        if ((info->flags & IEEE80211_TX_CTL_USE_RTS_CTS) ||
>>            (info->flags & IEEE80211_TX_CTL_USE_CTS_PROTECT)) {
>>                struct ieee80211_rate *rate;
>> @@ -755,6 +746,36 @@ ieee80211_tx_h_encrypt(struct ieee80211_
>>  }
>>
>>  static ieee80211_tx_result
>> +ieee80211_tx_h_calculate_duration(struct ieee80211_tx_data *tx)
>> +{
>> +       struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
>> +       int next_len, i;
>> +       int group_addr = is_multicast_ether_addr(hdr->addr1);
>> +
>> +       if (!(tx->flags & IEEE80211_TX_FRAGMENTED)) {
>> +               hdr->duration_id = ieee80211_duration(tx, group_addr, 0);
>> +               return TX_CONTINUE;
>> +       }
>> +
>> +       hdr->duration_id = ieee80211_duration(tx, group_addr,
>> +                                             tx->extra_frag[0]->len);
>> +
>> +       for (i = 0; i < tx->num_extra_frag; i++) {
>> +               if (i + 1 < tx->num_extra_frag) {
>> +                       next_len = tx->extra_frag[i + 1]->len;
>> +               } else {
>> +                       next_len = 0;
>> +                       tx->rate_idx = tx->last_frag_rate_idx;
>> +               }
>> +
>> +               hdr = (struct ieee80211_hdr *)tx->extra_frag[i]->data;
>> +               hdr->duration_id = ieee80211_duration(tx, 0, next_len);
>> +       }
>> +
>> +       return TX_CONTINUE;
>> +}
>> +
>> +static ieee80211_tx_result
>>  ieee80211_tx_h_stats(struct ieee80211_tx_data *tx)
>>  {
>>        int i;
>> @@ -788,6 +809,7 @@ static ieee80211_tx_handler ieee80211_tx
>>        ieee80211_tx_h_fragment,
>>        /* handlers after fragment must be aware of tx info fragmentation! */
>>        ieee80211_tx_h_encrypt,
>> +       ieee80211_tx_h_calculate_duration,
>>        ieee80211_tx_h_stats,
>>        NULL
>>  };
>> @@ -1139,24 +1161,6 @@ static int ieee80211_tx(struct net_devic
>>                return 0;
>>        }
>>
>> -       if (tx.extra_frag) {
>> -               for (i = 0; i < tx.num_extra_frag; i++) {
>> -                       int next_len, dur;
>> -                       struct ieee80211_hdr *hdr =
>> -                               (struct ieee80211_hdr *)
>> -                               tx.extra_frag[i]->data;
>> -
>> -                       if (i + 1 < tx.num_extra_frag) {
>> -                               next_len = tx.extra_frag[i + 1]->len;
>> -                       } else {
>> -                               next_len = 0;
>> -                               tx.rate_idx = tx.last_frag_rate_idx;
>> -                       }
>> -                       dur = ieee80211_duration(&tx, 0, next_len);
>> -                       hdr->duration_id = cpu_to_le16(dur);
>> -               }
>> -       }
>> -
>>  retry:
>>        ret = __ieee80211_tx(local, skb, &tx);
>>        if (ret) {
>>
>>
>>
>

Re: Broken fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]


> > Now it crashes in the driver...maybe the txinfo to cb changes. Looking into it.
> > Tomas
> Looks like this is the second part keyconf is null

Part? You mean fragment? What encryption algorithm? I've quickly looked
and all of them seem to do the control.hw_key assignment in their
respective _skb handler so it should be done for all fragments.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Broken fragmentation

[Tomas Winkler]

On Thu, Jun 19, 2008 at 3:23 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> > Now it crashes in the driver...maybe the txinfo to cb changes. Looking into it.
>> > Tomas
>> Looks like this is the second part keyconf is null
>
> Part? You mean fragment? What encryption algorithm? I've quickly looked
> and all of them seem to do the control.hw_key assignment in their
> respective _skb handler so it should be done for all fragments.

The problem is w/o crypto
I think this is not set on each fragment
info->flags |= IEEE80211_TX_CTL_DO_NOT_ENCRYPT;

Tomas

> johannes
>

Re: Broken fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

On Thu, 2008-06-19 at 16:08 +0300, Tomas Winkler wrote:
> On Thu, Jun 19, 2008 at 3:23 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> >
> >> > Now it crashes in the driver...maybe the txinfo to cb changes. Looking into it.
> >> > Tomas
> >> Looks like this is the second part keyconf is null
> >
> > Part? You mean fragment? What encryption algorithm? I've quickly looked
> > and all of them seem to do the control.hw_key assignment in their
> > respective _skb handler so it should be done for all fragments.
> 
> The problem is w/o crypto
> I think this is not set on each fragment
> info->flags |= IEEE80211_TX_CTL_DO_NOT_ENCRYPT;

Ouch, you're right, the whole tx info isn't properly initialised for
fragments.

Can you try putting

memcpy(frag->cb, first->cb, sizeof(first->cb));

at around line 690 of tx.c where this code is?

                frag->priority = first->priority;
                skb_reserve(frag, tx->local->tx_headroom +
                                  IEEE80211_ENCRYPT_HEADROOM);


[now I know what the cause is. I had another patch that used skb_segment
to do the fragmentation, but that was dropped and skb_segment does copy
skb->cb while our code doesn't]

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Broken fragmentation

[Tomas Winkler]

On Thu, Jun 19, 2008 at 4:24 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2008-06-19 at 16:08 +0300, Tomas Winkler wrote:
>> On Thu, Jun 19, 2008 at 3:23 PM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> >
>> >> > Now it crashes in the driver...maybe the txinfo to cb changes. Looking into it.
>> >> > Tomas
>> >> Looks like this is the second part keyconf is null
>> >
>> > Part? You mean fragment? What encryption algorithm? I've quickly looked
>> > and all of them seem to do the control.hw_key assignment in their
>> > respective _skb handler so it should be done for all fragments.
>>
>> The problem is w/o crypto
>> I think this is not set on each fragment
>> info->flags |= IEEE80211_TX_CTL_DO_NOT_ENCRYPT;
>
> Ouch, you're right, the whole tx info isn't properly initialised for
> fragments.
>
> Can you try putting
>
> memcpy(frag->cb, first->cb, sizeof(first->cb));
>
> at around line 690 of tx.c where this code is?
>
>                frag->priority = first->priority;
>                skb_reserve(frag, tx->local->tx_headroom +
>                                  IEEE80211_ENCRYPT_HEADROOM);
>
>
> [now I know what the cause is. I had another patch that used skb_segment
> to do the fragmentation, but that was dropped and skb_segment does copy
> skb->cb while our code doesn't]

No crash but still not at home. No fragmented traffic on the air...grrr.
Tomas

Re: Broken fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]

On Thu, 2008-06-19 at 17:45 +0300, Tomas Winkler wrote:
> On Thu, Jun 19, 2008 at 4:24 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Thu, 2008-06-19 at 16:08 +0300, Tomas Winkler wrote:
> >> On Thu, Jun 19, 2008 at 3:23 PM, Johannes Berg
> >> <johannes@sipsolutions.net> wrote:
> >> >
> >> >> > Now it crashes in the driver...maybe the txinfo to cb changes. Looking into it.
> >> >> > Tomas
> >> >> Looks like this is the second part keyconf is null
> >> >
> >> > Part? You mean fragment? What encryption algorithm? I've quickly looked
> >> > and all of them seem to do the control.hw_key assignment in their
> >> > respective _skb handler so it should be done for all fragments.
> >>
> >> The problem is w/o crypto
> >> I think this is not set on each fragment
> >> info->flags |= IEEE80211_TX_CTL_DO_NOT_ENCRYPT;
> >
> > Ouch, you're right, the whole tx info isn't properly initialised for
> > fragments.
> >
> > Can you try putting
> >
> > memcpy(frag->cb, first->cb, sizeof(first->cb));
> >
> > at around line 690 of tx.c where this code is?
> >
> >                frag->priority = first->priority;
> >                skb_reserve(frag, tx->local->tx_headroom +
> >                                  IEEE80211_ENCRYPT_HEADROOM);
> >
> >
> > [now I know what the cause is. I had another patch that used skb_segment
> > to do the fragmentation, but that was dropped and skb_segment does copy
> > skb->cb while our code doesn't]
> 
> No crash but still not at home. No fragmented traffic on the air...grrr.

Hmm, ok. I'll give it a try later.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Broken fragmentation

[Tomas Winkler]

On Thu, Jun 19, 2008 at 6:26 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2008-06-19 at 17:45 +0300, Tomas Winkler wrote:
>> On Thu, Jun 19, 2008 at 4:24 PM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Thu, 2008-06-19 at 16:08 +0300, Tomas Winkler wrote:
>> >> On Thu, Jun 19, 2008 at 3:23 PM, Johannes Berg
>> >> <johannes@sipsolutions.net> wrote:
>> >> >
>> >> >> > Now it crashes in the driver...maybe the txinfo to cb changes. Looking into it.
>> >> >> > Tomas
>> >> >> Looks like this is the second part keyconf is null
>> >> >
>> >> > Part? You mean fragment? What encryption algorithm? I've quickly looked
>> >> > and all of them seem to do the control.hw_key assignment in their
>> >> > respective _skb handler so it should be done for all fragments.
>> >>
>> >> The problem is w/o crypto
>> >> I think this is not set on each fragment
>> >> info->flags |= IEEE80211_TX_CTL_DO_NOT_ENCRYPT;
>> >
>> > Ouch, you're right, the whole tx info isn't properly initialised for
>> > fragments.
>> >
>> > Can you try putting
>> >
>> > memcpy(frag->cb, first->cb, sizeof(first->cb));
>> >
>> > at around line 690 of tx.c where this code is?
>> >
>> >                frag->priority = first->priority;
>> >                skb_reserve(frag, tx->local->tx_headroom +
>> >                                  IEEE80211_ENCRYPT_HEADROOM);
>> >
>> >
>> > [now I know what the cause is. I had another patch that used skb_segment
>> > to do the fragmentation, but that was dropped and skb_segment does copy
>> > skb->cb while our code doesn't]
>>
>> No crash but still not at home. No fragmented traffic on the air...grrr.
>
> Hmm, ok. I'll give it a try later.

Fragments are in the air but the peer is not able to reassemble them.
Something is wrong with the first fragment but I'm not sure yet.
Tomas

Re: Broken fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]

On Thu, 2008-06-19 at 21:13 +0300, Tomas Winkler wrote:
> On Thu, Jun 19, 2008 at 6:26 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Thu, 2008-06-19 at 17:45 +0300, Tomas Winkler wrote:
> >> On Thu, Jun 19, 2008 at 4:24 PM, Johannes Berg
> >> <johannes@sipsolutions.net> wrote:
> >> > On Thu, 2008-06-19 at 16:08 +0300, Tomas Winkler wrote:
> >> >> On Thu, Jun 19, 2008 at 3:23 PM, Johannes Berg
> >> >> <johannes@sipsolutions.net> wrote:
> >> >> >
> >> >> >> > Now it crashes in the driver...maybe the txinfo to cb changes. Looking into it.
> >> >> >> > Tomas
> >> >> >> Looks like this is the second part keyconf is null
> >> >> >
> >> >> > Part? You mean fragment? What encryption algorithm? I've quickly looked
> >> >> > and all of them seem to do the control.hw_key assignment in their
> >> >> > respective _skb handler so it should be done for all fragments.
> >> >>
> >> >> The problem is w/o crypto
> >> >> I think this is not set on each fragment
> >> >> info->flags |= IEEE80211_TX_CTL_DO_NOT_ENCRYPT;
> >> >
> >> > Ouch, you're right, the whole tx info isn't properly initialised for
> >> > fragments.
> >> >
> >> > Can you try putting
> >> >
> >> > memcpy(frag->cb, first->cb, sizeof(first->cb));
> >> >
> >> > at around line 690 of tx.c where this code is?
> >> >
> >> >                frag->priority = first->priority;
> >> >                skb_reserve(frag, tx->local->tx_headroom +
> >> >                                  IEEE80211_ENCRYPT_HEADROOM);
> >> >
> >> >
> >> > [now I know what the cause is. I had another patch that used skb_segment
> >> > to do the fragmentation, but that was dropped and skb_segment does copy
> >> > skb->cb while our code doesn't]
> >>
> >> No crash but still not at home. No fragmented traffic on the air...grrr.
> >
> > Hmm, ok. I'll give it a try later.
> 
> Fragments are in the air but the peer is not able to reassemble them.
> Something is wrong with the first fragment but I'm not sure yet.

Hm, ok. got a packet capture? Maybe that was broken before, I remember
having problems with fragmentation but putting it down to the
known-to-be-completely-broken AP I was using.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Broken fragmentation

[Tomas Winkler]

On Thu, Jun 19, 2008 at 9:16 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2008-06-19 at 21:13 +0300, Tomas Winkler wrote:
>> On Thu, Jun 19, 2008 at 6:26 PM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Thu, 2008-06-19 at 17:45 +0300, Tomas Winkler wrote:
>> >> On Thu, Jun 19, 2008 at 4:24 PM, Johannes Berg
>> >> <johannes@sipsolutions.net> wrote:
>> >> > On Thu, 2008-06-19 at 16:08 +0300, Tomas Winkler wrote:
>> >> >> On Thu, Jun 19, 2008 at 3:23 PM, Johannes Berg
>> >> >> <johannes@sipsolutions.net> wrote:
>> >> >> >
>> >> >> >> > Now it crashes in the driver...maybe the txinfo to cb changes. Looking into it.
>> >> >> >> > Tomas
>> >> >> >> Looks like this is the second part keyconf is null
>> >> >> >
>> >> >> > Part? You mean fragment? What encryption algorithm? I've quickly looked
>> >> >> > and all of them seem to do the control.hw_key assignment in their
>> >> >> > respective _skb handler so it should be done for all fragments.
>> >> >>
>> >> >> The problem is w/o crypto
>> >> >> I think this is not set on each fragment
>> >> >> info->flags |= IEEE80211_TX_CTL_DO_NOT_ENCRYPT;
>> >> >
>> >> > Ouch, you're right, the whole tx info isn't properly initialised for
>> >> > fragments.
>> >> >
>> >> > Can you try putting
>> >> >
>> >> > memcpy(frag->cb, first->cb, sizeof(first->cb));
>> >> >
>> >> > at around line 690 of tx.c where this code is?
>> >> >
>> >> >                frag->priority = first->priority;
>> >> >                skb_reserve(frag, tx->local->tx_headroom +
>> >> >                                  IEEE80211_ENCRYPT_HEADROOM);
>> >> >
>> >> >
>> >> > [now I know what the cause is. I had another patch that used skb_segment
>> >> > to do the fragmentation, but that was dropped and skb_segment does copy
>> >> > skb->cb while our code doesn't]
>> >>
>> >> No crash but still not at home. No fragmented traffic on the air...grrr.
>> >
>> > Hmm, ok. I'll give it a try later.
>>
>> Fragments are in the air but the peer is not able to reassemble them.
>> Something is wrong with the first fragment but I'm not sure yet.
>
> Hm, ok. got a packet capture? Maybe that was broken before, I remember
> having problems with fragmentation but putting it down to the
> known-to-be-completely-broken AP I was using.

It is working the latest problem was caused by the driver, Yet i still
don't understand the regression
in the driver.  Would be nice if you can confirm it works also with another HW.
Sending your patch out.
Thanks
Tomas

Re: Broken fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]


> >> Fragments are in the air but the peer is not able to reassemble them.
> >> Something is wrong with the first fragment but I'm not sure yet.
> >
> > Hm, ok. got a packet capture? Maybe that was broken before, I remember
> > having problems with fragmentation but putting it down to the
> > known-to-be-completely-broken AP I was using.
> 
> It is working the latest problem was caused by the driver, Yet i still
> don't understand the regression
> in the driver.  

Hmm, ok.

> Would be nice if you can confirm it works also with another HW.
> Sending your patch out.

Thanks, I'll give it a try tomorrow with a better AP.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Broken fragmentation

[Tomas Winkler]

On Fri, Jun 20, 2008 at 12:25 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> >> Fragments are in the air but the peer is not able to reassemble them.
>> >> Something is wrong with the first fragment but I'm not sure yet.
>> >
>> > Hm, ok. got a packet capture? Maybe that was broken before, I remember
>> > having problems with fragmentation but putting it down to the
>> > known-to-be-completely-broken AP I was using.
>>
>> It is working the latest problem was caused by the driver, Yet i still
>> don't understand the regression
>> in the driver.
>
> Hmm, ok.
>
So the bug is still in mac80211. Fragments are scheduled to different
TIDs and hence to different queues.
Investigating further. If you have something in mind let me know

Thanks
Tomas

Re: Broken fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 986 bytes --]

On Wed, 2008-06-25 at 12:50 +0300, Tomas Winkler wrote:
> On Fri, Jun 20, 2008 at 12:25 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> >
> >> >> Fragments are in the air but the peer is not able to reassemble them.
> >> >> Something is wrong with the first fragment but I'm not sure yet.
> >> >
> >> > Hm, ok. got a packet capture? Maybe that was broken before, I remember
> >> > having problems with fragmentation but putting it down to the
> >> > known-to-be-completely-broken AP I was using.
> >>
> >> It is working the latest problem was caused by the driver, Yet i still
> >> don't understand the regression
> >> in the driver.
> >
> > Hmm, ok.
> >
> So the bug is still in mac80211. Fragments are scheduled to different
> TIDs and hence to different queues.
> Investigating further. If you have something in mind let me know

Hmm, yes, I do indeed, try

skb_copy_queue_mapping(frag, first);

where you previously added the cb copy.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Broken fragmentation

[Tomas Winkler]

On Wed, Jun 25, 2008 at 1:02 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2008-06-25 at 12:50 +0300, Tomas Winkler wrote:
>> On Fri, Jun 20, 2008 at 12:25 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> >
>> >> >> Fragments are in the air but the peer is not able to reassemble them.
>> >> >> Something is wrong with the first fragment but I'm not sure yet.
>> >> >
>> >> > Hm, ok. got a packet capture? Maybe that was broken before, I remember
>> >> > having problems with fragmentation but putting it down to the
>> >> > known-to-be-completely-broken AP I was using.
>> >>
>> >> It is working the latest problem was caused by the driver, Yet i still
>> >> don't understand the regression
>> >> in the driver.
>> >
>> > Hmm, ok.
>> >
>> So the bug is still in mac80211. Fragments are scheduled to different
>> TIDs and hence to different queues.
>> Investigating further. If you have something in mind let me know
>
> Hmm, yes, I do indeed, try
>
> skb_copy_queue_mapping(frag, first);
>
> where you previously added the cb copy.
Yeah just found that too 2 min ago :)
Thanks
Tomas

Re: Broken fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

On Wed, 2008-06-25 at 13:30 +0300, Tomas Winkler wrote:

> >> So the bug is still in mac80211. Fragments are scheduled to different
> >> TIDs and hence to different queues.
> >> Investigating further. If you have something in mind let me know
> >
> > Hmm, yes, I do indeed, try
> >
> > skb_copy_queue_mapping(frag, first);
> >
> > where you previously added the cb copy.

> Yeah just found that too 2 min ago :)

:)

Before it happens again, can you think of anything else we need to copy?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Broken fragmentation

[Tomas Winkler]

On Wed, Jun 25, 2008 at 1:32 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2008-06-25 at 13:30 +0300, Tomas Winkler wrote:
>
>> >> So the bug is still in mac80211. Fragments are scheduled to different
>> >> TIDs and hence to different queues.
>> >> Investigating further. If you have something in mind let me know
>> >
>> > Hmm, yes, I do indeed, try
>> >
>> > skb_copy_queue_mapping(frag, first);
>> >
>> > where you previously added the cb copy.
>
>> Yeah just found that too 2 min ago :)
>
> :)
>
> Before it happens again, can you think of anything else we need to copy?

Not sure, now it works.
Shell I send a patch or blend it into the original one.
Thanks

> johannes
>

Re: Broken fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]

On Wed, 2008-06-25 at 13:35 +0300, Tomas Winkler wrote:
> On Wed, Jun 25, 2008 at 1:32 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Wed, 2008-06-25 at 13:30 +0300, Tomas Winkler wrote:
> >
> >> >> So the bug is still in mac80211. Fragments are scheduled to different
> >> >> TIDs and hence to different queues.
> >> >> Investigating further. If you have something in mind let me know
> >> >
> >> > Hmm, yes, I do indeed, try
> >> >
> >> > skb_copy_queue_mapping(frag, first);
> >> >
> >> > where you previously added the cb copy.
> >
> >> Yeah just found that too 2 min ago :)
> >
> > :)
> >
> > Before it happens again, can you think of anything else we need to copy?
> 
> Not sure, now it works.
> Shell I send a patch or blend it into the original one.

I don't care, ask John? Or just merge them, he doesn't seem to have
merged it yet.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Broken fragmentation

[Tomas Winkler]

On Wed, Jun 25, 2008 at 1:42 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2008-06-25 at 13:35 +0300, Tomas Winkler wrote:
>> On Wed, Jun 25, 2008 at 1:32 PM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Wed, 2008-06-25 at 13:30 +0300, Tomas Winkler wrote:
>> >
>> >> >> So the bug is still in mac80211. Fragments are scheduled to different
>> >> >> TIDs and hence to different queues.
>> >> >> Investigating further. If you have something in mind let me know
>> >> >
>> >> > Hmm, yes, I do indeed, try
>> >> >
>> >> > skb_copy_queue_mapping(frag, first);
>> >> >
>> >> > where you previously added the cb copy.
>> >
>> >> Yeah just found that too 2 min ago :)
>> >
>> > :)
>> >
>> > Before it happens again, can you think of anything else we need to copy?
>>
>> Not sure, now it works.
>> Shell I send a patch or blend it into the original one.
>
> I don't care, ask John? Or just merge them, he doesn't seem to have
> merged it yet.
If he didn't I will merge them.
Thanks
Tomas