search

RIOT-OS / RIOT

RIOT - The friendly OS for IoT
https://riot-os.org
GNU Lesser General Public License v2.1
4.89k stars 1.98k forks source link

netdev_ieee802154: Mismatch between radio ll address and in memory address #10380

Open bergzand opened 5 years ago

bergzand commented 5 years ago

Description

Most, if not all, radios adjust the link layer address to force it to unicast. However, the netdev_ieee802154 doesn't adjust the address, causing a mismatch between what gnrc thinks the link layer address is and what the radio has configured as a link layer address.

Steps to reproduce the issue

On examples/gnrc_networking on a samr21-xpro use the shell to set the long address to something non-unicast such as FF:F0:FF:F1:FF:F2:FF:F3.

Expected results

The address is set to and displayed as 7F:F0:FF:F1:FF:F2:FF:F3, to mark it as unicast

Actual results

ifconfig shows the address as FF:F0:FF:F1:FF:F2:FF:F3.

Versions

Operating System Environment
-----------------------------
       Operating System: Gentoo
                 Kernel: Linux 4.14.65-gentoo x86_64 Intel(R) Core(TM) i5-6200U CPU @ 2.30GHz

Installed compiler toolchains
-----------------------------
             native gcc: gcc (Gentoo 7.3.0-r3 p1.4) 7.3.0
      arm-none-eabi-gcc: arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2017-q4-major) 7.2.1 20170904 (release) [ARM/embedded-7-branch revision 255204]
                avr-gcc: avr-gcc (Gentoo 7.3.0-r3 p1.4) 7.3.0
       mips-mti-elf-gcc: missing
             msp430-gcc: missing
   riscv-none-embed-gcc: missing
                  clang: clang version 6.0.1 (tags/RELEASE_601/final)

Installed compiler libs
-----------------------
   arm-none-eabi-newlib: "2.5.0"
    mips-mti-elf-newlib: missing
riscv-none-embed-newlib: missing
               avr-libc: "2.0.0" ("20150208")

Installed development tools
---------------------------
                  cmake: cmake version 3.9.6
               cppcheck: Cppcheck 1.84
                doxygen: 1.8.14
                 flake8: 3.5.0 (mccabe: 0.6.1, pycodestyle: 2.3.1, pyflakes: 1.6.0) CPython 3.6.5 on Linux
                    git: git version 2.16.4
                openocd: Open On-Chip Debugger 0.10.0
                 python: Python 3.5.5
                python2: Python 2.7.15
                python3: Python 3.5.5
             coccinelle: missing
bergzand commented 5 years ago

On a second look, I think this problem only occurs with the short address, radios clear the first bit before setting it, to mark it as unicast. The netdev_ieee802154 layer doesn't clear this bit.

jnohlgard commented 5 years ago

Maybe it would be better to return an error if we try to netdev::set a broadcast address as the link layer address?

bergzand commented 5 years ago

After looking through the driver code, I see multiple issues with the ieee802.15.4 short address handling. The generic structure at the moment is that the set_short_addr handler of every radio sets both the netdev::short_addr and the register of radio. The handler doesn't set a return code, causing the generic netdev_ieee802154_set to also handle the set call, setting the netdev::short_addr a second time, without clearing the broadcast bit. The cc2538 is the only exception here, that one doesn't set the netdev::short_addr member.

What is happening:

Not clearing any bit:

The cc2538 doesn't clear the broadcast bit, same for socket_zep.

Only clearing the bit for the netdev::short_addr:

Some radios only clear the broadcast bit in the netdev::short_addr member, but not the value that is send to the radio. This results in somewhat correct behaviour where the netdev::short_addr remains in sync with the value in the radio.

Clearing the bit for both the register and the netdev::short_addr:

This results in a desync between the radio and the netdev struct due to the second write to the netdev::short_addr member.

bergzand commented 5 years ago

Maybe it would be better to return an error if we try to netdev::set a broadcast address as the link layer address?

For now I propose to at least ensure that the handling of the address is identical between the radios. In the future, having a sanity check in the set call somewhere is IMHO preferable

jnohlgard commented 5 years ago

@bergzand do you plan on writing a fix for this issue?

bergzand commented 5 years ago

@bergzand do you plan on writing a fix for this issue?

I kinda got stuck looking for a nice way to solve this issue.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

miri64 commented 4 years ago

This should be an easy fix, right?