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_nib: don't autoconfig IPv6 address without L2 addr #10483

Merged

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Nov 27, 2018

Contribution description

If the interface's link-layer doesn't use link-layer addresses it obviously doesn't make sense to auto-configure an IPv6 address from it. Moreover, I think the address fe80:: is actual illegal, but I couldn't find any references for it.

Testing procedure

Try running an application with slipdev. The interfaces should not have an auto-configured IPv6 address with this PR. Without this PR an address fe80:: (and the respective solicited nodes multicast address) show up.

Issues/PRs references

Unrelated but found when debugging #10480.

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Nov 27, 2018
@miri64
Copy link
Member Author

miri64 commented Dec 17, 2018

Ping? Maybe @gebart also could review, as he tested with #10480.

@miri64 miri64 added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Dec 17, 2018
Copy link
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

Change makes sense. Will test on actual hardware

@miri64
Copy link
Member Author

miri64 commented Jan 18, 2019

@gebart did you?

@miri64 miri64 requested review from kb2ma and removed request for cladmi March 26, 2019 16:10
@miri64
Copy link
Member Author

miri64 commented Mar 27, 2019

@kb2ma might you have time to have a look into this?

@kb2ma
Copy link
Member

kb2ma commented Apr 5, 2019

Tested on samr21-xpro. On current master I see the output below from ifconfig. When using this PR, the fe80:: line goes away as expected. I can still send and receive a CoAP message across the tunslip interface.

2019-04-05 06:29:05,297 - INFO # Iface  8 
2019-04-05 06:29:05,300 - INFO #           MTU:65535  HL:64  RTR  
2019-04-05 06:29:05,302 - INFO #           RTR_ADV  
2019-04-05 06:29:05,305 - INFO #           Link type: wired
2019-04-05 06:29:05,309 - INFO #           inet6 addr: fe80::  scope: local  VAL
2019-04-05 06:29:05,312 - INFO #           inet6 group: ff02::2
2019-04-05 06:29:05,315 - INFO #           inet6 group: ff02::1
2019-04-05 06:29:05,318 - INFO #           inet6 group: ff02::1:ff00:0

Please rebase. I'll retest and confirm I see the debug message in the code.

@miri64
Copy link
Member Author

miri64 commented Apr 5, 2019

Please rebase. I'll retest and confirm I see the debug message in the code.

Which debug message? I'm not quite sure why I need to rebase.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 5, 2019
@kb2ma
Copy link
Member

kb2ma commented Apr 5, 2019

I asked for a rebase because the PR is 4 months old. I don't know if any updated code in other parts of the network stack might impact the functionality in this PR. At any rate, not necessary now. I rebased myself for testing.

This PR still tests fine. My only question is when DEBUG is on in this module. I see the sequence of messages below. Is it confusing to see both line 2 and line 3, which was added by this PR? I don't know enough about debugging the network layer to give an opinion.

INFO # nib: add address based on fe80::/64 automatically to interface 7
INFO # nib: add address based on fe80::/64 automatically to interface 8
INFO # nib: interface 8 has no link-layer addresses

@kb2ma kb2ma added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Apr 5, 2019
@miri64
Copy link
Member Author

miri64 commented Apr 5, 2019

I asked for a rebase because the PR is 4 months old. I don't know if any updated code in other parts of the network stack might impact the functionality in this PR. At any rate, not necessary now. I rebased myself for testing.

Since murdock is doing this as well before compiling, I guess this isn't necessary at all ;-).

This PR still tests fine. My only question is when DEBUG is on in this module. I see the sequence of messages below. Is it confusing to see both line 2 and line 3, which was added by this PR? I don't know enough about debugging the network layer to give an opinion.

INFO # nib: add address based on fe80::/64 automatically to interface 7
INFO # nib: add address based on fe80::/64 automatically to interface 8
INFO # nib: interface 8 has no link-layer addresses

Ah, will move the check above the first DEBUG. This way it is less confusing, I hope.

@miri64
Copy link
Member Author

miri64 commented Apr 5, 2019

Ah, will move the check above the first DEBUG. This way it is less confusing, I hope.

Done.

Copy link
Member

@kb2ma kb2ma left a comment

Choose a reason for hiding this comment

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

Apologies for the delay, @miri64. I missed your fixup message.

Now I see the debug messages below. This definitely is clearer. ACK, please squash.

2019-04-18 21:09:08,535 - INFO #  nib: add address based on fe80::/64 automatically to interface 7
2019-04-18 21:09:08,539 - INFO # nib: interface 8 has no link-layer addresses

If the interface's link-layer doesn't use link-layer addresses it
obviously doesn't make sense to auto-configure an IPv6 address from it.
Moreover, I think the address `fe80::` is actual illegal, but I
couldn't find any references for it.
@miri64 miri64 force-pushed the gnrc_ipv6_nib/fix/no-auto-config-wo-l2addr branch from 2ebe8e9 to 5732e9b Compare April 19, 2019 06:36
@miri64
Copy link
Member Author

miri64 commented Apr 19, 2019

Squashed

@miri64 miri64 merged commit 9a64731 into RIOT-OS:master Apr 19, 2019
@miri64 miri64 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Apr 19, 2019
@miri64 miri64 deleted the gnrc_ipv6_nib/fix/no-auto-config-wo-l2addr branch April 19, 2019 06:53
@miri64
Copy link
Member Author

miri64 commented Apr 19, 2019

@danpetry I think this should be backported. Do you agree?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Reviewed: 3-testing The PR was tested according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants