2017-02-09 14:55:46

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH 0/5] Bluetooth: 6LoWPAN: Fix lladdr length

From: Luiz Augusto von Dentz <[email protected]>

These patches fixes lladdr length to be 6 bytes long and not 8 which cause
neighbor advertisement to be sent with wrong lladdr including FF:FE filler
bytes for eui64.

Note: This does not fix some of the existing crashes which I hope to address
in a different set.

Alexander Aring (2):
6lowpan: iphc: override l2 packet information
ipv6: addrconf: fix 48 bit 6lowpan autoconfiguration

Luiz Augusto von Dentz (1):
6lowpan: Use netdev addr_len to determine lladdr len

Patrik Flykt (2):
bluetooth: Set 6 byte device addresses
6lowpan: Set MAC address lenght according to LOWPAN_LLTYPE

net/6lowpan/core.c | 11 ++++++++++-
net/6lowpan/iphc.c | 25 ++++++++++++++++++++++--
net/bluetooth/6lowpan.c | 52 +++++++++++++------------------------------------
net/ipv6/addrconf.c | 17 +++++++++++-----
4 files changed, 58 insertions(+), 47 deletions(-)

--
2.9.3



2017-02-15 11:58:07

by Alexander Aring

[permalink] [raw]
Subject: Re: [PATCH 5/5] 6lowpan: Use netdev addr_len to determine lladdr len


Hi,

On 02/15/2017 11:49 AM, Luiz Augusto von Dentz wrote:
> Hi Alex,
>
> On Wed, Feb 15, 2017 at 12:21 PM, Alexander Aring <[email protected]> wrote:
>>
>> Hi,
>>
>> On 02/15/2017 09:24 AM, Luiz Augusto von Dentz wrote:
>>> Hi Alex,
>>>
>>> On Wed, Feb 15, 2017 at 9:44 AM, Alexander Aring <[email protected]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 02/09/2017 03:55 PM, Luiz Augusto von Dentz wrote:
>>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>>
>>>>> This allow technologies such as Bluetooth to use its native lladdr which
>>>>> is eui48 instead of eui64 which was expected by functions like
>>>>> lowpan_header_decompress and lowpan_header_compress.
>>>>>
>>>>> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
>>>>> ---
>>>>> net/6lowpan/iphc.c | 17 +++++++++++++++--
>>>>> net/bluetooth/6lowpan.c | 43 ++++++++++---------------------------------
>>>>> 2 files changed, 25 insertions(+), 35 deletions(-)
>>>>>
>>>>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>>>>> index fb5f6fa..ee88feb 100644
>>>>> --- a/net/6lowpan/iphc.c
>>>>> +++ b/net/6lowpan/iphc.c
>>>>> @@ -827,8 +827,21 @@ static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev,
>>>>> }
>>>>> break;
>>>>> default:
>>>>> - /* check for SAM/DAM = 11 */
>>>>> - memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>>>> + switch (dev->addr_len) {
>>>>> + case ETH_ALEN:
>>>>> + memcpy(&tmp.s6_addr[8], lladdr, 3);
>>>>> + tmp.s6_addr[11] = 0xFF;
>>>>> + tmp.s6_addr[12] = 0xFE;
>>>>> + memcpy(&tmp.s6_addr[13], lladdr + 3, 3);
>>>>> + break;
>>>>> + case EUI64_ADDR_LEN:
>>>>> + memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>>>> + break;
>>>>> + default:
>>>>> + dam = LOWPAN_IPHC_DAM_11;
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> /* second bit-flip (Universe/Local) is done according RFC2464 */
>>>>> tmp.s6_addr[8] ^= 0x02;
>>>>
>>>> move this handling in per link-layer layer decision, see below.
>>>>
>>>>> /* context information are always used */
>>>>
>>>> PLEASE... and this is one of my rant!
>>>>
>>>> PLEASE LOOK WHAT I HAVE DONE IN THIS FILE IN MY RFC PATCH SERIES YOU
>>>> NEED TO FIX MORE THAN JUST CONTEXT BASED COMPRESSION.
>>>>
>>>> e.g. stateless un/compression
>>>
>>> I haven't changed that and probably it needs a separate patch in that
>>> case since Im only, and I really mean only, fixing the address length
>>> and because your patches end up changing a lot more things it is
>>> pretty hard to extract the exact parts that fixes this.
>>>
>>
>> Yes, then please check everything which makes use of L3 address handling in
>> combination with the L2 address. This will be simple broken if you
>> remove the FF:FE pattern which was wrong before.
>
> Nope, FF:FE is the correct filler bytes for the interface identifier
> in case you haven't read:
> https://wiki.tools.ietf.org/html/rfc7668#section-3.2.2
>

Yes, but YOU fixing the L2 address handling to not use FF:FE bit pattern
anymore. You didn't touch all of compression/uncompression code which
still use "just copying 8 bytes from given lladdr to generate the IID".

>> In my patch-series I suppose I hit all parts and also testing it.
>
> Im pretty sure it was only possible to test with Linux to Linux in
> terms of Ble 6lowpan since there did not existed anything else, now we
> have zephyr to interoperate which is what Im testing with.
>

hmpf, what shall I say now... the BTLE 6LoWPAN code is still broken. At
my view of handling link-layer address.

>>>> Now I for this, I want to see a link-layer evaluation not address length based.
>>>> Please do so also for the other patch which fixing some behaviour in ipv6.
>>>> The reason is here that uncompress/compress addresses are defined by RFC
>>>> 6LoWPAN adapation.
>>>>
>>>> There is already a link-layer case, so please add BTLE LLTYPE there! Not
>>>> move your handling in default, add some WARN_ONCE there...
>>>
>>> Here I have to pretty blunt, it is a mistake to put link layer logic
>>> inside the 6lowpan module, first it creates a dependencies of these
>>> technologies and second it slow down the development because we end up
>>> with multiple trees having to synchronize which each other.
>>>
>>
>> This is only because the bluetooth 6LoWPAN implementation was wrong at
>> the initial commit.
>>
>> Synchronize each other? No, your changes doesn't require any 802.15.4
>> changes. You just need to fix it right and doing it not more broken than
>> before... which this patch does by fixing only the half of un/compress
>> addresses. Then some cases simple doesn't work. It's even worse, you
>> will grab 8 bytes from a pointer which is 6 bytes now.
>
> Don't I have to update the parts under net/6lowpan, doesn't that get
> handled in linux-wpan tree? Doesn't linux-bluetooth needs to
> synchronize with linux-wpan in case any of these functions get
> changed? For example if we do introduce a callback...
>

If you evaluate the LLTYPE before you can do what you want. But you need
to fix ALL handling of link-layer address compression/uncompression. You
changed only compress of stateful handling -> which is only 1/4 part of
what you need.

Sorry.

>> So now, to you link-layer logic... This is how 6LoWPAN works,
>> compression/uncompression will reconstruct L3 addresses from information
>> from L2 header...
>>
>> What we could do is to provide one callback to make a SLAAC address to a
>> given prefix, then call it in SLAAC generation (ipv6) or here in IPHC.
>> I think this is one of my 1001 cleanups for 6LoWPAN which I have in my
>> mind. Remove the switch-case stuff and make callback to provide this
>> handling which depends on L2 6LOWPAN implementation.
>
> And who said that SLAAC needs to be generated by 6lowpan code? The
> fact that 6lowpan used SLAAC does not mean it has to be generated
> there, actually this is the very reason there so much duplicated code
> trying to generate the same ipv6 based on the MAC address when in fact
> it never changes.
>

For stateless and source mac address -> the address will never changed.
I agree: for destination -> people can add different lladdr in ndisc.
Don't know the bluetooth policy there, maybe drop it... it should not
happen.

In case of stateful you don't know the L3 reconstruct address.

>> So we have no link-layer handling there anymore, but it's moved to a
>> callback which contains the 6LoWPAN BTLE module.
>
> We can do this upfront, we just need to settle in a common format,
> e.g.: interface identifier and that it.
>
>>> Also I have not seen a single mention of link local IP that is not
>>> using 6 or 8 bytes link layer format, and in fact if there exist one
>>> then we should probably have the caller handle the link local ip
>>> format, but then again I haven't seen any indication that these needs
>>> changing in fact for 15.4 16 bit address you can generate a valid 8
>>> bytes address before calling 6lowpan API, which is probably what we
>>> should do to avoid depending on 15.4 code inside 6lowpan.
>>>
>>
>> yes there is _currently_ not other 6LoWPAN implementation which has 8 or
>> 6 bytes.
>>
>> What do you think about a callback? Then these 2 byte 802.15.4 address
>> magic will be handled by net/ieee802154/6lowpan/core.c and we also can
>> call the callback in IPv6 code to make SLAAC address.
>
> The IID should be enough.
>

I don't know what I should say here. I need to work on my exam now. I am
busy with other things... send new patches and I will review it.

Do you see that you need to fix more of these stateless/stateful
compression functions?

- Alex

2017-02-15 11:50:35

by Alexander Aring

[permalink] [raw]
Subject: Re: [PATCH 5/5] 6lowpan: Use netdev addr_len to determine lladdr len


Hi,

On 02/15/2017 11:54 AM, Luiz Augusto von Dentz wrote:
> Hi Alex,
>
> On Wed, Feb 15, 2017 at 12:28 PM, Alexander Aring <[email protected]> wrote:
>> Hi,
>>
>> On 02/15/2017 11:16 AM, Luiz Augusto von Dentz wrote:
>>> Hi Alex,
>>>
>>> On Wed, Feb 15, 2017 at 10:24 AM, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>> Hi Alex,
>>>>
>>>> On Wed, Feb 15, 2017 at 9:44 AM, Alexander Aring <[email protected]> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 02/09/2017 03:55 PM, Luiz Augusto von Dentz wrote:
>>>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>>>
>>>>>> This allow technologies such as Bluetooth to use its native lladdr which
>>>>>> is eui48 instead of eui64 which was expected by functions like
>>>>>> lowpan_header_decompress and lowpan_header_compress.
>>>>>>
>>>>>> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
>>>>>> ---
>>>>>> net/6lowpan/iphc.c | 17 +++++++++++++++--
>>>>>> net/bluetooth/6lowpan.c | 43 ++++++++++---------------------------------
>>>>>> 2 files changed, 25 insertions(+), 35 deletions(-)
>>>>>>
>>>>>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>>>>>> index fb5f6fa..ee88feb 100644
>>>>>> --- a/net/6lowpan/iphc.c
>>>>>> +++ b/net/6lowpan/iphc.c
>>>>>> @@ -827,8 +827,21 @@ static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev,
>>>>>> }
>>>>>> break;
>>>>>> default:
>>>>>> - /* check for SAM/DAM = 11 */
>>>>>> - memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>>>>> + switch (dev->addr_len) {
>>>>>> + case ETH_ALEN:
>>>>>> + memcpy(&tmp.s6_addr[8], lladdr, 3);
>>>>>> + tmp.s6_addr[11] = 0xFF;
>>>>>> + tmp.s6_addr[12] = 0xFE;
>>>>>> + memcpy(&tmp.s6_addr[13], lladdr + 3, 3);
>>>>>> + break;
>>>>>> + case EUI64_ADDR_LEN:
>>>>>> + memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>>>>> + break;
>>>>>> + default:
>>>>>> + dam = LOWPAN_IPHC_DAM_11;
>>>>>> + goto out;
>>>>>> + }
>>>>>> +
>>>>>> /* second bit-flip (Universe/Local) is done according RFC2464 */
>>>>>> tmp.s6_addr[8] ^= 0x02;
>>>>>
>>>>> move this handling in per link-layer layer decision, see below.
>>>>>
>>>>>> /* context information are always used */
>>>>>
>>>>> PLEASE... and this is one of my rant!
>>>>>
>>>>> PLEASE LOOK WHAT I HAVE DONE IN THIS FILE IN MY RFC PATCH SERIES YOU
>>>>> NEED TO FIX MORE THAN JUST CONTEXT BASED COMPRESSION.
>>>>>
>>>>> e.g. stateless un/compression
>>>>
>>>> I haven't changed that and probably it needs a separate patch in that
>>>> case since Im only, and I really mean only, fixing the address length
>>>> and because your patches end up changing a lot more things it is
>>>> pretty hard to extract the exact parts that fixes this.
>>>
>>> Btw I just checked your patch and there doesn't seem to have fix what
>>> you saying, in fact it is exact the same code as above:
>>>
>>> http://www.spinics.net/lists/linux-bluetooth/msg67937.html
>>>
>>
>> Just to be sure we talking about the both thing:
>>
>> For example:
>> lowpan_iphc_uncompress_addr
>>
>>
>> You didn't changed that function which still use 8 byte lladdr and FF:FE
>> pattern. It will still be broken.
>
> And what make you believe it was broken in the first place, ndisc was
> broken because of address length was wrong but in case of 6lowpan it
> is still correct to use FF:FE in the IID following which is what is
> state here:
>
> https://wiki.tools.ietf.org/html/rfc7668#section-3.2.2
>

Yes, this patch series fix to remove FF:FE pattern and change addr_len
from 8 to 6.

You need to change _everything_ to reconstruct the L3 address from L2
address in IPHC code, if you doing that. Because this code thinks still
"we have 8 bytes with FF:FE pattern". This patch series makes more
broken than repairing it!

That's why my Patch series simple remove old code and introduce new
code... When you doing it clean: You need to touch every handling,
that's why 1. remove btle 6lowpan 2. make address handling 3. add new code.

>> My idea: Make a callback with prefix and lladdr as parameter to generate
>> such address. Then on stateless you can call it with L2 saddr/daddr and
>> ff80::/64 prefix. For stateful the same but with prefix from context
>> table.
>
> These address never changes in case of Bluetooth, besides I don't
> think we want to risk the callback calling something else in a
> re-entrant fashion, so I think what we should give the IID or the
> link-local ipv6 address directly and stop generating the address on
> every compress/uncompress.

It's not true for stateful compression, for stateless - okay yes you are
right.

- Alex

2017-02-15 10:54:20

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 5/5] 6lowpan: Use netdev addr_len to determine lladdr len

Hi Alex,

On Wed, Feb 15, 2017 at 12:28 PM, Alexander Aring <[email protected]> wrote:
> Hi,
>
> On 02/15/2017 11:16 AM, Luiz Augusto von Dentz wrote:
>> Hi Alex,
>>
>> On Wed, Feb 15, 2017 at 10:24 AM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> Hi Alex,
>>>
>>> On Wed, Feb 15, 2017 at 9:44 AM, Alexander Aring <[email protected]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 02/09/2017 03:55 PM, Luiz Augusto von Dentz wrote:
>>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>>
>>>>> This allow technologies such as Bluetooth to use its native lladdr which
>>>>> is eui48 instead of eui64 which was expected by functions like
>>>>> lowpan_header_decompress and lowpan_header_compress.
>>>>>
>>>>> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
>>>>> ---
>>>>> net/6lowpan/iphc.c | 17 +++++++++++++++--
>>>>> net/bluetooth/6lowpan.c | 43 ++++++++++---------------------------------
>>>>> 2 files changed, 25 insertions(+), 35 deletions(-)
>>>>>
>>>>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>>>>> index fb5f6fa..ee88feb 100644
>>>>> --- a/net/6lowpan/iphc.c
>>>>> +++ b/net/6lowpan/iphc.c
>>>>> @@ -827,8 +827,21 @@ static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev,
>>>>> }
>>>>> break;
>>>>> default:
>>>>> - /* check for SAM/DAM = 11 */
>>>>> - memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>>>> + switch (dev->addr_len) {
>>>>> + case ETH_ALEN:
>>>>> + memcpy(&tmp.s6_addr[8], lladdr, 3);
>>>>> + tmp.s6_addr[11] = 0xFF;
>>>>> + tmp.s6_addr[12] = 0xFE;
>>>>> + memcpy(&tmp.s6_addr[13], lladdr + 3, 3);
>>>>> + break;
>>>>> + case EUI64_ADDR_LEN:
>>>>> + memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>>>> + break;
>>>>> + default:
>>>>> + dam = LOWPAN_IPHC_DAM_11;
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> /* second bit-flip (Universe/Local) is done according RFC2464 */
>>>>> tmp.s6_addr[8] ^= 0x02;
>>>>
>>>> move this handling in per link-layer layer decision, see below.
>>>>
>>>>> /* context information are always used */
>>>>
>>>> PLEASE... and this is one of my rant!
>>>>
>>>> PLEASE LOOK WHAT I HAVE DONE IN THIS FILE IN MY RFC PATCH SERIES YOU
>>>> NEED TO FIX MORE THAN JUST CONTEXT BASED COMPRESSION.
>>>>
>>>> e.g. stateless un/compression
>>>
>>> I haven't changed that and probably it needs a separate patch in that
>>> case since Im only, and I really mean only, fixing the address length
>>> and because your patches end up changing a lot more things it is
>>> pretty hard to extract the exact parts that fixes this.
>>
>> Btw I just checked your patch and there doesn't seem to have fix what
>> you saying, in fact it is exact the same code as above:
>>
>> http://www.spinics.net/lists/linux-bluetooth/msg67937.html
>>
>
> Just to be sure we talking about the both thing:
>
> For example:
> lowpan_iphc_uncompress_addr
>
>
> You didn't changed that function which still use 8 byte lladdr and FF:FE
> pattern. It will still be broken.

And what make you believe it was broken in the first place, ndisc was
broken because of address length was wrong but in case of 6lowpan it
is still correct to use FF:FE in the IID following which is what is
state here:

https://wiki.tools.ietf.org/html/rfc7668#section-3.2.2

> My idea: Make a callback with prefix and lladdr as parameter to generate
> such address. Then on stateless you can call it with L2 saddr/daddr and
> ff80::/64 prefix. For stateful the same but with prefix from context
> table.

These address never changes in case of Bluetooth, besides I don't
think we want to risk the callback calling something else in a
re-entrant fashion, so I think what we should give the IID or the
link-local ipv6 address directly and stop generating the address on
every compress/uncompress.


--
Luiz Augusto von Dentz

2017-02-15 10:49:57

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 5/5] 6lowpan: Use netdev addr_len to determine lladdr len

Hi Alex,

On Wed, Feb 15, 2017 at 12:21 PM, Alexander Aring <[email protected]> wrote:
>
> Hi,
>
> On 02/15/2017 09:24 AM, Luiz Augusto von Dentz wrote:
>> Hi Alex,
>>
>> On Wed, Feb 15, 2017 at 9:44 AM, Alexander Aring <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>> On 02/09/2017 03:55 PM, Luiz Augusto von Dentz wrote:
>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>
>>>> This allow technologies such as Bluetooth to use its native lladdr which
>>>> is eui48 instead of eui64 which was expected by functions like
>>>> lowpan_header_decompress and lowpan_header_compress.
>>>>
>>>> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
>>>> ---
>>>> net/6lowpan/iphc.c | 17 +++++++++++++++--
>>>> net/bluetooth/6lowpan.c | 43 ++++++++++---------------------------------
>>>> 2 files changed, 25 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>>>> index fb5f6fa..ee88feb 100644
>>>> --- a/net/6lowpan/iphc.c
>>>> +++ b/net/6lowpan/iphc.c
>>>> @@ -827,8 +827,21 @@ static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev,
>>>> }
>>>> break;
>>>> default:
>>>> - /* check for SAM/DAM = 11 */
>>>> - memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>>> + switch (dev->addr_len) {
>>>> + case ETH_ALEN:
>>>> + memcpy(&tmp.s6_addr[8], lladdr, 3);
>>>> + tmp.s6_addr[11] = 0xFF;
>>>> + tmp.s6_addr[12] = 0xFE;
>>>> + memcpy(&tmp.s6_addr[13], lladdr + 3, 3);
>>>> + break;
>>>> + case EUI64_ADDR_LEN:
>>>> + memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>>> + break;
>>>> + default:
>>>> + dam = LOWPAN_IPHC_DAM_11;
>>>> + goto out;
>>>> + }
>>>> +
>>>> /* second bit-flip (Universe/Local) is done according RFC2464 */
>>>> tmp.s6_addr[8] ^= 0x02;
>>>
>>> move this handling in per link-layer layer decision, see below.
>>>
>>>> /* context information are always used */
>>>
>>> PLEASE... and this is one of my rant!
>>>
>>> PLEASE LOOK WHAT I HAVE DONE IN THIS FILE IN MY RFC PATCH SERIES YOU
>>> NEED TO FIX MORE THAN JUST CONTEXT BASED COMPRESSION.
>>>
>>> e.g. stateless un/compression
>>
>> I haven't changed that and probably it needs a separate patch in that
>> case since Im only, and I really mean only, fixing the address length
>> and because your patches end up changing a lot more things it is
>> pretty hard to extract the exact parts that fixes this.
>>
>
> Yes, then please check everything which makes use of L3 address handling in
> combination with the L2 address. This will be simple broken if you
> remove the FF:FE pattern which was wrong before.

Nope, FF:FE is the correct filler bytes for the interface identifier
in case you haven't read:
https://wiki.tools.ietf.org/html/rfc7668#section-3.2.2

> In my patch-series I suppose I hit all parts and also testing it.

Im pretty sure it was only possible to test with Linux to Linux in
terms of Ble 6lowpan since there did not existed anything else, now we
have zephyr to interoperate which is what Im testing with.

>>> Now I for this, I want to see a link-layer evaluation not address length based.
>>> Please do so also for the other patch which fixing some behaviour in ipv6.
>>> The reason is here that uncompress/compress addresses are defined by RFC
>>> 6LoWPAN adapation.
>>>
>>> There is already a link-layer case, so please add BTLE LLTYPE there! Not
>>> move your handling in default, add some WARN_ONCE there...
>>
>> Here I have to pretty blunt, it is a mistake to put link layer logic
>> inside the 6lowpan module, first it creates a dependencies of these
>> technologies and second it slow down the development because we end up
>> with multiple trees having to synchronize which each other.
>>
>
> This is only because the bluetooth 6LoWPAN implementation was wrong at
> the initial commit.
>
> Synchronize each other? No, your changes doesn't require any 802.15.4
> changes. You just need to fix it right and doing it not more broken than
> before... which this patch does by fixing only the half of un/compress
> addresses. Then some cases simple doesn't work. It's even worse, you
> will grab 8 bytes from a pointer which is 6 bytes now.

Don't I have to update the parts under net/6lowpan, doesn't that get
handled in linux-wpan tree? Doesn't linux-bluetooth needs to
synchronize with linux-wpan in case any of these functions get
changed? For example if we do introduce a callback...

> So now, to you link-layer logic... This is how 6LoWPAN works,
> compression/uncompression will reconstruct L3 addresses from information
> from L2 header...
>
> What we could do is to provide one callback to make a SLAAC address to a
> given prefix, then call it in SLAAC generation (ipv6) or here in IPHC.
> I think this is one of my 1001 cleanups for 6LoWPAN which I have in my
> mind. Remove the switch-case stuff and make callback to provide this
> handling which depends on L2 6LOWPAN implementation.

And who said that SLAAC needs to be generated by 6lowpan code? The
fact that 6lowpan used SLAAC does not mean it has to be generated
there, actually this is the very reason there so much duplicated code
trying to generate the same ipv6 based on the MAC address when in fact
it never changes.

> So we have no link-layer handling there anymore, but it's moved to a
> callback which contains the 6LoWPAN BTLE module.

We can do this upfront, we just need to settle in a common format,
e.g.: interface identifier and that it.

>> Also I have not seen a single mention of link local IP that is not
>> using 6 or 8 bytes link layer format, and in fact if there exist one
>> then we should probably have the caller handle the link local ip
>> format, but then again I haven't seen any indication that these needs
>> changing in fact for 15.4 16 bit address you can generate a valid 8
>> bytes address before calling 6lowpan API, which is probably what we
>> should do to avoid depending on 15.4 code inside 6lowpan.
>>
>
> yes there is _currently_ not other 6LoWPAN implementation which has 8 or
> 6 bytes.
>
> What do you think about a callback? Then these 2 byte 802.15.4 address
> magic will be handled by net/ieee802154/6lowpan/core.c and we also can
> call the callback in IPv6 code to make SLAAC address.

The IID should be enough.


--
Luiz Augusto von Dentz

2017-02-15 10:28:00

by Alexander Aring

[permalink] [raw]
Subject: Re: [PATCH 5/5] 6lowpan: Use netdev addr_len to determine lladdr len

Hi,

On 02/15/2017 11:16 AM, Luiz Augusto von Dentz wrote:
> Hi Alex,
>
> On Wed, Feb 15, 2017 at 10:24 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Alex,
>>
>> On Wed, Feb 15, 2017 at 9:44 AM, Alexander Aring <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>> On 02/09/2017 03:55 PM, Luiz Augusto von Dentz wrote:
>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>
>>>> This allow technologies such as Bluetooth to use its native lladdr which
>>>> is eui48 instead of eui64 which was expected by functions like
>>>> lowpan_header_decompress and lowpan_header_compress.
>>>>
>>>> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
>>>> ---
>>>> net/6lowpan/iphc.c | 17 +++++++++++++++--
>>>> net/bluetooth/6lowpan.c | 43 ++++++++++---------------------------------
>>>> 2 files changed, 25 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>>>> index fb5f6fa..ee88feb 100644
>>>> --- a/net/6lowpan/iphc.c
>>>> +++ b/net/6lowpan/iphc.c
>>>> @@ -827,8 +827,21 @@ static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev,
>>>> }
>>>> break;
>>>> default:
>>>> - /* check for SAM/DAM = 11 */
>>>> - memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>>> + switch (dev->addr_len) {
>>>> + case ETH_ALEN:
>>>> + memcpy(&tmp.s6_addr[8], lladdr, 3);
>>>> + tmp.s6_addr[11] = 0xFF;
>>>> + tmp.s6_addr[12] = 0xFE;
>>>> + memcpy(&tmp.s6_addr[13], lladdr + 3, 3);
>>>> + break;
>>>> + case EUI64_ADDR_LEN:
>>>> + memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>>> + break;
>>>> + default:
>>>> + dam = LOWPAN_IPHC_DAM_11;
>>>> + goto out;
>>>> + }
>>>> +
>>>> /* second bit-flip (Universe/Local) is done according RFC2464 */
>>>> tmp.s6_addr[8] ^= 0x02;
>>>
>>> move this handling in per link-layer layer decision, see below.
>>>
>>>> /* context information are always used */
>>>
>>> PLEASE... and this is one of my rant!
>>>
>>> PLEASE LOOK WHAT I HAVE DONE IN THIS FILE IN MY RFC PATCH SERIES YOU
>>> NEED TO FIX MORE THAN JUST CONTEXT BASED COMPRESSION.
>>>
>>> e.g. stateless un/compression
>>
>> I haven't changed that and probably it needs a separate patch in that
>> case since Im only, and I really mean only, fixing the address length
>> and because your patches end up changing a lot more things it is
>> pretty hard to extract the exact parts that fixes this.
>
> Btw I just checked your patch and there doesn't seem to have fix what
> you saying, in fact it is exact the same code as above:
>
> http://www.spinics.net/lists/linux-bluetooth/msg67937.html
>

Just to be sure we talking about the both thing:

For example:
lowpan_iphc_uncompress_addr


You didn't changed that function which still use 8 byte lladdr and FF:FE
pattern. It will still be broken.

My idea: Make a callback with prefix and lladdr as parameter to generate
such address. Then on stateless you can call it with L2 saddr/daddr and
ff80::/64 prefix. For stateful the same but with prefix from context
table.

btw: I cc now linux-wpan.

- Alex

2017-02-15 10:21:01

by Alexander Aring

[permalink] [raw]
Subject: Re: [PATCH 5/5] 6lowpan: Use netdev addr_len to determine lladdr len


Hi,

On 02/15/2017 09:24 AM, Luiz Augusto von Dentz wrote:
> Hi Alex,
>
> On Wed, Feb 15, 2017 at 9:44 AM, Alexander Aring <[email protected]> wrote:
>>
>> Hi,
>>
>> On 02/09/2017 03:55 PM, Luiz Augusto von Dentz wrote:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> This allow technologies such as Bluetooth to use its native lladdr which
>>> is eui48 instead of eui64 which was expected by functions like
>>> lowpan_header_decompress and lowpan_header_compress.
>>>
>>> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
>>> ---
>>> net/6lowpan/iphc.c | 17 +++++++++++++++--
>>> net/bluetooth/6lowpan.c | 43 ++++++++++---------------------------------
>>> 2 files changed, 25 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>>> index fb5f6fa..ee88feb 100644
>>> --- a/net/6lowpan/iphc.c
>>> +++ b/net/6lowpan/iphc.c
>>> @@ -827,8 +827,21 @@ static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev,
>>> }
>>> break;
>>> default:
>>> - /* check for SAM/DAM = 11 */
>>> - memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>> + switch (dev->addr_len) {
>>> + case ETH_ALEN:
>>> + memcpy(&tmp.s6_addr[8], lladdr, 3);
>>> + tmp.s6_addr[11] = 0xFF;
>>> + tmp.s6_addr[12] = 0xFE;
>>> + memcpy(&tmp.s6_addr[13], lladdr + 3, 3);
>>> + break;
>>> + case EUI64_ADDR_LEN:
>>> + memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>> + break;
>>> + default:
>>> + dam = LOWPAN_IPHC_DAM_11;
>>> + goto out;
>>> + }
>>> +
>>> /* second bit-flip (Universe/Local) is done according RFC2464 */
>>> tmp.s6_addr[8] ^= 0x02;
>>
>> move this handling in per link-layer layer decision, see below.
>>
>>> /* context information are always used */
>>
>> PLEASE... and this is one of my rant!
>>
>> PLEASE LOOK WHAT I HAVE DONE IN THIS FILE IN MY RFC PATCH SERIES YOU
>> NEED TO FIX MORE THAN JUST CONTEXT BASED COMPRESSION.
>>
>> e.g. stateless un/compression
>
> I haven't changed that and probably it needs a separate patch in that
> case since Im only, and I really mean only, fixing the address length
> and because your patches end up changing a lot more things it is
> pretty hard to extract the exact parts that fixes this.
>

Yes, then please check everything which makes use of L3 address handling in
combination with the L2 address. This will be simple broken if you
remove the FF:FE pattern which was wrong before.

In my patch-series I suppose I hit all parts and also testing it.

>> Now I for this, I want to see a link-layer evaluation not address length based.
>> Please do so also for the other patch which fixing some behaviour in ipv6.
>> The reason is here that uncompress/compress addresses are defined by RFC
>> 6LoWPAN adapation.
>>
>> There is already a link-layer case, so please add BTLE LLTYPE there! Not
>> move your handling in default, add some WARN_ONCE there...
>
> Here I have to pretty blunt, it is a mistake to put link layer logic
> inside the 6lowpan module, first it creates a dependencies of these
> technologies and second it slow down the development because we end up
> with multiple trees having to synchronize which each other.
>

This is only because the bluetooth 6LoWPAN implementation was wrong at
the initial commit.

Synchronize each other? No, your changes doesn't require any 802.15.4
changes. You just need to fix it right and doing it not more broken than
before... which this patch does by fixing only the half of un/compress
addresses. Then some cases simple doesn't work. It's even worse, you
will grab 8 bytes from a pointer which is 6 bytes now.

So now, to you link-layer logic... This is how 6LoWPAN works,
compression/uncompression will reconstruct L3 addresses from information
from L2 header...

What we could do is to provide one callback to make a SLAAC address to a
given prefix, then call it in SLAAC generation (ipv6) or here in IPHC.
I think this is one of my 1001 cleanups for 6LoWPAN which I have in my
mind. Remove the switch-case stuff and make callback to provide this
handling which depends on L2 6LOWPAN implementation.

So we have no link-layer handling there anymore, but it's moved to a
callback which contains the 6LoWPAN BTLE module.

> Also I have not seen a single mention of link local IP that is not
> using 6 or 8 bytes link layer format, and in fact if there exist one
> then we should probably have the caller handle the link local ip
> format, but then again I haven't seen any indication that these needs
> changing in fact for 15.4 16 bit address you can generate a valid 8
> bytes address before calling 6lowpan API, which is probably what we
> should do to avoid depending on 15.4 code inside 6lowpan.
>

yes there is _currently_ not other 6LoWPAN implementation which has 8 or
6 bytes.

What do you think about a callback? Then these 2 byte 802.15.4 address
magic will be handled by net/ieee802154/6lowpan/core.c and we also can
call the callback in IPv6 code to make SLAAC address.

I think this is the right way to do it.

- Alex

2017-02-15 10:16:46

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 5/5] 6lowpan: Use netdev addr_len to determine lladdr len

Hi Alex,

On Wed, Feb 15, 2017 at 10:24 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Alex,
>
> On Wed, Feb 15, 2017 at 9:44 AM, Alexander Aring <[email protected]> wrote:
>>
>> Hi,
>>
>> On 02/09/2017 03:55 PM, Luiz Augusto von Dentz wrote:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> This allow technologies such as Bluetooth to use its native lladdr which
>>> is eui48 instead of eui64 which was expected by functions like
>>> lowpan_header_decompress and lowpan_header_compress.
>>>
>>> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
>>> ---
>>> net/6lowpan/iphc.c | 17 +++++++++++++++--
>>> net/bluetooth/6lowpan.c | 43 ++++++++++---------------------------------
>>> 2 files changed, 25 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>>> index fb5f6fa..ee88feb 100644
>>> --- a/net/6lowpan/iphc.c
>>> +++ b/net/6lowpan/iphc.c
>>> @@ -827,8 +827,21 @@ static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev,
>>> }
>>> break;
>>> default:
>>> - /* check for SAM/DAM = 11 */
>>> - memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>> + switch (dev->addr_len) {
>>> + case ETH_ALEN:
>>> + memcpy(&tmp.s6_addr[8], lladdr, 3);
>>> + tmp.s6_addr[11] = 0xFF;
>>> + tmp.s6_addr[12] = 0xFE;
>>> + memcpy(&tmp.s6_addr[13], lladdr + 3, 3);
>>> + break;
>>> + case EUI64_ADDR_LEN:
>>> + memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>>> + break;
>>> + default:
>>> + dam = LOWPAN_IPHC_DAM_11;
>>> + goto out;
>>> + }
>>> +
>>> /* second bit-flip (Universe/Local) is done according RFC2464 */
>>> tmp.s6_addr[8] ^= 0x02;
>>
>> move this handling in per link-layer layer decision, see below.
>>
>>> /* context information are always used */
>>
>> PLEASE... and this is one of my rant!
>>
>> PLEASE LOOK WHAT I HAVE DONE IN THIS FILE IN MY RFC PATCH SERIES YOU
>> NEED TO FIX MORE THAN JUST CONTEXT BASED COMPRESSION.
>>
>> e.g. stateless un/compression
>
> I haven't changed that and probably it needs a separate patch in that
> case since Im only, and I really mean only, fixing the address length
> and because your patches end up changing a lot more things it is
> pretty hard to extract the exact parts that fixes this.

Btw I just checked your patch and there doesn't seem to have fix what
you saying, in fact it is exact the same code as above:

http://www.spinics.net/lists/linux-bluetooth/msg67937.html

>> Now I for this, I want to see a link-layer evaluation not address length based.
>> Please do so also for the other patch which fixing some behaviour in ipv6.
>> The reason is here that uncompress/compress addresses are defined by RFC
>> 6LoWPAN adapation.
>>
>> There is already a link-layer case, so please add BTLE LLTYPE there! Not
>> move your handling in default, add some WARN_ONCE there...
>
> Here I have to pretty blunt, it is a mistake to put link layer logic
> inside the 6lowpan module, first it creates a dependencies of these
> technologies and second it slow down the development because we end up
> with multiple trees having to synchronize which each other.
>
> Also I have not seen a single mention of link local IP that is not
> using 6 or 8 bytes link layer format, and in fact if there exist one
> then we should probably have the caller handle the link local ip
> format, but then again I haven't seen any indication that these needs
> changing in fact for 15.4 16 bit address you can generate a valid 8
> bytes address before calling 6lowpan API, which is probably what we
> should do to avoid depending on 15.4 code inside 6lowpan.
>
>> btw:
>>
>> here exists also a cleanup to not always implement the same stuff. We
>> need for stateful/stateless compression one function, because and
>> remember my words if you want to make this cleanup:
>>
>> Stateless compression is stateful compression with prefix fe80::/64 as
>> context.
>>
>> ---
>>
>> Sorry for my rant, but I am somehow pissed off because intel people
>> doesn't look what I did there in my RFC.
>>
>> ... I think now I know why linux kernel people do sometimes a rant. :-)
>>
>> Thanks.
>>
>>> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
>>> index 1456b01..d2a1aa5 100644
>>> --- a/net/bluetooth/6lowpan.c
>>> +++ b/net/bluetooth/6lowpan.c
>>> @@ -64,7 +64,7 @@ struct lowpan_peer {
>>> struct l2cap_chan *chan;
>>>
>>> /* peer addresses in various formats */
>>> - unsigned char eui64_addr[EUI64_ADDR_LEN];
>>> + unsigned char lladdr[ETH_ALEN];
>>> struct in6_addr peer_addr;
>>> };
>>>
>>> @@ -80,8 +80,6 @@ struct lowpan_btle_dev {
>>> struct delayed_work notify_peers;
>>> };
>>>
>>> -static void set_addr(u8 *eui, u8 *addr, u8 addr_type);
>>> -
>>> static inline struct lowpan_btle_dev *
>>> lowpan_btle_dev(const struct net_device *netdev)
>>> {
>>> @@ -277,7 +275,6 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
>>> const u8 *saddr;
>>> struct lowpan_btle_dev *dev;
>>> struct lowpan_peer *peer;
>>> - unsigned char eui64_daddr[EUI64_ADDR_LEN];
>>>
>>> dev = lowpan_btle_dev(netdev);
>>>
>>> @@ -287,10 +284,9 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
>>> if (!peer)
>>> return -EINVAL;
>>>
>>> - saddr = peer->eui64_addr;
>>> - set_addr(&eui64_daddr[0], chan->src.b, chan->src_type);
>>> + saddr = peer->lladdr;
>>>
>>> - return lowpan_header_decompress(skb, netdev, &eui64_daddr, saddr);
>>> + return lowpan_header_decompress(skb, netdev, netdev->dev_addr, saddr);
>>
>> This looks not right, you are sure that netdev->dev_addr is the
>> _destination_ address? It looks very confusing, because dev_addr is
>> normally the source address of the netdevice interface.
>>
>>> }
>>>
>>> static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>>> @@ -477,7 +473,7 @@ static int setup_header(struct sk_buff *skb, struct net_device *netdev,
>>> }
>>> }
>>>
>>> - daddr = peer->eui64_addr;
>>> + daddr = peer->lladdr;
>>> *peer_addr = addr;
>>> *peer_addr_type = addr_type;
>>> lowpan_cb(skb)->chan = peer->chan;
>>> @@ -663,27 +659,6 @@ static struct device_type bt_type = {
>>> .name = "bluetooth",
>>> };
>>>
>>> -static void set_addr(u8 *eui, u8 *addr, u8 addr_type)
>>> -{
>>> - /* addr is the BT address in little-endian format */
>>> - eui[0] = addr[5];
>>> - eui[1] = addr[4];
>>> - eui[2] = addr[3];
>>> - eui[3] = 0xFF;
>>> - eui[4] = 0xFE;
>>> - eui[5] = addr[2];
>>> - eui[6] = addr[1];
>>> - eui[7] = addr[0];
>>> -
>>> - /* Universal/local bit set, BT 6lowpan draft ch. 3.2.1 */
>>> - if (addr_type == BDADDR_LE_PUBLIC)
>>> - eui[0] &= ~0x02;
>>> - else
>>> - eui[0] |= 0x02;
>>> -
>>> - BT_DBG("type %d addr %*phC", addr_type, 8, eui);
>>> -}
>>> -
>>
>> YAY, this function is totally wrong! :-)
>>
>>> static void ifup(struct net_device *netdev)
>>> {
>>> int err;
>>> @@ -762,14 +737,16 @@ static struct l2cap_chan *add_peer_chan(struct l2cap_chan *chan,
>>> peer->chan = chan;
>>> memset(&peer->peer_addr, 0, sizeof(struct in6_addr));
>>>
>>> + baswap((void *)peer->lladdr, &chan->dst);
>>> +
>>> /* RFC 2464 ch. 5 */
>>> peer->peer_addr.s6_addr[0] = 0xFE;
>>> peer->peer_addr.s6_addr[1] = 0x80;
>>> - set_addr((u8 *)&peer->peer_addr.s6_addr + 8, chan->dst.b,
>>> - chan->dst_type);
>>>
>>> - memcpy(&peer->eui64_addr, (u8 *)&peer->peer_addr.s6_addr + 8,
>>> - EUI64_ADDR_LEN);
>>> + memcpy(&peer->peer_addr.s6_addr[8], peer->lladdr, 3);
>>> + peer->peer_addr.s6_addr[11] = 0xFF;
>>> + peer->peer_addr.s6_addr[12] = 0xFE;
>>> + memcpy(&peer->peer_addr.s6_addr[13], peer->lladdr + 3, 3);
>>>
>>
>> What the hell is that? I suppose this is to handling something correct
>> to a behaviour which is another big thing which is totally broken in the
>> BTLE 6LoWPAN code.
>>
>>> /* IPv6 address needs to have the U/L bit set properly so toggle
>>> * it back here.
>>>
>>
>> - Alex
>
>
>
> --
> Luiz Augusto von Dentz



--
Luiz Augusto von Dentz

2017-02-15 08:24:02

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 5/5] 6lowpan: Use netdev addr_len to determine lladdr len

Hi Alex,

On Wed, Feb 15, 2017 at 9:44 AM, Alexander Aring <[email protected]> wrote:
>
> Hi,
>
> On 02/09/2017 03:55 PM, Luiz Augusto von Dentz wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> This allow technologies such as Bluetooth to use its native lladdr which
>> is eui48 instead of eui64 which was expected by functions like
>> lowpan_header_decompress and lowpan_header_compress.
>>
>> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
>> ---
>> net/6lowpan/iphc.c | 17 +++++++++++++++--
>> net/bluetooth/6lowpan.c | 43 ++++++++++---------------------------------
>> 2 files changed, 25 insertions(+), 35 deletions(-)
>>
>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>> index fb5f6fa..ee88feb 100644
>> --- a/net/6lowpan/iphc.c
>> +++ b/net/6lowpan/iphc.c
>> @@ -827,8 +827,21 @@ static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev,
>> }
>> break;
>> default:
>> - /* check for SAM/DAM = 11 */
>> - memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>> + switch (dev->addr_len) {
>> + case ETH_ALEN:
>> + memcpy(&tmp.s6_addr[8], lladdr, 3);
>> + tmp.s6_addr[11] = 0xFF;
>> + tmp.s6_addr[12] = 0xFE;
>> + memcpy(&tmp.s6_addr[13], lladdr + 3, 3);
>> + break;
>> + case EUI64_ADDR_LEN:
>> + memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
>> + break;
>> + default:
>> + dam = LOWPAN_IPHC_DAM_11;
>> + goto out;
>> + }
>> +
>> /* second bit-flip (Universe/Local) is done according RFC2464 */
>> tmp.s6_addr[8] ^= 0x02;
>
> move this handling in per link-layer layer decision, see below.
>
>> /* context information are always used */
>
> PLEASE... and this is one of my rant!
>
> PLEASE LOOK WHAT I HAVE DONE IN THIS FILE IN MY RFC PATCH SERIES YOU
> NEED TO FIX MORE THAN JUST CONTEXT BASED COMPRESSION.
>
> e.g. stateless un/compression

I haven't changed that and probably it needs a separate patch in that
case since Im only, and I really mean only, fixing the address length
and because your patches end up changing a lot more things it is
pretty hard to extract the exact parts that fixes this.

> Now I for this, I want to see a link-layer evaluation not address length based.
> Please do so also for the other patch which fixing some behaviour in ipv6.
> The reason is here that uncompress/compress addresses are defined by RFC
> 6LoWPAN adapation.
>
> There is already a link-layer case, so please add BTLE LLTYPE there! Not
> move your handling in default, add some WARN_ONCE there...

Here I have to pretty blunt, it is a mistake to put link layer logic
inside the 6lowpan module, first it creates a dependencies of these
technologies and second it slow down the development because we end up
with multiple trees having to synchronize which each other.

Also I have not seen a single mention of link local IP that is not
using 6 or 8 bytes link layer format, and in fact if there exist one
then we should probably have the caller handle the link local ip
format, but then again I haven't seen any indication that these needs
changing in fact for 15.4 16 bit address you can generate a valid 8
bytes address before calling 6lowpan API, which is probably what we
should do to avoid depending on 15.4 code inside 6lowpan.

> btw:
>
> here exists also a cleanup to not always implement the same stuff. We
> need for stateful/stateless compression one function, because and
> remember my words if you want to make this cleanup:
>
> Stateless compression is stateful compression with prefix fe80::/64 as
> context.
>
> ---
>
> Sorry for my rant, but I am somehow pissed off because intel people
> doesn't look what I did there in my RFC.
>
> ... I think now I know why linux kernel people do sometimes a rant. :-)
>
> Thanks.
>
>> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
>> index 1456b01..d2a1aa5 100644
>> --- a/net/bluetooth/6lowpan.c
>> +++ b/net/bluetooth/6lowpan.c
>> @@ -64,7 +64,7 @@ struct lowpan_peer {
>> struct l2cap_chan *chan;
>>
>> /* peer addresses in various formats */
>> - unsigned char eui64_addr[EUI64_ADDR_LEN];
>> + unsigned char lladdr[ETH_ALEN];
>> struct in6_addr peer_addr;
>> };
>>
>> @@ -80,8 +80,6 @@ struct lowpan_btle_dev {
>> struct delayed_work notify_peers;
>> };
>>
>> -static void set_addr(u8 *eui, u8 *addr, u8 addr_type);
>> -
>> static inline struct lowpan_btle_dev *
>> lowpan_btle_dev(const struct net_device *netdev)
>> {
>> @@ -277,7 +275,6 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
>> const u8 *saddr;
>> struct lowpan_btle_dev *dev;
>> struct lowpan_peer *peer;
>> - unsigned char eui64_daddr[EUI64_ADDR_LEN];
>>
>> dev = lowpan_btle_dev(netdev);
>>
>> @@ -287,10 +284,9 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
>> if (!peer)
>> return -EINVAL;
>>
>> - saddr = peer->eui64_addr;
>> - set_addr(&eui64_daddr[0], chan->src.b, chan->src_type);
>> + saddr = peer->lladdr;
>>
>> - return lowpan_header_decompress(skb, netdev, &eui64_daddr, saddr);
>> + return lowpan_header_decompress(skb, netdev, netdev->dev_addr, saddr);
>
> This looks not right, you are sure that netdev->dev_addr is the
> _destination_ address? It looks very confusing, because dev_addr is
> normally the source address of the netdevice interface.
>
>> }
>>
>> static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>> @@ -477,7 +473,7 @@ static int setup_header(struct sk_buff *skb, struct net_device *netdev,
>> }
>> }
>>
>> - daddr = peer->eui64_addr;
>> + daddr = peer->lladdr;
>> *peer_addr = addr;
>> *peer_addr_type = addr_type;
>> lowpan_cb(skb)->chan = peer->chan;
>> @@ -663,27 +659,6 @@ static struct device_type bt_type = {
>> .name = "bluetooth",
>> };
>>
>> -static void set_addr(u8 *eui, u8 *addr, u8 addr_type)
>> -{
>> - /* addr is the BT address in little-endian format */
>> - eui[0] = addr[5];
>> - eui[1] = addr[4];
>> - eui[2] = addr[3];
>> - eui[3] = 0xFF;
>> - eui[4] = 0xFE;
>> - eui[5] = addr[2];
>> - eui[6] = addr[1];
>> - eui[7] = addr[0];
>> -
>> - /* Universal/local bit set, BT 6lowpan draft ch. 3.2.1 */
>> - if (addr_type == BDADDR_LE_PUBLIC)
>> - eui[0] &= ~0x02;
>> - else
>> - eui[0] |= 0x02;
>> -
>> - BT_DBG("type %d addr %*phC", addr_type, 8, eui);
>> -}
>> -
>
> YAY, this function is totally wrong! :-)
>
>> static void ifup(struct net_device *netdev)
>> {
>> int err;
>> @@ -762,14 +737,16 @@ static struct l2cap_chan *add_peer_chan(struct l2cap_chan *chan,
>> peer->chan = chan;
>> memset(&peer->peer_addr, 0, sizeof(struct in6_addr));
>>
>> + baswap((void *)peer->lladdr, &chan->dst);
>> +
>> /* RFC 2464 ch. 5 */
>> peer->peer_addr.s6_addr[0] = 0xFE;
>> peer->peer_addr.s6_addr[1] = 0x80;
>> - set_addr((u8 *)&peer->peer_addr.s6_addr + 8, chan->dst.b,
>> - chan->dst_type);
>>
>> - memcpy(&peer->eui64_addr, (u8 *)&peer->peer_addr.s6_addr + 8,
>> - EUI64_ADDR_LEN);
>> + memcpy(&peer->peer_addr.s6_addr[8], peer->lladdr, 3);
>> + peer->peer_addr.s6_addr[11] = 0xFF;
>> + peer->peer_addr.s6_addr[12] = 0xFE;
>> + memcpy(&peer->peer_addr.s6_addr[13], peer->lladdr + 3, 3);
>>
>
> What the hell is that? I suppose this is to handling something correct
> to a behaviour which is another big thing which is totally broken in the
> BTLE 6LoWPAN code.
>
>> /* IPv6 address needs to have the U/L bit set properly so toggle
>> * it back here.
>>
>
> - Alex



--
Luiz Augusto von Dentz

2017-02-15 07:46:24

by Alexander Aring

[permalink] [raw]
Subject: Re: [PATCH 0/5] Bluetooth: 6LoWPAN: Fix lladdr length


Hi,

please add cc to linux-wpan mailinglist for 6LoWPAN related changes.

Thanks.

- Alex

2017-02-15 07:44:19

by Alexander Aring

[permalink] [raw]
Subject: Re: [PATCH 5/5] 6lowpan: Use netdev addr_len to determine lladdr len


Hi,

On 02/09/2017 03:55 PM, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> This allow technologies such as Bluetooth to use its native lladdr which
> is eui48 instead of eui64 which was expected by functions like
> lowpan_header_decompress and lowpan_header_compress.
>
> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
> ---
> net/6lowpan/iphc.c | 17 +++++++++++++++--
> net/bluetooth/6lowpan.c | 43 ++++++++++---------------------------------
> 2 files changed, 25 insertions(+), 35 deletions(-)
>
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index fb5f6fa..ee88feb 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -827,8 +827,21 @@ static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev,
> }
> break;
> default:
> - /* check for SAM/DAM = 11 */
> - memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
> + switch (dev->addr_len) {
> + case ETH_ALEN:
> + memcpy(&tmp.s6_addr[8], lladdr, 3);
> + tmp.s6_addr[11] = 0xFF;
> + tmp.s6_addr[12] = 0xFE;
> + memcpy(&tmp.s6_addr[13], lladdr + 3, 3);
> + break;
> + case EUI64_ADDR_LEN:
> + memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
> + break;
> + default:
> + dam = LOWPAN_IPHC_DAM_11;
> + goto out;
> + }
> +
> /* second bit-flip (Universe/Local) is done according RFC2464 */
> tmp.s6_addr[8] ^= 0x02;

move this handling in per link-layer layer decision, see below.

> /* context information are always used */

PLEASE... and this is one of my rant!

PLEASE LOOK WHAT I HAVE DONE IN THIS FILE IN MY RFC PATCH SERIES YOU
NEED TO FIX MORE THAN JUST CONTEXT BASED COMPRESSION.

e.g. stateless un/compression

Now I for this, I want to see a link-layer evaluation not address length based.
Please do so also for the other patch which fixing some behaviour in ipv6.
The reason is here that uncompress/compress addresses are defined by RFC
6LoWPAN adapation.

There is already a link-layer case, so please add BTLE LLTYPE there! Not
move your handling in default, add some WARN_ONCE there...

btw:

here exists also a cleanup to not always implement the same stuff. We
need for stateful/stateless compression one function, because and
remember my words if you want to make this cleanup:

Stateless compression is stateful compression with prefix fe80::/64 as
context.

---

Sorry for my rant, but I am somehow pissed off because intel people
doesn't look what I did there in my RFC.

... I think now I know why linux kernel people do sometimes a rant. :-)

Thanks.

> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index 1456b01..d2a1aa5 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -64,7 +64,7 @@ struct lowpan_peer {
> struct l2cap_chan *chan;
>
> /* peer addresses in various formats */
> - unsigned char eui64_addr[EUI64_ADDR_LEN];
> + unsigned char lladdr[ETH_ALEN];
> struct in6_addr peer_addr;
> };
>
> @@ -80,8 +80,6 @@ struct lowpan_btle_dev {
> struct delayed_work notify_peers;
> };
>
> -static void set_addr(u8 *eui, u8 *addr, u8 addr_type);
> -
> static inline struct lowpan_btle_dev *
> lowpan_btle_dev(const struct net_device *netdev)
> {
> @@ -277,7 +275,6 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
> const u8 *saddr;
> struct lowpan_btle_dev *dev;
> struct lowpan_peer *peer;
> - unsigned char eui64_daddr[EUI64_ADDR_LEN];
>
> dev = lowpan_btle_dev(netdev);
>
> @@ -287,10 +284,9 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
> if (!peer)
> return -EINVAL;
>
> - saddr = peer->eui64_addr;
> - set_addr(&eui64_daddr[0], chan->src.b, chan->src_type);
> + saddr = peer->lladdr;
>
> - return lowpan_header_decompress(skb, netdev, &eui64_daddr, saddr);
> + return lowpan_header_decompress(skb, netdev, netdev->dev_addr, saddr);

This looks not right, you are sure that netdev->dev_addr is the
_destination_ address? It looks very confusing, because dev_addr is
normally the source address of the netdevice interface.

> }
>
> static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> @@ -477,7 +473,7 @@ static int setup_header(struct sk_buff *skb, struct net_device *netdev,
> }
> }
>
> - daddr = peer->eui64_addr;
> + daddr = peer->lladdr;
> *peer_addr = addr;
> *peer_addr_type = addr_type;
> lowpan_cb(skb)->chan = peer->chan;
> @@ -663,27 +659,6 @@ static struct device_type bt_type = {
> .name = "bluetooth",
> };
>
> -static void set_addr(u8 *eui, u8 *addr, u8 addr_type)
> -{
> - /* addr is the BT address in little-endian format */
> - eui[0] = addr[5];
> - eui[1] = addr[4];
> - eui[2] = addr[3];
> - eui[3] = 0xFF;
> - eui[4] = 0xFE;
> - eui[5] = addr[2];
> - eui[6] = addr[1];
> - eui[7] = addr[0];
> -
> - /* Universal/local bit set, BT 6lowpan draft ch. 3.2.1 */
> - if (addr_type == BDADDR_LE_PUBLIC)
> - eui[0] &= ~0x02;
> - else
> - eui[0] |= 0x02;
> -
> - BT_DBG("type %d addr %*phC", addr_type, 8, eui);
> -}
> -

YAY, this function is totally wrong! :-)

> static void ifup(struct net_device *netdev)
> {
> int err;
> @@ -762,14 +737,16 @@ static struct l2cap_chan *add_peer_chan(struct l2cap_chan *chan,
> peer->chan = chan;
> memset(&peer->peer_addr, 0, sizeof(struct in6_addr));
>
> + baswap((void *)peer->lladdr, &chan->dst);
> +
> /* RFC 2464 ch. 5 */
> peer->peer_addr.s6_addr[0] = 0xFE;
> peer->peer_addr.s6_addr[1] = 0x80;
> - set_addr((u8 *)&peer->peer_addr.s6_addr + 8, chan->dst.b,
> - chan->dst_type);
>
> - memcpy(&peer->eui64_addr, (u8 *)&peer->peer_addr.s6_addr + 8,
> - EUI64_ADDR_LEN);
> + memcpy(&peer->peer_addr.s6_addr[8], peer->lladdr, 3);
> + peer->peer_addr.s6_addr[11] = 0xFF;
> + peer->peer_addr.s6_addr[12] = 0xFE;
> + memcpy(&peer->peer_addr.s6_addr[13], peer->lladdr + 3, 3);
>

What the hell is that? I suppose this is to handling something correct
to a behaviour which is another big thing which is totally broken in the
BTLE 6LoWPAN code.

> /* IPv6 address needs to have the U/L bit set properly so toggle
> * it back here.
>

- Alex

2017-02-15 07:29:32

by Alexander Aring

[permalink] [raw]
Subject: Re: [PATCH 4/5] ipv6: addrconf: fix 48 bit 6lowpan autoconfiguration


Hi,

On 02/09/2017 03:55 PM, Luiz Augusto von Dentz wrote:
> From: Alexander Aring <[email protected]>
>
> This patch adds support for 48 bit 6LoWPAN address length
> autoconfiguration which is the case for BTLE 6LoWPAN.
>
> Signed-off-by: Alexander Aring <[email protected]>
> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
> ---
> net/ipv6/addrconf.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index ac9bd56..dede33f 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -2050,12 +2050,19 @@ static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
> __ipv6_dev_ac_dec(ifp->idev, &addr);
> }
>
> -static int addrconf_ifid_eui64(u8 *eui, struct net_device *dev)
> +static int addrconf_ifid_6lowpan(u8 *eui, struct net_device *dev)
> {
> - if (dev->addr_len != EUI64_ADDR_LEN)
> + switch (dev->addr_len) {
> + case ETH_ALEN:
> + return addrconf_ifid_eui48(eui, dev);
> + case EUI64_ADDR_LEN:
> + memcpy(eui, dev->dev_addr, EUI64_ADDR_LEN);
> + eui[0] ^= 2;
> + break;
> + default:
> return -1;
> - memcpy(eui, dev->dev_addr, EUI64_ADDR_LEN);
> - eui[0] ^= 2;
> + }
> +
> return 0;
> }
>

The question is here to evaluate the link-layer type here... I see
problems coming if another 6LoWPAN adaptation is there and use the same
address length but RFC describes different handling to generate SLAAC.

Nevertheless it should work right now, we can always touch the code
again. :-)

- Alex

2017-02-09 14:55:50

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH 4/5] ipv6: addrconf: fix 48 bit 6lowpan autoconfiguration

From: Alexander Aring <[email protected]>

This patch adds support for 48 bit 6LoWPAN address length
autoconfiguration which is the case for BTLE 6LoWPAN.

Signed-off-by: Alexander Aring <[email protected]>
Signed-off-by: Luiz Augusto von Dentz <[email protected]>
---
net/ipv6/addrconf.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ac9bd56..dede33f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2050,12 +2050,19 @@ static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
__ipv6_dev_ac_dec(ifp->idev, &addr);
}

-static int addrconf_ifid_eui64(u8 *eui, struct net_device *dev)
+static int addrconf_ifid_6lowpan(u8 *eui, struct net_device *dev)
{
- if (dev->addr_len != EUI64_ADDR_LEN)
+ switch (dev->addr_len) {
+ case ETH_ALEN:
+ return addrconf_ifid_eui48(eui, dev);
+ case EUI64_ADDR_LEN:
+ memcpy(eui, dev->dev_addr, EUI64_ADDR_LEN);
+ eui[0] ^= 2;
+ break;
+ default:
return -1;
- memcpy(eui, dev->dev_addr, EUI64_ADDR_LEN);
- eui[0] ^= 2;
+ }
+
return 0;
}

@@ -2146,7 +2153,7 @@ static int ipv6_generate_eui64(u8 *eui, struct net_device *dev)
case ARPHRD_IPGRE:
return addrconf_ifid_gre(eui, dev);
case ARPHRD_6LOWPAN:
- return addrconf_ifid_eui64(eui, dev);
+ return addrconf_ifid_6lowpan(eui, dev);
case ARPHRD_IEEE1394:
return addrconf_ifid_ieee1394(eui, dev);
case ARPHRD_TUNNEL6:
--
2.9.3


2017-02-09 14:55:49

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH 3/5] 6lowpan: iphc: override l2 packet information

From: Alexander Aring <[email protected]>

The skb->pkt_type need to be set by L2, but on 6LoWPAN there exists L2
e.g. BTLE which doesn't has multicast addressing. If it's a multicast or
not is detected by IPHC headers multicast bit. The IPv6 layer will
evaluate this pkt_type, so we force set this type while uncompressing.
Should be okay for 802.15.4 as well.

Signed-off-by: Alexander Aring <[email protected]>
---
net/6lowpan/iphc.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 79f1fa2..fb5f6fa 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -666,6 +666,8 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,

switch (iphc1 & (LOWPAN_IPHC_M | LOWPAN_IPHC_DAC)) {
case LOWPAN_IPHC_M | LOWPAN_IPHC_DAC:
+ skb->pkt_type = PACKET_BROADCAST;
+
spin_lock_bh(&lowpan_dev(dev)->ctx.lock);
ci = lowpan_iphc_ctx_get_by_id(dev, LOWPAN_IPHC_CID_DCI(cid));
if (!ci) {
@@ -681,11 +683,15 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
spin_unlock_bh(&lowpan_dev(dev)->ctx.lock);
break;
case LOWPAN_IPHC_M:
+ skb->pkt_type = PACKET_BROADCAST;
+
/* multicast */
err = lowpan_uncompress_multicast_daddr(skb, &hdr.daddr,
iphc1 & LOWPAN_IPHC_DAM_MASK);
break;
case LOWPAN_IPHC_DAC:
+ skb->pkt_type = PACKET_HOST;
+
spin_lock_bh(&lowpan_dev(dev)->ctx.lock);
ci = lowpan_iphc_ctx_get_by_id(dev, LOWPAN_IPHC_CID_DCI(cid));
if (!ci) {
@@ -701,6 +707,8 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
spin_unlock_bh(&lowpan_dev(dev)->ctx.lock);
break;
default:
+ skb->pkt_type = PACKET_HOST;
+
err = lowpan_iphc_uncompress_addr(skb, dev, &hdr.daddr,
iphc1 & LOWPAN_IPHC_DAM_MASK,
daddr);
--
2.9.3


2017-02-09 14:55:48

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH 2/5] 6lowpan: Set MAC address lenght according to LOWPAN_LLTYPE

From: Patrik Flykt <[email protected]>

Set MAC address length according to the 6LoWPAN link layer in use.
Bluetooth Low Energy uses 48 bit addressing while IEEE802.15.4 uses
64 bits.

Signed-off-by: Patrik Flykt <[email protected]>
---
net/6lowpan/core.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/6lowpan/core.c b/net/6lowpan/core.c
index 5945f7e..5f9909a 100644
--- a/net/6lowpan/core.c
+++ b/net/6lowpan/core.c
@@ -23,7 +23,16 @@ int lowpan_register_netdevice(struct net_device *dev,
{
int i, ret;

- dev->addr_len = EUI64_ADDR_LEN;
+ switch (lltype) {
+ case LOWPAN_LLTYPE_IEEE802154:
+ dev->addr_len = EUI64_ADDR_LEN;
+ break;
+
+ case LOWPAN_LLTYPE_BTLE:
+ dev->addr_len = ETH_ALEN;
+ break;
+ }
+
dev->type = ARPHRD_6LOWPAN;
dev->mtu = IPV6_MIN_MTU;
dev->priv_flags |= IFF_NO_QUEUE;
--
2.9.3


2017-02-09 14:55:47

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH 1/5] bluetooth: Set 6 byte device addresses

From: Patrik Flykt <[email protected]>

Set BTLE MAC addresses that are 6 bytes long and not 8 bytes
that are used in other places with 6lowpan.

Signed-off-by: Patrik Flykt <[email protected]>
Signed-off-by: Luiz Augusto von Dentz <[email protected]>
---
net/bluetooth/6lowpan.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 1904a93..1456b01 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -80,6 +80,8 @@ struct lowpan_btle_dev {
struct delayed_work notify_peers;
};

+static void set_addr(u8 *eui, u8 *addr, u8 addr_type);
+
static inline struct lowpan_btle_dev *
lowpan_btle_dev(const struct net_device *netdev)
{
@@ -272,9 +274,10 @@ static int give_skb_to_upper(struct sk_buff *skb, struct net_device *dev)
static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
struct l2cap_chan *chan)
{
- const u8 *saddr, *daddr;
+ const u8 *saddr;
struct lowpan_btle_dev *dev;
struct lowpan_peer *peer;
+ unsigned char eui64_daddr[EUI64_ADDR_LEN];

dev = lowpan_btle_dev(netdev);

@@ -285,9 +288,9 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
return -EINVAL;

saddr = peer->eui64_addr;
- daddr = dev->netdev->dev_addr;
+ set_addr(&eui64_daddr[0], chan->src.b, chan->src_type);

- return lowpan_header_decompress(skb, netdev, daddr, saddr);
+ return lowpan_header_decompress(skb, netdev, &eui64_daddr, saddr);
}

static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
@@ -681,13 +684,6 @@ static void set_addr(u8 *eui, u8 *addr, u8 addr_type)
BT_DBG("type %d addr %*phC", addr_type, 8, eui);
}

-static void set_dev_addr(struct net_device *netdev, bdaddr_t *addr,
- u8 addr_type)
-{
- netdev->addr_assign_type = NET_ADDR_PERM;
- set_addr(netdev->dev_addr, addr->b, addr_type);
-}
-
static void ifup(struct net_device *netdev)
{
int err;
@@ -803,7 +799,8 @@ static int setup_netdev(struct l2cap_chan *chan, struct lowpan_btle_dev **dev)
if (!netdev)
return -ENOMEM;

- set_dev_addr(netdev, &chan->src, chan->src_type);
+ netdev->addr_assign_type = NET_ADDR_PERM;
+ baswap((void *)netdev->dev_addr, &chan->src);

netdev->netdev_ops = &netdev_ops;
SET_NETDEV_DEV(netdev, &chan->conn->hcon->hdev->dev);
--
2.9.3


2017-02-09 14:55:51

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH 5/5] 6lowpan: Use netdev addr_len to determine lladdr len

From: Luiz Augusto von Dentz <[email protected]>

This allow technologies such as Bluetooth to use its native lladdr which
is eui48 instead of eui64 which was expected by functions like
lowpan_header_decompress and lowpan_header_compress.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
---
net/6lowpan/iphc.c | 17 +++++++++++++++--
net/bluetooth/6lowpan.c | 43 ++++++++++---------------------------------
2 files changed, 25 insertions(+), 35 deletions(-)

diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index fb5f6fa..ee88feb 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -827,8 +827,21 @@ static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev,
}
break;
default:
- /* check for SAM/DAM = 11 */
- memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
+ switch (dev->addr_len) {
+ case ETH_ALEN:
+ memcpy(&tmp.s6_addr[8], lladdr, 3);
+ tmp.s6_addr[11] = 0xFF;
+ tmp.s6_addr[12] = 0xFE;
+ memcpy(&tmp.s6_addr[13], lladdr + 3, 3);
+ break;
+ case EUI64_ADDR_LEN:
+ memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN);
+ break;
+ default:
+ dam = LOWPAN_IPHC_DAM_11;
+ goto out;
+ }
+
/* second bit-flip (Universe/Local) is done according RFC2464 */
tmp.s6_addr[8] ^= 0x02;
/* context information are always used */
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 1456b01..d2a1aa5 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -64,7 +64,7 @@ struct lowpan_peer {
struct l2cap_chan *chan;

/* peer addresses in various formats */
- unsigned char eui64_addr[EUI64_ADDR_LEN];
+ unsigned char lladdr[ETH_ALEN];
struct in6_addr peer_addr;
};

@@ -80,8 +80,6 @@ struct lowpan_btle_dev {
struct delayed_work notify_peers;
};

-static void set_addr(u8 *eui, u8 *addr, u8 addr_type);
-
static inline struct lowpan_btle_dev *
lowpan_btle_dev(const struct net_device *netdev)
{
@@ -277,7 +275,6 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
const u8 *saddr;
struct lowpan_btle_dev *dev;
struct lowpan_peer *peer;
- unsigned char eui64_daddr[EUI64_ADDR_LEN];

dev = lowpan_btle_dev(netdev);

@@ -287,10 +284,9 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
if (!peer)
return -EINVAL;

- saddr = peer->eui64_addr;
- set_addr(&eui64_daddr[0], chan->src.b, chan->src_type);
+ saddr = peer->lladdr;

- return lowpan_header_decompress(skb, netdev, &eui64_daddr, saddr);
+ return lowpan_header_decompress(skb, netdev, netdev->dev_addr, saddr);
}

static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
@@ -477,7 +473,7 @@ static int setup_header(struct sk_buff *skb, struct net_device *netdev,
}
}

- daddr = peer->eui64_addr;
+ daddr = peer->lladdr;
*peer_addr = addr;
*peer_addr_type = addr_type;
lowpan_cb(skb)->chan = peer->chan;
@@ -663,27 +659,6 @@ static struct device_type bt_type = {
.name = "bluetooth",
};

-static void set_addr(u8 *eui, u8 *addr, u8 addr_type)
-{
- /* addr is the BT address in little-endian format */
- eui[0] = addr[5];
- eui[1] = addr[4];
- eui[2] = addr[3];
- eui[3] = 0xFF;
- eui[4] = 0xFE;
- eui[5] = addr[2];
- eui[6] = addr[1];
- eui[7] = addr[0];
-
- /* Universal/local bit set, BT 6lowpan draft ch. 3.2.1 */
- if (addr_type == BDADDR_LE_PUBLIC)
- eui[0] &= ~0x02;
- else
- eui[0] |= 0x02;
-
- BT_DBG("type %d addr %*phC", addr_type, 8, eui);
-}
-
static void ifup(struct net_device *netdev)
{
int err;
@@ -762,14 +737,16 @@ static struct l2cap_chan *add_peer_chan(struct l2cap_chan *chan,
peer->chan = chan;
memset(&peer->peer_addr, 0, sizeof(struct in6_addr));

+ baswap((void *)peer->lladdr, &chan->dst);
+
/* RFC 2464 ch. 5 */
peer->peer_addr.s6_addr[0] = 0xFE;
peer->peer_addr.s6_addr[1] = 0x80;
- set_addr((u8 *)&peer->peer_addr.s6_addr + 8, chan->dst.b,
- chan->dst_type);

- memcpy(&peer->eui64_addr, (u8 *)&peer->peer_addr.s6_addr + 8,
- EUI64_ADDR_LEN);
+ memcpy(&peer->peer_addr.s6_addr[8], peer->lladdr, 3);
+ peer->peer_addr.s6_addr[11] = 0xFF;
+ peer->peer_addr.s6_addr[12] = 0xFE;
+ memcpy(&peer->peer_addr.s6_addr[13], peer->lladdr + 3, 3);

/* IPv6 address needs to have the U/L bit set properly so toggle
* it back here.
--
2.9.3