Page MenuHomeFreeBSD

netinet6: fix ND link-layer address option layout for IPoIB
ClosedPublic

Authored by vinicius_ferrao.net.br on Wed, Jul 8, 1:24 AM.
Referenced Files
F162309641: D58096.id181579.diff
Sat, Jul 11, 10:35 PM
F162282223: D58096.id181536.diff
Sat, Jul 11, 4:27 PM
F162278166: D58096.diff
Sat, Jul 11, 3:42 PM
Unknown Object (File)
Thu, Jul 9, 11:07 PM
Unknown Object (File)
Thu, Jul 9, 2:59 AM
Unknown Object (File)
Wed, Jul 8, 11:26 PM
Unknown Object (File)
Wed, Jul 8, 8:36 PM
Unknown Object (File)
Wed, Jul 8, 7:58 PM

Details

Summary

RFC 4391 (IP over InfiniBand), section 9.3, lays the ND source/target
link-layer address option out as type, length (3), two reserved zero
octets, then the 20-octet IPoIB link-layer address. The ND code
assumed the Ethernet layout (RFC 4861, section 4.6.1) everywhere and
read/wrote the address directly after the option header, i.e. two
octets early. The option-length sanity check computes 24 for both
layouts for a 20-octet address, so the mismatch was silent.

Against a conformant peer (e.g. Linux) this broke IPv6 over IPoIB in
both directions: the address parsed from a received NS/NA option was
shifted by two bytes, so the cached link-layer address embedded a GID
that does not exist on the fabric and replies were sent nowhere, and
the options we built carried our address at the wrong offset for the
peer. IPv4/ARP was unaffected. This blocked everything IPv6 over
IPoIB against Linux, including rdma_cm's IP-to-GID resolution and
therefore NFS-over-RDMA-over-InfiniBand with IPv6.

Add nd6_lladdr_opt_pad(), the analog of Linux's
ndisc_addr_option_pad(): the number of reserved octets between the
option header and the address (2 for IFT_INFINIBAND and
IFT_INFINIBANDLAG, 0 otherwise). Apply it at the option parse sites
(nd6_ns_input(), nd6_na_input(), nd6_rs_input(),
nd6_ra_opt_src_lladdr(), icmp6_redirect_input()) and the option build
sites (nd6_ns_output_fib(), nd6_na_output_fib(),
icmp6_redirect_output()). Zero the redirect target option before
filling it; previously its trailing pad bytes were left uninitialized
on links with 20-octet addresses. No functional change for link
types without reserved octets.

PR: 296585
Sponsored by: VersatusHPC

Test Plan

Two hosts on a ConnectX-6 InfiniBand fabric with IPoIB configured
(if_addrlen 20): a FreeBSD 16-CURRENT node and a Linux node (also
verified FreeBSD-to-FreeBSD with a second FreeBSD node).

Before: ping6 between the FreeBSD and Linux IPoIB addresses gets 100%
loss; the neighbor stays FAILED. tcpdump shows the Neighbor
Advertisement's target link-layer option carrying the 20-octet address
at the wrong offset (Ethernet layout, no reserved octets), while IPv4
ARP over the same interfaces works.

After: ping6 resolves in both directions; both ndp/ip -6 neigh
caches hold the peer's full 20-octet link-layer address, and tcpdump
shows the option in the RFC 4391 layout (two reserved octets, then the
address) byte-identical to what Linux emits. The Router Advertisement
input path was additionally exercised with a crafted RA carrying the
RFC 4391 source link-layer option, and ndp learned the router with
its full 20-octet address.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Can you please make the pad variables unsigned? (and while here the associated length variables as well)

sys/netinet6/nd6_nbr.c
608

caddr_t not needed here at all.

Oh this is a good catch! I was just about to put some cx6 NICs in some boxes here to test.

Just to be complete - did you also test the non-IB case? (straight up ipv6 over ethernet at least.)

Can you please make the pad variables unsigned? (and while here the associated length variables as well)

Done, in the original it was using what looks like the defaults to reduce the changes, the same for the casting to caddr_t.

But everything is addressed now. I'm running my test suite just to be sure now, but those change should not break anything.

Oh this is a good catch! I was just about to put some cx6 NICs in some boxes here to test.

Just to be complete - did you also test the non-IB case? (straight up ipv6 over ethernet at least.)

Hi @adrian

Yes, I've tested IPv6 with Ethernet on the patched kernel. ND works and resolves both directions. The implementation relies on nd6_lladdr_opt_pad() that returns 0 for everything except InfiniBand, so pad=0 makes every parse/build site compute the same offsets and lengths as before.

I do have tcpdump' output if needed.

We also run it on a 24h stress test, it was in another context but using this patch and it looks stable.

This revision is now accepted and ready to land.Wed, Jul 8, 3:17 PM
pouria requested changes to this revision.Wed, Jul 8, 6:30 PM

Nice catch! Thank you for your contribution.
IMHO, we simply need to make sure the length of our nd_opt header is align to octets.

sys/netinet6/icmp6.c
2515

Please use memset here.

sys/netinet6/nd6_nbr.c
603

No need for padding here,
8 byte alignments does the same.
for example: (20 + 7) & ~7 = 24;

1116

Since you're changing this line, please use memcpy instead.

1183–1189

We should simply do the 8-byte alignment here:

/* 8 byte alignments... */
optlen = (optlen + 7) & ~7;

RFC original intention here was simply to align to octets for header length.
Therefore, IMHO, there is no need for extra func too.

sys/netinet6/nd6_rtr.c
480

Again, the + 2) & ~7) here is doing your padding here.

This revision now requires changes to proceed.Wed, Jul 8, 6:30 PM
sys/netinet6/nd6_rtr.c
480

Again, the + 2) & ~7) here is doing your padding here.

  • + 7) & ~7)
vinicius_ferrao.net.br added inline comments.
sys/netinet6/nd6_nbr.c
1183–1189

Therefore, IMHO, there is no need for extra func too.

The function isn't computing alignment here, it supplies the address offset within the option. And also the function was made because it's used in more than one place, so it avoids code repetition.

1183–1189

We should simply do the 8-byte alignment here:

/* 8 byte alignments... */
optlen = (optlen + 7) & ~7;

RFC original intention here was simply to align to octets for header length.

Yes, but, alignment alone is what the code did before this patch. The option was already 24 octets, with the address at offset 2 and the zeros trailing.

The position must be fixed as stated here: https://www.rfc-editor.org/info/rfc4391/#section-9.3

Did I misunderstand?

I'm new to this Phabricator thing, and didn't thank @pouria correctly, because I clicked 'submit' too early.

So thanks for the review!

pouria added inline comments.
sys/netinet6/nd6_nbr.c
1183–1189

Yes, but, alignment alone is what the code did before this patch. The option was already 24 octets, with the address at offset 2 and the zeros trailing.

The position must be fixed as stated here: https://www.rfc-editor.org/info/rfc4391/#section-9.3

Did I misunderstand?

You're absolutely right.

This revision is now accepted and ready to land.Thu, Jul 9, 7:10 AM

Do you want me to commit this one? @vinicius_ferrao.net.br

Unfortunately, I don't have access to IB-capable NIC, did you test your final revision?
Simple NA/NS/RA are easy, however, did you managed to test the icmp6 redirect part too?

Yes, please @pouria.

I've tested the new changes against CX-6 and CX-5, but only on x86_64. Everything is working so far. I've also run this patch in a stress test in the context of another project (a NFS over RDMA server, which is how I found out about this issue in first place) and it worked.

To test the redirect I've used 3 VMs, being a FreeBSD router plus two Linux clients. Both input and output worked fine from everyone.

Thanks again.

PS: I don't know how FreeBSD works, but given the criticallity of this bug, should this be pushed to what will become 15.2-RELEASE? I marked only in CURRENT because this is what I'm testing against.

we can merge it into stable/15 before its cut and/or ask the release manager to merge it from stable/15 to the release branch after its cut but before its released.

As long as everyone's happy here with the change and it lands, it should make it into 15.2.

Yes, please @pouria.

I've tested the new changes against CX-6 and CX-5, but only on x86_64. Everything is working so far. I've also run this patch in a stress test in the context of another project (a NFS over RDMA server, which is how I found out about this issue in first place) and it worked.

To test the redirect I've used 3 VMs, being a FreeBSD router plus two Linux clients. Both input and output worked fine from everyone.

Thank you, I'll run my own tests and I'll push this soon.

we can merge it into stable/15 before its cut and/or ask the release manager to merge it from stable/15 to the release branch after its cut but before its released.

As long as everyone's happy here with the change and it lands, it should make it into 15.2.

I'm also fine with MFC to: stable/15

It's really hard to MFC this change to stable/15.
Could you please prepare a patch for stable/15 too?
If you're willing to do that, I could talk to release manager.

It's really hard to MFC this change to stable/15.
Could you please prepare a patch for stable/15 too?
If you're willing to do that, I could talk to release manager.

Yes. How is the procedure? I should open other Phabricator review or just attach a patch here?

Yes. How is the procedure? I should open other Phabricator review or just attach a patch here?

Please open another review.