Page MenuHomeFreeBSD

if_stf: add 6rd support
ClosedPublic

Authored by kp on Wed, Nov 17, 5:03 PM.

Details

Summary

Implement IPv6 Rapid Deployment (RFC5969) on top of the existing 6to4
(RFC3056) if_stf code.

PR: 253328
Obtained from: pfSense
Sponsored by: Rubicon Communications, LLC ("Netgate")

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kp requested review of this revision.Wed, Nov 17, 5:03 PM
  • Please update stf(4) manual page, too.
  • I would suggest disabling 6to4 functionality when a braddr is configured. A mixed configuration of 6to4 and 6rd is quite confusing when seeing a result of ifconfig(8) command-line output because the braddr is a parameter for the interface, not a single 6rd domain configuration. I believe no one (except for me) uses the both at the same time.
sbin/ifconfig/Makefile
38

This should be under .if ${MK_INET6_SUPPORT} != "no".

sbin/ifconfig/ifstf.c
93

ifconfig(8) generally resolves a host name wherever an address can be specified. I am not sure if it must be supported here because a 6rd configuration is address-family specific, but this should be noted in the manual page at least.

119

This can be removed safely because strtonum() should also check whether it is a number or not.

122

I think either 0 or 32 must be explicity rejected because 0 is silently converted to 32 in the kernel.

125

"prefix length" is better than "width", I think.

131

memcpy()?

sys/net/if_stf.c
433
  • LHS should be simply addr6.
  • RHS can be simplified by using IFA_IN6(is).
  • memcpy() or bcopy() might be preferred. The line 438 uses it even for a 32-bit long address, for example. And by the way, L.392 rewrote bcopy() with memcpy(). Consistency in this file might be good.
433–442

I think there is no need to define ia6 after fixing the following two lines.

434
  • IA6_MASKIN6(ia)
515

in_nullhost()?

819

This should be inside the next if-conditional.

822

These two lines are confusing. How about v4prefix = ntohl(sc->srcv4_addr & (0xffffffffU << v4suffixlen))?

823

(sc->v4prefixlen > 32) must be rejected and return with NULL. It never happens if SIOCSDRVSPEC guarantees it is between 1 to 32, but having a sanity check is useful.

832

(plen > 64) must be rejected and return with NULL before this. It is still possible to happen.

875

sizeof(args)

880

bcopy or memcpy.

881

The value must be checked to guarantee that it is within 1 to 32.

903

Consistency. SIOCSDRVSPEC checks ifd_cmd first, and this checks ifd_len first.

kp marked 17 inline comments as done.Fri, Nov 19, 6:19 PM
In D33037#746495, @hrs wrote:
  • Please update stf(4) manual page, too.

There's D33042 for that.

  • I would suggest disabling 6to4 functionality when a braddr is configured. A mixed configuration of 6to4 and 6rd is quite confusing when seeing a result of ifconfig(8) command-line output because the braddr is a parameter for the interface, not a single 6rd domain configuration. I believe no one (except for me) uses the both at the same time.

Thank you for the extensive review!

sbin/ifconfig/ifstf.c
93

All examples of 6rd configuration I can find uses the IP address for the BR. The pfSense implementation also only supports using an IP address.

All together I'd be inclined to keep this the way it is. It'll be easy to extend later if we do encounter users who want to use a DNS name.

The if_stf.4 changes for 6rd describe the configuration command as "The border router IPv4 address is configured with the
.Xr ifconfig 8
.Cm stfv4br
command."

sys/net/if_stf.c
515

That expects a struct in_addr, and we use in_addr_t here.

kp marked an inline comment as done.

Review remarks

Looks good to me. Thanks for your hard work!

This revision is now accepted and ready to land.Fri, Nov 19, 8:15 PM
This revision was automatically updated to reflect the committed changes.