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

gnrc/ipv6_auto_subnets: init RPL root when adding a prefix #17350

Merged
merged 4 commits into from
May 2, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Dec 7, 2021

Contribution description

Currently there is no way to initialize a RPL root from the shell in the same way as with automatic prefix configuration.
This changes the nib prefix add command to use the same mechanism as uhcpc and dhcpv6 when adding a new prefix.

I also added this to the ipv6_auto_subnets as this was missing there before.

To avoid conflicts with heterogeneous downstream networks (wired and wireless downstream) I added a check to only initialize RPL on a wireless interface.

Testing procedure

Issues/PRs references

@benpicco benpicco requested a review from miri64 December 7, 2021 14:08
@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Dec 7, 2021
@benpicco benpicco added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Dec 7, 2021
@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 Dec 9, 2021
@benpicco benpicco requested a review from JKRhb January 4, 2022 12:36
Copy link
Member

@JKRhb JKRhb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the PR using you instructions and adding the prefix works fine :) I noticed, though, that removing the prefix with nib prefix del 7 2001:db8::/32 does not update the RPL root/instance table while the ifconfig gets properly updated. Is this something that should also be considered in this PR?

@miri64
Copy link
Member

miri64 commented Jan 4, 2022

I noticed, though, that removing the prefix with nib prefix del 7 2001:db8::/32 does not update the RPL root/instance table while the ifconfig gets properly updated. Is this something that should also be considered in this PR?

Does removing the prefix remove the address, too? If so, it should be updated as it would be when removing the address (best via the removal of the address), if not, the prefix list should be considered independent of the address list and the behavior is correct. Only question then is how RPL's PIO handles that situation.

@JKRhb
Copy link
Member

JKRhb commented Jan 4, 2022

Does removing the prefix remove the address, too?

The current behavior can be seen below: The address gets removed from the netif, but the instance table does not change. Is this correct or should the latter be updated as well?

> nib prefix add 7 2001:db8::/32
nib prefix add 7 2001:db8::/32
> rpl
rpl
instance table: [X]
parent table:   [ ]     [ ]     [ ]

instance [0 | Iface: 7 | mop: 2 | ocp: 0 | mhri: 256 | mri 0]
        dodag [2001:db8::e8ac:916:53b0:3ada | R: 256 | OP: Router | PIO: on | TR(I=[8,20], k=10, c=0)]
> ifconfig
ifconfig
Iface  7  HWaddr: 3A:DA  Channel: 26  NID: 0x23 
          Long HWaddr: EA:AC:09:16:53:B0:3A:DA 
          L2-PDU:102  MTU:1280  HL:64  RTR  
          6LO  IPHC  
          Source address length: 8
          Link type: wireless
          inet6 addr: fe80::e8ac:916:53b0:3ada  scope: link  VAL
          inet6 addr: 2001:db8::e8ac:916:53b0:3ada  scope: global  VAL
          inet6 group: ff02::2
          inet6 group: ff02::1
          inet6 group: ff02::1:ffb0:3ada
          inet6 group: ff02::1a
          
          Statistics for Layer 2
            RX packets 0  bytes 0
            TX packets 14 (Multicast: 14)  bytes 461
            TX succeeded 14 errors 0
          Statistics for IPv6
            RX packets 0  bytes 0
            TX packets 14 (Multicast: 14)  bytes 1208
            TX succeeded 14 errors 0

> nib prefix del 7 2001:db8::/32
nib prefix del 7 2001:db8::/32
> rpl
rpl
instance table: [X]
parent table:   [ ]     [ ]     [ ]

instance [0 | Iface: 7 | mop: 2 | ocp: 0 | mhri: 256 | mri 0]
        dodag [2001:db8::e8ac:916:53b0:3ada | R: 256 | OP: Router | PIO: on | TR(I=[8,20], k=10, c=0)]
> ifconfig
ifconfig
Iface  7  HWaddr: 3A:DA  Channel: 26  NID: 0x23 
          Long HWaddr: EA:AC:09:16:53:B0:3A:DA 
          L2-PDU:102  MTU:1280  HL:64  RTR  
          6LO  IPHC  
          Source address length: 8
          Link type: wireless
          inet6 addr: fe80::e8ac:916:53b0:3ada  scope: link  VAL
          inet6 group: ff02::2
          inet6 group: ff02::1
          inet6 group: ff02::1:ffb0:3ada
          inet6 group: ff02::1a
          
          Statistics for Layer 2
            RX packets 0  bytes 0
            TX packets 17 (Multicast: 17)  bytes 547
            TX succeeded 18 errors 0
          Statistics for IPv6
            RX packets 0  bytes 0
            TX packets 17 (Multicast: 17)  bytes 1436
            TX succeeded 17 errors 0

@miri64
Copy link
Member

miri64 commented Jan 5, 2022

But using ifconfig 7 del 2001:db8::e8ac:916:53b0:3ada is changing the instance ID? That was what I was asking previously, I guess...

@miri64
Copy link
Member

miri64 commented Jan 5, 2022

If not, than this is faulty behaviour, which is however unrelated to this PR as this would most likely also be the case in current master.

@benpicco
Copy link
Contributor Author

benpicco commented Jan 6, 2022

So what should I do here - also update nib prefix del, or is this out of the scope of this PR?

@miri64
Copy link
Member

miri64 commented Jan 6, 2022

So what should I do here - also update nib prefix del, or is this out of the scope of this PR?

As already said before: I would prefer to rather hook this up with the address removal rather than the prefix removal (which in turn calls address removal anyway), as the RPL network is defined by its DODAG ID (of type IPv6 address, not IPv6 prefix). That said, since I imagine that this would also require some clean-up on the RPL handler's part, I think it is best to do this in a separate PR.

@miri64
Copy link
Member

miri64 commented Jan 6, 2022

As already said before: I would prefer to rather hook this up with the address removal rather than the prefix removal (which in turn calls address removal anyway), as the RPL network is defined by its DODAG ID (of type IPv6 address, not IPv6 prefix).

(this also makes the current behavior somewhat of a bug btw, since I don't think it is defined how a RPL network should operate with its DODAG ID address not assigned to an interface of the root)

@JKRhb
Copy link
Member

JKRhb commented Jan 7, 2022

I guess we could merge this PR then, right? (And deal with the prefix/address removal separately.)

@miri64
Copy link
Member

miri64 commented Jan 7, 2022

Please try to use the addresses getter provided by NETAPI or the gnrc_netif_ipv6_addrs_get() helper. This way, this operation can be made more thread-safe.

@benpicco
Copy link
Contributor Author

benpicco commented Jan 7, 2022

I don't follow - how is it any more thread safe to do

ipv6_addr_t addrs[CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF];

idx = gnrc_netif_ipv6_add_prefix(netif, &pfx, pfx_len, valid_ltime, pref_ltime);
gnrc_netif_ipv6_addrs_get(netif, addrs, CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF);
gnrc_rpl_configure_root(netif, &addrs[idx]);

How would an address that was just added be subject to change anyway? (And then we would actually want the current version of the address for the RPL root)

If the address changes gnrc_netif_get_by_ipv6_addr() in gnrc_rpl_root_instance_init() would fail to begin with, so copying the address here will not save us from such edge case.

@benpicco
Copy link
Contributor Author

ping @miri64 😉
That use case was actually the reason why gnrc_netif_ipv6_add_prefix() returns the index to the address array.
I don't see how copying the whole address array to the stack buys us anything here, if the address changes the rpl root would then be wrong.

@miri64
Copy link
Member

miri64 commented Jan 20, 2022

ping @miri64 wink
That use case was actually the reason why gnrc_netif_ipv6_add_prefix() returns the index to the address array.
I don't see how copying the whole address array to the stack buys us anything here, if the address changes the rpl root would then be wrong.

Fine, but then put at least a gnrc_netif_acquire()/gnrc_netif_release() around from before when you add a prefix to after when the address is added to RPL.

@benpicco
Copy link
Contributor Author

benpicco commented Jan 20, 2022

Ok, added - I had to slightly modify gnrc_netif_ipv6_add_prefix() to avoid creating a dead-lock with the new locking.

@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 20, 2022
@benpicco
Copy link
Contributor Author

ping @miri64 😉

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Feb 25, 2022
Comment on lines 426 to 429
/* only run RPL on wireless interfaces */
if (gnrc_netapi_get(netif->pid, NETOPT_IS_WIRED, 0, NULL, 0) == 1) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh... I know defacto it doesn't make sense to run RPL over a wired network, but e.g. for virtualization this might still be important (e.g. many testings of RPL were done with DESVirt on native' TAP if I remember correctly). Does it maybe make more sense to make this dependent on other factors e.g. if a the given OFs can be calculated?

Copy link
Contributor Author

@benpicco benpicco Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

many testings of RPL were done with DESVirt on native TAP if I remember correctly

Doesn't it make much more sense to use ZEP for that?
Actually this whole PR only exists to enable testing of RPL on CI using ZEP - #17353

Does it maybe make more sense to make this dependent on other factors e.g. if a the given OFs can be calculated?

But OF0 can always be calculated.

If we want a TAP interface to simulate a wireless link, we should rather unset the NETOPT_IS_WIRED property there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this whole PR only exists to enable testing of RPL on CI using ZEP - #17353

I am not really clear, how this would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that adding a prefix via nib prefix add should behave the same as adding a prefix via router advertisement or adding a prefix via UHCP / DHCPv6.

Those will also init RPL on those interfaces, but when adding this I thought it would make sense no to do so for wired interfaces.

I can drop that commit if you prefer that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that adding a prefix via nib prefix add should behave the same as adding a prefix via router advertisement or adding a prefix via UHCP / DHCPv6.

Those will also init RPL on those interfaces, but when adding this I thought it would make sense no to do so for wired interfaces.

Mh maybe then rather nib prefix add and rpl root should remain separate commands and not everything should be automated ;-).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is now #17709

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no experience with RPL, but quickly searching for wireless in RFC65550 shows to me that it is not strictly restricted to wireless interfaces.

1.2. Expectations of Link-Layer Type

In compliance with the layered architecture of IP, RPL does not rely
on any particular features of a specific link-layer technology. RPL
is designed to be able to operate over a variety of different link
layers, including ones that are constrained, potentially lossy, or
typically utilized in conjunction with highly constrained host or
router devices, such as but not limited to, low-power wireless or PLC
(Power Line Communication) technologies.

@miri64
Copy link
Member

miri64 commented Feb 25, 2022

The more I think of it and with the discussion unfolding in #17350 (review), I am getting more and more the feeling, that nib prefix add and rpl root (or their respective functions) should remain separate. Bootstrapping is nice, but it leads to unforeseen side-effects, I see more harm than good.

@benpicco
Copy link
Contributor Author

benpicco commented Feb 25, 2022

I still would like to have fd660ec in - I think I just forgot that in the initial PR and it should behave the same as UHCP and DHCPv6.
But then I would also want 8ec0d7b because having a wired and a wireless downstream interface does not seem so unlikely.

I can drop 19c675e.
It would still be nice to have a more high-level command for adding a prefix as right now one has to

  • nib prefix add the prefix
  • ifconfig add an address based on the prefix
  • rpl init

This is easy to get wrong.

@miri64
Copy link
Member

miri64 commented Feb 25, 2022

It would still be nice to have a more high-level command for adding a prefix as right now one has to

I am not against that. I just think there might be harm in having too much bootstrapping in nib commands.

@github-actions github-actions bot removed the Area: tests Area: tests and testing framework label Feb 25, 2022
@benpicco benpicco changed the title gnrc/ipv6_auto_subnets, gnrc_ipv6_nib_nc: init RPL root when adding a prefix gnrc/ipv6_auto_subnets: init RPL root when adding a prefix Feb 25, 2022
@benpicco benpicco requested a review from fabian18 March 23, 2022 12:28
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are very small and look similar as in DHCPv6 and UHCPC, so I think they make sense. But I have no experience with RPL, or ipv6_auto_subnets so far.

Comment on lines 426 to 429
/* only run RPL on wireless interfaces */
if (gnrc_netapi_get(netif->pid, NETOPT_IS_WIRED, 0, NULL, 0) == 1) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no experience with RPL, but quickly searching for wireless in RFC65550 shows to me that it is not strictly restricted to wireless interfaces.

1.2. Expectations of Link-Layer Type

In compliance with the layered architecture of IP, RPL does not rely
on any particular features of a specific link-layer technology. RPL
is designed to be able to operate over a variety of different link
layers, including ones that are constrained, potentially lossy, or
typically utilized in conjunction with highly constrained host or
router devices, such as but not limited to, low-power wireless or PLC
(Power Line Communication) technologies.

@benpicco
Copy link
Contributor Author

I guess if RPL works across multiple interfaces that would make sense?
But our implementation only uses a single interface, so replaying packets over that would not make sense as with all wired interfaces, all nodes connected to the wire should reach all messages, unlike with wireless interfaces.

@benpicco benpicco force-pushed the gnrc_rpl/auto_init_on_prefix branch from 4771bd3 to 2add3dd Compare April 6, 2022 13:39
@benpicco
Copy link
Contributor Author

Would it make sense to only enable RPL for 6Lo interfaces instead?

@miri64
Copy link
Member

miri64 commented Apr 17, 2022

Would it make sense to only enable RPL for 6Lo interfaces instead?

No. RPL is not bound to 6LoWPAN, but can be used with any mesh network technology.

@benpicco
Copy link
Contributor Author

Fine, then let's just drop 8ec0d7b

@benpicco benpicco force-pushed the gnrc_rpl/auto_init_on_prefix branch from 2add3dd to 5785856 Compare April 29, 2022 20:56
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call sequence is as in dhcpv6 and uhcpc and initialization works.

> 2022-05-02 19:10:07,389 # ifconfig
2022-05-02 19:10:07,395 # Iface  6  HWaddr: 00:00  Channel: 26  Page: 0  NID: 0x23  PHY: O-QPSK 
2022-05-02 19:10:07,396 #           
2022-05-02 19:10:07,401 #           Long HWaddr: 4E:49:6F:54:4F:76:00:00 
2022-05-02 19:10:07,407 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2022-05-02 19:10:07,414 #           AUTOACK  ACK_REQ  CSMA  L2-PDU:102  MTU:1280  HL:64  RTR  
2022-05-02 19:10:07,417 #           RTR_ADV  6LO  IPHC  
2022-05-02 19:10:07,420 #           Source address length: 8
2022-05-02 19:10:07,422 #           Link type: wireless
2022-05-02 19:10:07,428 #           inet6 addr: fe80::4c49:6f54:4f76:0  scope: link  VAL
2022-05-02 19:10:07,434 #           inet6 addr: fd00:a:a:8000:4c49:6f54:4f76:0  scope: global  VAL
2022-05-02 19:10:07,437 #           inet6 group: ff02::2
2022-05-02 19:10:07,439 #           inet6 group: ff02::1
2022-05-02 19:10:07,443 #           inet6 group: ff02::1:ff76:0
2022-05-02 19:10:07,446 #           inet6 group: ff02::1a
2022-05-02 19:10:07,447 #           
2022-05-02 19:10:07,451 # Iface  5  HWaddr: 4E:49:6F:54:00:00  Link: up 
2022-05-02 19:10:07,459 #           L2-PDU:1500  MTU:1500  HL:64  RTR  
2022-05-02 19:10:07,461 #           Source address length: 6
2022-05-02 19:10:07,463 #           Link type: wired
2022-05-02 19:10:07,466 #           inet6 addr: fe80::4c49:6fff:fe54:0  scope: link  VAL
2022-05-02 19:10:07,473 #           inet6 addr: fd00:a:a:0:4c49:6fff:fe54:0  scope: global  VAL
2022-05-02 19:10:07,475 #           inet6 group: ff02::2
2022-05-02 19:10:07,477 #           inet6 group: ff02::1
2022-05-02 19:10:07,481 #           inet6 group: ff02::1:ff54:0
2022-05-02 19:10:07,482 #           
> rpl
2022-05-02 19:10:11,976 # rpl
2022-05-02 19:10:11,977 # instance table:       [X]
2022-05-02 19:10:11,980 # parent table: [ ]     [ ]     [ ]
2022-05-02 19:10:11,980 # 
2022-05-02 19:10:11,986 # instance [0 | Iface: 6 | mop: 2 | ocp: 0 | mhri: 256 | mri 0]
2022-05-02 19:10:11,994 #       dodag [fd00:a:a:8000:4c49:6f54:4f76:0 | R: 256 | OP: Router | PIO: on | TR(I=[8,20], k=10, c=0)]

ACK.

@benpicco
Copy link
Contributor Author

benpicco commented May 2, 2022

Thank you for the review!

@benpicco benpicco merged commit 1b92fb9 into RIOT-OS:master May 2, 2022
@benpicco benpicco deleted the gnrc_rpl/auto_init_on_prefix branch May 2, 2022 19:38
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants