Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nib.c: add interface selection rules for static link local address assignment #20784

Conversation

DanielLockau-MLPA
Copy link
Contributor

@DanielLockau-MLPA DanielLockau-MLPA commented Jul 15, 2024

UPDATED ON: 2024-08-14, 2024-08-15

Contribution description

Setting static link local addresses is supported in nib.c.
This contribution adds options for applying static link local addresses only on specific interfaces.
Setting the static link local address can be selected by either netdev driver type or by interface number.

Testing procedure

Compile examples/gnrc_networking for board native, uncommenting the Makefile lines

IPV6_STATIC_LLADDR ?= '"fe80::cafe:cafe:cafe:1"'
CFLAGS += -DCONFIG_GNRC_IPV6_STATIC_LLADDR=$(IPV6_STATIC_LLADDR)

A static link local address will show up in ifconfig:

Iface  6  HWaddr: 56:CB:2C:67:4D:F3
          ...
          inet6 addr: fe80::cafe:cafe:cafe:7  scope: link  VAL
          ...

You can now play with the include options. Either

CFLAGS += -DCONFIG_GNRC_IPV6_STATIC_LLADDR_NETDEV_MASK="(1ULL << NETDEV_TAP)"

or

CFLAGS += -DCONFIG_GNRC_IPV6_STATIC_LLADDR_NETDEV_MASK="(1ULL << NETDEV_ANY)"

should leave the netif configuration unaltered, with the static link local address still showing up.

Other values for any of the config variable should make the link local address disappear.

Issues/PRs references

None.

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jul 15, 2024
@benpicco benpicco requested a review from fabian18 July 15, 2024 14:17
@benpicco
Copy link
Contributor

Maybe it's just me, but this inverse logic always leads to mental contortions.
Why not make it CONFIG_GNRC_IPV6_STATIC_LLADDR_NETDEV_MASK so the mask only tells which netdevs it applies to.

If it's not set, the static address would still apply on all interfaces.

@fabian18
Copy link
Contributor

fabian18 commented Aug 9, 2024

+1 for CONFIG_GNRC_IPV6_STATIC_LLADDR_NETDEV_MASK to stay compatible with the current implementation and in favor of less confusion, compared to CONFIG_GNRC_IPV6_STATIC_LLADDR_EXCLUDE_IFACE_MASK.

Was it really intended back then, to have a static address for all interfaces?
The most flexible solution IMO is to map netdev ID and index to a static address.
It takes probably more ROM to configure this but everyone would probably know what to expect from it.
... maybe as an alternative for CONFIG_GNRC_IPV6_STATIC_LLADDR.

But to solve the issue, a quick CONFIG_GNRC_IPV6_STATIC_LLADDR_NETDEV_MASK would probably be better.

@benpicco
Copy link
Contributor

benpicco commented Aug 9, 2024

The most flexible solution IMO is to map netdev ID and index to a static address.

The problem with this approach is that netdev IDs are not stable and will shift around as modules (threads) get added, so using netdev IDs is always very brittle.

What works better is netdev type + index, but that's a bit more complicated code/configuration wise, so using the same link-local address for all interfaces was simply the easiest solution.

@DanielLockau-MLPA
Copy link
Contributor Author

@benpicco @fabian18 Should I drop the second variant of selecting by interface ID altogether or also make it a positive selector?

@DanielLockau-MLPA DanielLockau-MLPA force-pushed the dl/riot/20240715__nib__static_ll_addr_excludes branch from 74c7498 to 9ce1aa1 Compare August 14, 2024 12:42
@DanielLockau-MLPA
Copy link
Contributor Author

@benpicco I just updated this PR with a positive selection.

@DanielLockau-MLPA DanielLockau-MLPA changed the title nib.c: add exclude rules for static link local addresses nib.c: add interface selection rules for static link local address assignment Aug 14, 2024
sys/net/gnrc/network_layer/ipv6/nib/nib.c Outdated Show resolved Hide resolved
sys/net/gnrc/network_layer/ipv6/nib/nib.c Outdated Show resolved Hide resolved
sys/net/gnrc/network_layer/ipv6/nib/nib.c Outdated Show resolved Hide resolved
sys/net/gnrc/network_layer/ipv6/nib/nib.c Outdated Show resolved Hide resolved
@fabian18
Copy link
Contributor

@benpicco @fabian18 Should I drop the second variant of selecting by interface ID altogether or also make it a positive selector?

I would not miss it. It is a potential foot gun, if for example in an application you disable one kind of interfaces but don't change this configuration and after that another interface unexpectedly has that address or the PID no longer exists.

@fabian18
Copy link
Contributor

Looks good from my perspective.

@benpicco
Copy link
Contributor

Please squash!

@DanielLockau-MLPA DanielLockau-MLPA force-pushed the dl/riot/20240715__nib__static_ll_addr_excludes branch from 6efe112 to 7240d37 Compare August 26, 2024 06:31
@DanielLockau-MLPA
Copy link
Contributor Author

Please squash!

done

@benpicco benpicco enabled auto-merge August 26, 2024 08:51
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 26, 2024
@riot-ci
Copy link

riot-ci commented Aug 26, 2024

Murdock results

✔️ PASSED

7240d37 nib.c: allow selection of interfaces for static link local addresses

Success Failures Total Runtime
10179 0 10180 14m:16s

Artifacts

@benpicco benpicco added this pull request to the merge queue Aug 26, 2024
Merged via the queue into RIOT-OS:master with commit 73581fa Aug 26, 2024
26 checks passed
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
@benpicco benpicco deleted the dl/riot/20240715__nib__static_ll_addr_excludes branch December 17, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants