fix nfsuserd to find a mapped "localhost" ip address and to use INET6 when INET isn't available

[rmacklem]

Summary

When jails are enabled, 127.0.0.1 is mapped to another IP address. This breaks the nfsuserd daemon.
This patch handles this case by finding out what address "localhost" is mapped to.
The patch also adds code to make nfsuserd to use INET6 if INET isn't available on the system.
(This code will probably be committed as two patches, one for each of the above changes. It also
requires a small kernel patch not included here and a small change to the nfsuserd.8 man page
that is not included here. Both of these are trivial and probably don't need review.)

Test Plan

I have tested it for both INET and INET6 with some extra syslog() logging in the code.
I also commented out the "same address" test, so that nfsbind_localhost() was called.
I have never run a jail and my test system doesn't have enough free disk space, so testing
with a jail running is still needed.

[rgrimes]

I can not speak much for all this code, other than to say I can read it and pretty much understand what is going on, however I would like to address the issue that this code is missing the conditional compilation on if I even want either or both of IPv4 or ipv6 supported. I believe most (all) our other code has #ifdef INET or #ifdef INET6 in it, see usr.bin/netstat as an example. I am not hardbound on using "localhost" as a grep of the code shows this has seriously been ignored, which means /etc/hosts is no longer the source of truth for the value of localhost :-(.

usr.sbin/nfsuserd/nfsuserd.c

822 ↗

(On Diff #53999)

I believe that this should actually be a namelookup of "localhost", not a hardcoding of the value ::1

842 ↗

(On Diff #53999)

Again here, I believe this should be a name lookup of localhost, not a hardcoded 127.0.0.1, as it is valid for a person to choose to set localhost to another value.

[rmacklem]

The problem with doing a name lookup in this daemon is that it assumes/expects it
to work. Often (an NFS mounted root fs is one example and a system configured to
use DNS before the local /etc/hosts file) are not yet working and an NFS mount is
expected to work.
(ie. In general I'd agree with you, but for NFS not so much...)

In this case, I believe "127.0.0.1" and "::1" are defined by RFCs and will not be changing,
so I think hard coding them is acceptable and avoids any dependency on being able
to access /etc/hosts or a DNS service to allow it to run.

[rmacklem]

Added a comment w.r.t. why "localhost" is hardcoded. I will also note that the kernel will
use these hardcoped values (I don't believe the kernel can do a name lookup and I wouldn't
want it to in this case), so the code doesn't want to use any other value, if a sysadmin were
to change the values for "localhost".

I had thought that "#ifdef INET6" was no longer needed?
Maybe bz@ can clarify this?
(If required, I will add them, although I think it makes the code less pleasant to read.)

[rgrimes]

In D19218#411391, @rmacklem wrote:

The problem with doing a name lookup in this daemon is that it assumes/expects it
to work. Often (an NFS mounted root fs is one example and a system configured to
use DNS before the local /etc/hosts file) are not yet working and an NFS mount is
expected to work.
(ie. In general I'd agree with you, but for NFS not so much...)

A nfs route mount occurs inside the kernel when neither /etc/hosts nor DNS
is avaliable, and this code, from the client side, does not come into play at all.
Further more, this is a usr.sbin program, so long after networking and DNS
are up and running.

In this case, I believe "127.0.0.1" and "::1" are defined by RFCs and will not be changing,
so I think hard coding them is acceptable and avoids any dependency on being able
to access /etc/hosts or a DNS service to allow it to run.

As I already stated it is not uncommon for a administrator to over ride the value of localhost in /etc/hosts and expect that things do the right thing.
And the RFC's to my knowledge never actually say that 127.0.0.1 must be the localhost, they do say:
"This is ordinarily implemented using only 127.0.0.1/32 for loopback." [rfc5735, but obsoleted by rfc6890 which does not even seem to mention 127.0.0.1]
The fact is 127/8 is reserved for this purpose, and is used by many in more ways than just the .1 address.
For ipv6 the ground is clearer, but lacks any must language as far as ::1, in rfc4291, 2.5.3, "The unicast address 0:0:0:0:0:0:0:1 is called the loopback address.

It may be used by a node to send an IPv6 packet to itself."

[rmacklem]

W.r.t. the NFS root mount, you are sort of correct at this time.
What would happen for an NFSv4 root mount at this time is
that the NFS client code would have a miss on the "uid<->username"
cache and attempt an upcall to the nfsuserd, which would not
work. (I think it will eventually fail and use "nobody", but I'd have
to look at the code to be sure.
As such, NFSv4 root mounts are not currently recommended, but I
would like to resolve that someday.

I have never been a professional developer, but I have been a
sysadmin for 30years and I have seen ugly situations during
recovery from a power failure where the DNS service wasn't
yet available. (The DNS server didn't boot or was overloaded.)
For example, if the DNS does NFS mounts and the NFS server
isn't running yet because it needs DNS...)
Most of this is avoided by specifying use of /etc/hosts firsts
and making sure the critical entries are in there, but that doesn't
work if the root fs is NFS mounted or the sysadmin doesn't
follow the above advise w.r.t. configuration setup.

I have always tried to avoid unnecessary dependencies on
the NFS code working and still believe this is the correct way
to go.

If you prefer, in this case the kernel is doing an upcall using
the address "127.0.0.1" and so long as that is supported by
the system as a valid address for the local machine, this is
sufficient. It does not need to be "localhost" as defined by
the DNS.
I can update comments to purge all mention of "localhost"
to reflect this.

Btw, I think this is an example of where phabricator is not
a good tool. This discussion is better held on a mailing list
and I will ask on freebsd-net@ what others think about this.

[rgrimes]

In D19218#411491, @rmacklem wrote:

W.r.t. the NFS root mount, you are sort of correct at this time.
What would happen for an NFSv4 root mount at this time is
that the NFS client code would have a miss on the "uid<->username"
cache and attempt an upcall to the nfsuserd, which would not
work. (I think it will eventually fail and use "nobody", but I'd have
to look at the code to be sure.
As such, NFSv4 root mounts are not currently recommended, but I
would like to resolve that someday.

Fixing that would be good, I did not realize we could not do a
nfsv4 root mount. BUTT if doing that means hard coding things
like localhost into the kernel as 127.0.0.1 we should make that
value tunable. I did search the /sys tree and found miminal
use of 127.0.0.1 often coded as 7f000001, and would like to
see those go away or become tuneables.
It is simply wrong to hard code this type of stuff.

I have never been a professional developer, but I have been a
sysadmin for 30years and I have seen ugly situations during
recovery from a power failure where the DNS service wasn't
yet available. (The DNS server didn't boot or was overloaded.)
For example, if the DNS does NFS mounts and the NFS server
isn't running yet because it needs DNS...)
Most of this is avoided by specifying use of /etc/hosts firsts
and making sure the critical entries are in there, but that doesn't
work if the root fs is NFS mounted or the sysadmin doesn't
follow the above advise w.r.t. configuration setup.

Most modern setups just do not function of DNS is not
up and running, PXE/iPXE booting fails as often files are
downloaded from named servers, etc.
If your DNS infastructure depends on NFS your probably
doing something wrong. Not that it isnt done, but needs
special care. I do not care to break the norm for the odd
ball.

I have always tried to avoid unnecessary dependencies on
the NFS code working and still believe this is the correct way
to go.

IMHO, any hard coded constant in the kernel of the value
of "localhost" is a mistake, and at least should be a tunable
that can be set.

If you prefer, in this case the kernel is doing an upcall using
the address "127.0.0.1" and so long as that is supported by
the system as a valid address for the local machine, this is
sufficient. It does not need to be "localhost" as defined by
the DNS.

What I think your overlooking is that on some system
127.0.0.1 may NOT work, because the system admin has
changed this to 127.0.0.3 for the instance of this kernel..

I can update comments to purge all mention of "localhost"
to reflect this.

Just the opposite of what I am advocating, it should purge
all mention of 127.0.0.1.

Btw, I think this is an example of where phabricator is not
a good tool. This discussion is better held on a mailing list
and I will ask on freebsd-net@ what others think about this.

Full agree, please do take this conversation to @net

[bz]

I've not seen what you two said and done since I read through after you put it up; I'll have some comments but I'll be mostly afk this; I'll try to get to this Tuesday afternoon/evening. It's flagged important and open in a browser already.

[bz]

Ok, so one thing at a time:

you want in your Makefile

.if ${MK_INET_SUPPORT} != "no"
CFLAGS+=-DINET
.endif
.if ${MK_INET6_SUPPORT} != "no"
CFLAGS+=-DINET6
.endif

and then put all INET or INET6 specific code under #ifdefs ideally. It's still needed. We still have people who want to only have one or the other.

[bz]

I think doing DNS things for these is not a good thing. Especially given inside an IP jail a "localhost" will give you different results.

I'd use available macros for 127.1 and ::1 (INADDR_LOOPBACK, IN6ADDR_LOOPBACK_INIT) to initialize things.

I'd prefer you'd prefer IPv6 to IPv4 as that'll help us to find problems for the future and not keep the past alive.

You'll find that you might want to re-structure the code as "use_inet6" is not exactly helpful. There's a struct sockaddr_union which can hold both address families and you could switch based on the sa_family field instead maybe?

[rmacklem]

This version of the patch includes the changes that bz@ recommended.
I think he meant sockaddr_storage when he said sockaddr_union, so I
made "fromip" one of those instead of having both "fromip" and "fromip6"
plus "use_inet6" to decide which to use.
It uses INET6 if it can and uses the INADDR_LOOPBACK and IN6ADDR_LOOPBACK_INIT
macros.

[rgrimes]

In D19218#412405, @bz wrote:

I think doing DNS things for these is not a good thing. Especially given inside an IP jail a "localhost" will give you different results.

Could you elaborate on that "give you different results?" This may be the very thing I am trying to say, and it may be what could break this code if run inside a jail, or in my other cases test bed with modified localhost value.

I am happy with the INET/INET6 resolution, and the use of macros for loopback is certainly better than inline constants

[rgrimes]

I smashed the wrong thing. I do not see the Makefile in the diffs, nor do I see the inline #ifdef's.

[rmacklem]

Whether or not "#ifdef INET6" and "#ifdef INET" should be in the code is still an open question.
I was hoping bz@ could answer this?
(I, personally, was of the understanding that this was no longer required/recommended for
FreeBSD sources. If bz@ does not know the correct answer for this, I will ask on freebsd-net@.)
At this point I have made no changes to the Makefile.
To build/run it, there is a small kernel change required, but I did not include that in this review.

[rgrimes]

In D19218#412862, @rmacklem wrote:

Whether or not "#ifdef INET6" and "#ifdef INET" should be in the code is still an open question.
I was hoping bz@ could answer this?
(I, personally, was of the understanding that this was no longer required/recommended for
FreeBSD sources. If bz@ does not know the correct answer for this, I will ask on freebsd-net@.)
At this point I have made no changes to the Makefile.
To build/run it, there is a small kernel change required, but I did not include that in this review.

Oh wow, bz must of replied in email or something and not on the review :-(, and I believe I have deleted the email. He confirmed that we still need these, and showed what to do in the Makefile. bz@ could you please take your 2 sets of comments that you sent and place them in the review, thanks Rod

[rmacklem]

Oops, my mistake. I didn't notice he had made multiple comments.
I see it now and will add the ifdefs.

[bz]

I like this one a lot more already. Left a few minor comments which might help.

usr.sbin/nfsuserd/nfsuserd.c

103 ↗

(On Diff #54171)

Do you need the temp variable or could you use the initializer in the place where it's used instead?

513 ↗

(On Diff #54171)

A switch() will help you if you'll do the #ifdefs.

814 ↗

(On Diff #54171)

sin and sin6?

818 ↗

(On Diff #54171)

Again a switch might make the #ifdef easier.

[bz]

@rgrimes inside a jail "localhost", a bind to, e.g., ::1, might end up on an IPv6 address of the jail, normally the first one, and not on ::1 hence it won't be "localhost". It's not changing what localhost is (e.g., in /etc/hosts), it's the kernel which changes the address under the hood during the syscalls (connect being another one).

[rmacklem]

Updated the patch with changes suggested by bz@. I did not change the usage
of INET6_LOOOPBACK_INIT, since I couldn't see an easy way to use it to initialize
a "struct sockaddr_storage". (There might be a C trick to using this initializer
that I don't know about.

Thanks bz@ and rgrimes@ for your comments, rick

[bz]

usr.sbin/nfsuserd/Makefile

2 ↗

(On Diff #54277)

There's a missing:

.include <src.opts.mk>

usr.sbin/nfsuserd/nfsuserd.c

117 ↗

(On Diff #54277)

I can't compile test as it's not defined for me? Is that part of your kernel changes not here?

[rmacklem]

This patch has the .include <src.opts.mk> added to the Makefile and has the kernel
patch, which I was going to leave until later, but bz@ wanted to compile the code,
so here it is.
(Note that the kernel patch mostly replaces the AF_LOCAL socket code that is unused,
because the patch to nfsuserd.c for this was reverted and never MFC'd. As such, the kernel
changes won't apply to a stable branch and is only for head. I am planning on first reverting
the kernel change for AF_LOCAL and then applying the changes, so the patch that will be
committed to head for the kernel changes won't look the same as this patch, although the
post-commit code should look the same.)

[rgrimes]

The added kernel code seems to be mssing INET/INET6 ifdefs. None of the inline comments are marked done, but the code looks as if you have addressed them.

sys/fs/nfs/nfs_commonsubs.c

3539 ↗

(On Diff #54318)

#ifdef INET6/#endif protection is needed here?

[bz]

The user space part seems fine to me now; I assume we'll see another review for the kernel parts?

[rmacklem]

Yes, sometime in April I will revert the AF_LOCAL patch from head and then create an updated (simpler)
kernel patch, including the #ifdef INET/INET6.

[rmacklem]

This version of the patch has an updated set of kernel changes
against head that include the #ifdef INET and INET6.

[rmacklem]

Oops, I think I uploaded the wrong version of the diff. Here is
the correct one with updated kernel changes that include #ifdef INET and INET6.

[rgrimes]

usr.sbin/nfsuserd/nfsuserd.c

192 ↗

(On Diff #55870)

Does it make since to have a CTASSERT that this code is pretty much useless if
!(INET | INET6)? I didnt check the makefile one level above to see that we do not build under those conditions but the sequence of code here reminded me that it is valid to build a system with neither INET or INET6.

324 ↗

(On Diff #55870)

Is this #ifdef DEBUG really that huge, it seems to kill a very large amount of code? Infact I am unable to locate the #endif

[rmacklem]

Replied to the inline comments.

usr.sbin/nfsuserd/nfsuserd.c

192 ↗

(On Diff #55870)

Well, I know nothing about the Makefiles. I suppose building it
when neither INET nor INET6 are defined doesn't make much sense,
but it will build and simply exit with that error when run, so it
doesn't seem harmful. (I suspect thereare very few builds without INET or INET6 done.)

324 ↗

(On Diff #55870)

You need to click on the "show all lines". By default, it wasn't displaying
the next few lines with the #else and #endif.

[rgrimes]

usr.sbin/nfsuserd/nfsuserd.c

192 ↗

(On Diff #55870)

I am ok with that explination, please mark this issue as Done.

324 ↗

(On Diff #55870)

I tried that, sadly the show all lines didnt actually show all lines, it only showed me all lines ABOVE my current place in the diff. I found the other show all lines and now I can
see if #endif. Sorry for the noise. Please just mark my original comment as done.

[rmacklem]

Marked rgrimes inline comments done per his request.

[bz]

Scrolling through the kernel changes it looks good to me; not done an in-detail review on them.