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

RDNSS: support more ipv6 addresses #193

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WenChao1Hou
Copy link

@WenChao1Hou WenChao1Hou commented Oct 30, 2022

RFC 6106 recommended that the number of RDNSS addresses thatshould be learned and maintained through the RDNSS RA option should be limited to three. RFC 8106 removes that recommendation; thus, the number of RDNSS addresses to maintain is determined by an implementer's local policy.The number of RDNSS addresses to maintain is determined by the user's own profile

Fix:#182

@@ -594,6 +596,12 @@ static struct safe_buffer_list *add_ra_options_rdnss(struct safe_buffer_list *sb

memset(&rdnssinfo, 0, sizeof(rdnssinfo));

size_t const bytes = sizeof(rdnssinfo) + sizeof(struct in6_addr) * rdnss->AdvRDNSSNumber;
if (bytes > (256 * 8)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you pick 2048?

  • 1280 bytes is the min IPv6 MTU; what would happen in that case?
  • can radvd safely send multiple RA packets with different RDNSS entries? (other framework code for example splits large options into their own packets)

Copy link
Author

Choose a reason for hiding this comment

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

in my opinion
1、The IPv6 MTU value may change the processing of the kernel protocol stack.
2、RDNSS 2048 is consistent with DNSSL packet maximum in radvd

Do you have any good suggestions for me?

Copy link
Member

@robbat2 robbat2 left a comment

Choose a reason for hiding this comment

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

looks mostly good, some thoughts added; will wait for CI validation.

@robbat2
Copy link
Member

robbat2 commented Oct 30, 2022

Please amend the commit to include a Signed-Off-By trailer per DCO.

@robbat2
Copy link
Member

robbat2 commented Oct 30, 2022

Compile failure, wonder if you missed something?

send.c:620:31: error: use of undeclared identifier 'rdnssbuf'
                safe_buffer_append(sbl->sb, rdnssbuf->buffer, rdnssbuf->used);
                                            ^
send.c:620:49: error: use of undeclared identifier 'rdnssbuf'
                safe_buffer_append(sbl->sb, rdnssbuf->buffer, rdnssbuf->used);
                                                              ^
send.c:621:20: error: use of undeclared identifier 'rdnssbuf'
                safe_buffer_free(rdnssbuf);
                                 ^

@robbat2 robbat2 changed the title RDNSS section support more ipv6 aaddrss RDNSS: support more ipv6 addresses Oct 30, 2022
@robbat2 robbat2 added the rdnss label Oct 30, 2022
@robbat2
Copy link
Member

robbat2 commented Oct 30, 2022

I've got some tests kicking around to add as well after you have it fixed.

robbat2 added a commit to robbat2/radvd that referenced this pull request Oct 30, 2022
More testing, expected to fail until PR#193 is merged.

Reference: radvd-project#193
Signed-off-by: Robin H. Johnson <[email protected]>
@stappersg
Copy link
Member

Please amend the commit to include a Signed-Off-By trailer per DCO.

@WenChao1Hou that is up-to-you. So when it is day again in your time zone, please add Signed-Off-By.

@WenChao1Hou
Copy link
Author

Compile failure, wonder if you missed something?

send.c:620:31: error: use of undeclared identifier 'rdnssbuf'
                safe_buffer_append(sbl->sb, rdnssbuf->buffer, rdnssbuf->used);
                                            ^
send.c:620:49: error: use of undeclared identifier 'rdnssbuf'
                safe_buffer_append(sbl->sb, rdnssbuf->buffer, rdnssbuf->used);
                                                              ^
send.c:621:20: error: use of undeclared identifier 'rdnssbuf'
                safe_buffer_free(rdnssbuf);
                                 ^

Compile failure, wonder if you missed something?

send.c:620:31: error: use of undeclared identifier 'rdnssbuf'
                safe_buffer_append(sbl->sb, rdnssbuf->buffer, rdnssbuf->used);
                                            ^
send.c:620:49: error: use of undeclared identifier 'rdnssbuf'
                safe_buffer_append(sbl->sb, rdnssbuf->buffer, rdnssbuf->used);
                                                              ^
send.c:621:20: error: use of undeclared identifier 'rdnssbuf'
                safe_buffer_free(rdnssbuf);
                                 ^

it was an oversight on my part,The variable name was not updated in time when submitted, I will update MR as soon as possible

RFC 6106 recommended that the number of RDNSS addresses thatshould be learned and maintained through the RDNSS RA option should be limited to three.  RFC 8106 removes that recommendation; thus, the number of RDNSS addresses to maintain is determined by an implementer's local policy.The number of RDNSS addresses to maintain is determined by the user's own profile

Signed-off-by: WenChao1Hou <[email protected]>
@the-j0k3r
Copy link

the-j0k3r commented Nov 5, 2022

We had merged this patch downstream at this code state https://svn.dd-wrt.com/changeset/50745, I did not test it myself but clearly it caused some instability and was reverted

One such report https://forum.dd-wrt.com/phpBB2/viewtopic.php?p=1275031#1275031

Same thing happened to me. If you clean those two ipv6 dns lines (leave them empty) radvd deamon will not crash. Your/my /tmp/radvd.conf look/ed like:

root@tp:/tmp# cat radvd.conf
interface br0
{
IgnoreIfMissing on;
AdvSendAdvert on;
MinRtrAdvInterval 3;
MaxRtrAdvInterval 10;
AdvHomeAgentFlag off;
AdvManagedFlag off;
AdvOtherConfigFlag on;
AdvLinkMTU 1452;
prefix ::/64
{
AdvOnLink on;
AdvAutonomous on;
AdvValidLifetime 30;
AdvPreferredLifetime 20;
};
RDNSS 2606:4700:4700::1111 2606:4700:4700::1001{};
};

On 50786 once you leave those boxes empty you'll end up whith the same conf sans that RDNSS line and radvd won´t die.

Syslog entries of radvd deamon dying:

Nov 3 07:07:12 tp daemon.info radvd[9224]: version 2.19 started
Nov 3 07:07:12 tp daemon.err radvd[9226]: Exiting, privsep_read_loop had readn return 0 bytes
Nov 3 07:07:12 tp daemon.err radvd[9226]: Exiting, privsep_read_loop is complete.

Previous firm with that line working: 50485.

@wcbonner @BrainSlayer

@reubenhwk
Copy link
Collaborator

Just catching up here....why are more than 3 RDNS servers needed? I saw that more than three doesn't work, but I did NOT see any explanation as to why more than three are needed.

@wcbonner
Copy link

wcbonner commented Nov 7, 2022

Just catching up here....why are more than 3 RDNS servers needed? I saw that more than three doesn't work, but I did NOT see any explanation as to why more than three are needed.

There isn't really a huge need for more than three, but it would be nice if the program didn't crash or error out if more are listed in the config file.

The problem I ran into was in a downstream project when I wanted to specify two dns servers and the program was already adding two servers it had received from another network broadcast or dhcp interaction.

radvd was silently exiting in the background and my local network wasn't getting advertisements.

@reubenhwk
Copy link
Collaborator

Ack. It's been a long time since I've work on this code... Nonetheless I have concerns that not having a hard limit would eventually lead to MTU issues. At the same time I'm not sure that exiting the program is also the right thing to do. Not sure. Maybe best effort is good enough, maybe not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

6 participants