Re: [PATCH] 6lowpan: Fix extraction of flow label field

[Alexander Aring <alex.aring@gmail.com>]

On Tue, Jun 30, 2015 at 03:24:52AM -0700, Lukasz Duda wrote:
> The lowpan_fetch_skb function is used to fetch the first byte,
> which also increments the data pointer in skb structure,
> making subsequent array lookup of byte 0 actually being byte 1.
> 
> To decompress the first byte of the Flow Label when the TF flag is
> set to 0x01, the second half of the first byte is needed.
> 
> The patch fixes the extraction of the Flow Label field.
> 
> Signed-off-by: Lukasz Duda <lukasz.duda@nordicsemi.no>
> Signed-off-by: Glenn Ruben Bakke <glenn.ruben.bakke@nordicsemi.no>

Acked-by: Alexander Aring <alex.aring@gmail.com>

> ---
>  net/6lowpan/iphc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 9055d7b..74e56d7 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -284,7 +284,7 @@ lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
>  		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
>  			return -EINVAL;
>  
> -		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
> +		hdr.flow_lbl[0] = (tmp & 0x0F) | ((tmp >> 2) & 0x30);
>  		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
>  		skb_pull(skb, 2);
>  		break;

This code part is really hard to decrypt/understand. Nevertheless for
some historical(contiki) reasons we have this situation mainline now
and I had no time to cleanup this code. Actual it remembers me on the
early state of the iphc code.

I would recommended to cleanup the whole traffic flow decompress/compress
part (Maybe also with some magic numbers defines which is used on both sides
and some static inline functions). Really don't like the actually stuff there.
Anyway thank you for dig into this issue now.

One reason why definitly something goes wrong there is that skb->data[0]
information is used for hdr.flow_lbl[1] and hdr.flow_lbl[0] which can't
be. I didn't test it yet and trust you that this will do the right
behaviour for now. I also have no time that I can write really a testcase
for that. What I can say currently is that something is wrong here
and your good looks fine for me and makes more sense than the current behaviour.

Thanks.

- Alex