Differential D19839

add INET6 support to nfsdumpstate for the client callback address
ClosedPublic
Actions

Authored by rmacklem on Apr 7 2019, 1:12 AM.

Details

Reviewers

bz
rgrimes

Commits

rS346191: Add support for INET6 addresses to the kernel code that dumps open/lock state.
rS346190: Fix nfsdumpstate(8) so that it can print out INET6 callback addresses.

Summary

PR#223036 noted that the client callback address was always printed out as 0.0.0.0
by nfsdumpstate(8) when an INET6 address was used.
This patch adds INET6 support for this, so that an INET6 address prints out.

Test Plan

Has been tested for NFSv4.0 and NFSv4.1 on an INET6 mount.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rmacklem created this revision.Apr 7 2019, 1:12 AM

Herald added a subscriber: imp. · View Herald TranscriptApr 7 2019, 1:12 AM

Harbormaster completed remote builds in B23554: Diff 55896.Apr 7 2019, 1:12 AM

Fixed one place where I had typed AF_INET when it should have been AF_INET6.
This didn't seem to have caused problems during testing, so I don't think it actually mattered,
but it is now correct.

Harbormaster completed remote builds in B23555: Diff 55897.Apr 7 2019, 1:31 AM

rgrimes added inline comments.Apr 7 2019, 3:57 AM

sys/fs/nfsserver/nfs_nfsdserv.c
3720 ↗	(On Diff #55897)	This may need #ifdef INET6 protection, as I do not believe struct sockaddr_in6 should be defined unless INET6 is defined, if it is that is another bug. Fixing this may lead to the further rethink this code.
3764 ↗	(On Diff #55897)	So we print the port number as octets using .%d.%d on the end of the IP address? I would of thought it was in the more normal form of uint16 :%d. I see that that the printf's are untouched by your diff, so that is long standanding prior art perhaps?
4035 ↗	(On Diff #55897)	Again, a reference to a struct sockaddr_in6 outside of ifdef INET6
sys/fs/nfsserver/nfs_nfsdstate.c
4077 ↗	(On Diff #55897)	Is INET6_ADDRSTRLEN defined outside of #ifdef INET6? If so there is a bug someplace else.
4130 ↗	(On Diff #55897)	Here the port length is only 7, in all the code I have read up to this point in this review the claim is 8. When doing the max calculation it appeared to be .255.255. which is 8 characters. I would expect the minimum to be .0.0 which is 4, not 3.
4231 ↗	(On Diff #55897)	Shouldnt that 0x0 be IN_ADDRANY? Also isnt it valid for this to be bound to a wildcard socket and how does this get printed if it is bound to a wildcard socket? It looks as if when bound to a wildcard socket we force the port to be 0 to, which is probably wrong? I may totally be miss reading this code too.

Added replies to the inline comments. I will update the patch with the changes mentioned
in the replies later to-day.
Thanks for the review, rick

sys/fs/nfsserver/nfs_nfsdserv.c
3720 ↗	(On Diff #55897)	Here's a snippet from netinet/in.h, which I understand includes in6.h automatically for all kernel builds. (It will complain if you try to include netinet6/in6.h directly.) As such, sockaddr_in6 will always be defined. /* INET6 stuff */ #if POSIX_VISIBLE >= 200112 #define KAME_NETINET_IN_H_INCLUDED_ #include <netinet6/in6.h> #undef __KAME_NETINET_IN_H_INCLUDED_ #endif
3764 ↗	(On Diff #55897)	That is my understanding of RFC 1833, which is the one referenced by RFC 7530 (the NFSv4.0 one). It has been this way since the code was written and I have not heard any reports of interoperability problems, although I'll admit I haven't tested an INET6 mount against a non-FreeBSD server. (I know the .%d.%d is correct for INET.)
4035 ↗	(On Diff #55897)	Yep. There will be lots of those all over the NFS kernel code. I think it is always defined, as noted above.
sys/fs/nfsserver/nfs_nfsdstate.c
4077 ↗	(On Diff #55897)	Again, defined in in6.h, which is always included.
4130 ↗	(On Diff #55897)	The above code is correct, but you are right, it is confusing. More correctly, it is "8 - 1", where the "- 1" is because the total here does not include NULL termination (which is included in INET6_ADDRSTRLEN). I'll change the comment above and re-write the lines below as INET[6]6_ADDRSTRLEN - 1 + 8.
4231 ↗	(On Diff #55897)	I suppose it could be written as INADDR_ANY, but that might be more confusing. It might be better for me to add a comment explaining that the NFSv4 folks use 0.0.0.0 to indicate "no valid address" for this case. This is another case where the code has been this way for at least 15 years. (Same goes for all zeros for INET6.) Works to indicate "invalid address" because it can't work as a destination host address.

This version of the patch has the changes mentioned in the inline comments.
A comment has been added explaining that INET[6]_ADDRSTRLEN includes
NULL termination and the length calculation has been changed to "-1 + 8"
from "+ 7" to make the calculation more understandable.

I have also replaced "0x0" with INADDR_ANY and added a comment explaining
that this is (mis)used to indicate "no valid callback address exists" by NFSv4.0.

I have not #ifdef INET6'd the uses of "struct sockaddr_in6" since it is always
included when "netinet/in.h" is included.

Harbormaster completed remote builds in B23561: Diff 55925.Apr 7 2019, 8:34 PM

You can mark all my comments as done, you have addressed them to my satisfaction.

sys/fs/nfsserver/nfs_nfsdserv.c
3720 ↗	(On Diff #55897)	You can mark my comment as done, this is not a bug you have introduced, but is a much bigger problem of things being done wrong for INET6.
3764 ↗	(On Diff #55897)	I am satisfied, you can mark my comment as done.
4035 ↗	(On Diff #55897)	Ok again, and mark as done please.