Page MenuHomeFreeBSD

rtadvd: add multi pref64 support
Needs ReviewPublic

Authored by pouria on Sat, Jan 10, 5:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 10, 11:48 PM
Unknown Object (File)
Sat, Jan 10, 10:56 PM
Unknown Object (File)
Sat, Jan 10, 8:01 PM
Unknown Object (File)
Sat, Jan 10, 6:31 PM
Subscribers

Details

Reviewers
glebius
zlei
markj
ivy
hrs
bz
Group Reviewers
network
Summary

Add support for multi pref64 in rtadvd and rtadvctl

Test Plan

To test, you need to configure
your rtadvd.conf with a pref64 prefix:

bridge0:\
	:raflags="oal":rltime#600:\
	:addr="2a01:e140:cafe::":prefixlen#64:\
        :pref640="2a01:e140:dead:ff::":\
        :pref641="3fff::":pref64len1#64:

then simply check the rtadvctl show output.
Also verified with tcpdump.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 69904
Build 66787: arc lint + arc unit

Event Timeline

Added reviewers @ivy who introduced PREF64 (RFC 8781) support for rtadvd(8), and @hrs who wrote rtadvctl(8).

usr.sbin/rtadvctl/rtadvctl.c
923

I'd prefer to convert this into an assertion, given this static function is invoked only by function action_show() and follows the condition rai->rai_pref64.p64_enabled.

932

Then no need to return a zero.

Convert action_show_pref64 from int to void and use assertion to address @zlei comment.

This revision is now accepted and ready to land.Tue, Jan 13, 9:43 AM
bz requested changes to this revision.Tue, Jan 13, 5:39 PM
bz added a subscriber: bz.
bz added inline comments.
usr.sbin/rtadvctl/rtadvctl.c
630

RFC 8781 states in section 5: "This option may appear more than once in an RA"

Looking at rtadvd.h it is a list but as I much as I understand here, we would always only print a single prefix?

631

What's the point of a separate printf here compared to doing that in action_show_pref64()? File style?

This revision now requires changes to proceed.Tue, Jan 13, 5:39 PM
usr.sbin/rtadvctl/rtadvctl.c
631

I agree moving the printf inside action_show_pref64() is better.

925

Can you please write down a comment that would explain the arbitrary numbers.

usr.sbin/rtadvctl/rtadvctl.c
630

Unfortunately, the current implementation does not work that way.
We did not use TAILQ_HEAD inside the struct rainfo the way we have done for other options, such as prefix, rdnss, or dnssl.
Given that our current implementation doesn't provide the ability to use multiple pref64 options at the same time, I will probably have to abandon this revision and start implementing it the standard way, as stated in RFC 8781 section 5, in rtadvd.
I'll try to implement as you said.

925

The thing that comes to my mind is:

/* RFC 8781 Section 4: Map PLC values to prefix lengths */

The RFC 6052 uses a table to describe the values, as there isn't a straightforward algorithm to implement the mapping.
I have mapped the PLC 0 to 96, and since the other values have a fixed spacing (8-bit), I was able to calculate the rest.

usr.sbin/rtadvctl/rtadvctl.c
925

Yes, referencing the RFC that would explain where the values come from should be good enough.

Change revision to add support for multiple PREF64 options.

pouria retitled this revision from rtadvctl: add pref64 support to rtadvd: add multi pref64 support.Wed, Jan 14, 9:39 PM
pouria edited the summary of this revision. (Show Details)
pouria edited the test plan for this revision. (Show Details)

Here is the output sample of rtadvctl:

% mdo rtadvctl -v show
bridge0: flags=<UP,TRANSITIVE,PERSIST> status=<RA_SEND> mtu 1500
DefaultLifetime: 10m
MinAdvInterval/MaxAdvInterval: 3m20s/10m
AdvLinkMTU: <none>, Flags: MO, Preference: low
ReachableTime: 0s, RetransTimer: 0s, CurHopLimit: 64
AdvIfPrefixes: yes
Next RA send: Thu Jan 15 00:58:25 2026
Last RA send: Thu Jan 15 00:58:06 2026
Prefixes (1):
2a01:e140:1234:5678::/64 (CONFIG, vltime=30d, pltime=7d, flags=LA)
DNSSL entries:
spmzt.net (ltime=15m)
PREF64:
2a01:e140:cafe:ff::/96 (ltime: 3m45s)
2a01:e140:dead:ff::/64 (ltime: 3m45s)

@bz: I also tested it and I can confirm that it works using wireshark.
@zlei: I had to rollback the assert part, as there's no need for p64_enabled anymore. so I removed it.
@glebius should these minor changes to the manual be approved by the manpages group?

I also found a bug related to flag inconsistency. However, I will fix it in another revision.

@glebius should these minor changes to the manual be approved by the manpages group?

They should automatically receive a notification of a review that touches a manual page. If somebody from the group submits corrections, please follow them. If the manpages is silent in the next couple days while we are reviewing the code, it should be fine to proceed with push without their review.

rebase to latest commit and cleanup unused var

Thanks a lot for going the extra mile and changing the entire code!

usr.sbin/rtadvctl/rtadvctl.c
921

Just style but usually I/we'd sort; not sure what style.9 thinks about this these days.

struct pref64 *prf64;
uint16_t *prf64_cnt;
char ntopbuf[INET6_ADDRSTRLEN];
char ssbuf[SSBUFLEN];
char *p;
int i;
uint16_t prf64len;
931

That if seems unnecessary given the for loop will otherwise just never be executed?

usr.sbin/rtadvd/config.c
962

Not entirely true anymore as you may already have added a pref64 to the tailq and may add more but I think it's fine.
If we wanted to do anything we could print the address as well to known which entry? Just an idea, nothing you have to do.

974

No spaces between casts:

prf64->p64_sl = (uint16_t)(uint64_t)val64;

I am also not sure if you need the (uint64_t) in between?

usr.sbin/rtadvd/control_server.c
553

This seems wrong. If I am correct you only have one prf64_cnt at the beginning of the buffer and nothing further inter-spaced before/after each record?

571

In theory that should be "len" as well but there seems to be a possibility in the code that if the count of entries changes between the loops all other kinds of things go wrong as well; seems to not be a concern in this code anywhere (just looked up backwards at dns as well).

usr.sbin/rtadvd/rtadvd.conf.5
452

This last line is probably a copy&paste error.

509

Why not use the well known NAT64 prefix here in the example? 64:ff9b::/96
Also do you want to specify a pref64len as well?

Maybe put the pref63 on its own line as well to make it more clear?

the only purpose that RFC8781 mentions for having multiple PREF64s advertised is renumbering, in which case the lifetime of the deprecated prefixes should be set to zero. iiuc, this diff allows that by setting pref64lifetime0, pref64lifetime1, etc. - can i check i have that right?

P.S. Love the KAME project, but honestly, most of their userland code is weird.
For instance, I also have a branch for mobile ipv6 implementation, where I made a fair amount of changes to rtadvd, rtsold, rtadvctl, and others.
I have to say the layer of indirection in KAME code makes adding a single floating point number almost impossible without refactor.
I don't like their style of coding in userland either. However, To see if I should use our own style or simply follow existing, I checked the other revisions for these toolset found other developers simply didn't touch KAME style.

usr.sbin/rtadvctl/rtadvctl.c
921

You're right, unfortunately none of the KAME code follows our style.
That's why I'm reusing their order of variables declarations.

931

I thought the same myself. It feels unnecessary. I removed this line before and then rollback it as I didn't want to touch their style.
I will remove it if others also feeling the same.

usr.sbin/rtadvd/config.c
962

Nice catch. I'll see what can I do for it.

974

Again it's clearly style violation and the second cast is not required at all. But they've used the exact double casting over and over in the code.

usr.sbin/rtadvd/control_server.c
553

You're right. I'll fix that.