Patch jail so that it can support mountd/nfsd in a vnet jail

[rmacklem]

Summary

D37519 has grown to be a large patch. This is the relatively
small part of it that applies to the jail code.
I think having it as a separate patch simplifies review and
it should be ok to commit before the rest of D37519.

VNET_SYSUNINT() registered the cleanup function, but it
was never called, since vnet_destroy() was never called when
a vnet jail that was running mountd/nfsd was removed.

As such, this patch defines a global function pointer that
the nfsd can set to nfsrv_cleanup(), which does the cleanup.
(I used a function pointer, since nfsd can be a loadable module.)

Test Plan

Tested by running mountd/nfsd within multiple vnet
jails.

[jamie]

What's the purpose of nfsd_call_vnet_cleanup being a function pointer? If it's because you need VNET_NFSD for it to exist, it should do to have #ifdef VNET_NFSD in prison_cleanup(). If it's to handle nfsd being dynamically loaded, the canonical way to handle it is via the OSD system, where the osd_jail_call(...PR_METHOD_REMOVE) would handle it. Or is there a third reason I don't know?

[rmacklem]

I had defined nfsd_call_vnetcleanup because the nfsd
can be loaded dynamically as a module and I had
never heard of the OSD framework.

As suggested by jamie@, the nfsd code now uses
osd_jail_register() to register the cleanup function
and that seems to work fine.

As such, this patch has the nfsd_call_vnetcleanup bits removed.

[jamie]

sys/kern/kern_jail.c

4518

If you put this in the dynamically loaded part of the nfsd code, that should make it so the parameter only shows up when nfsd is loaded. For that matter, it could also be wrapped inside #ifdef VNET_NFSD.

I think it generally better not to have a switch that can't be flipped, though another possibility is to keep the parameter always there, but supply an error when attempting to set it on a non-qualifying jail. For that matter, even if you move the SYSCTL_JAIL_PARAM, it would still be handy to check for and disallow setting the flag in a non-vnet jail or non-VNET_NFSD system.

[rmacklem]

sys/kern/kern_jail.c

4518

Ok, you kinda lost me with the above comment.

I thought about putting "allow.nfsd" ifdef'd on
VNET_NFSD, but then I think it might be irritating
for people to have the error spit out when a jail
is started and "allow.nfsd" happens to be in /etc/jail.conf.

mountd/nfsd won't start up even when "allow.nfsd" is specified,
since the call to prison_check_nfsd() will fail.

But if you prefer "allow.nfsd" to only be available to be set when
the kernel is configured with VNET_NFSD, I am fine with that, too.

[jamie]

sys/kern/kern_jail.c

4518

It's true that allow.nfsd being set on non-vnet-nfsd jails isn't an operational problem. The only question is what is best done in user communication.

I see what you mean about an expected parameter disappearing with a different configuration, though that's already the way other module-specific jail parameters work (e.g. linux compat and SYS IPC). So either way would be good.

I would expect that any jail with allow.nfsd in its configuration would be broken if nfsd isn't actually there, so an error message might be handy. Just saying the parameter doesn't exist is an admittedly blunt tool for the job.

[jamie]

Also, the jail.8 man page should have the parameter, which is a good place to mention the requirements for using it.

[rmacklem]

sys/kern/kern_jail.c

4518

The nfsd loads dynamically when the daemon is started,
so it will always be there if/when someone wants to run it.
(Unless they don't have "options NFSD" in their kernel and
also have not built nfsd.ko, but that's a weird case that nfsd(8)
handles.)

What could be checked here is, as you've noted, whether or
not VNET_NFSD is defined.

If you'd prefer the above SYSCTL_JAIL_PARAM() to be #ifdef'd
VNET_NFSD, I have no problem doing that.
Whatever you think is best.

I have no idea if I can configure it so that the test for the jail
being in its own file system can be used to decide whether or
not to show the param?

I can add a printf() to the nfs kernel code where prison_check_nfsd()
fails, so that there is a console message indicating why the nfsd
would not start up. Does that sound sufficient?

Oh, and the man page patch is in D37665.

[jamie]

sys/kern/kern_jail.c

4518

Ah, I hadn't considered a load-on-demand situation. Yes, then it would definitely be a problem defining the parameter in the dynamic code. I like the idea of only showing the parameter #ifdef VNET_NFSD.

The test for the jail being in its own filesystem can't determine whether the parameter is shown, since the concept of "shown" is independent of any particular prison. You could put something in kern_jail_set that returns an error mesasage (via vfs_opterror) if allow.vnet is set in a jail that lacks the necessary requirements. There are probably already things you can technically allow that you can't really do though, so it's not a huge issue.

[rmacklem]

This version has "allow.nfsd" #ifdef'd on VNET_NFSD,
as suggested by jamie@.

It also prints out a console warning if a jail with
allow.nfsd is not on a separate file system.
(I used printf() instead of vfs_opterror() because
the latter did not print out anything, I think due
to no "errmsg" option being set.)

[jamie]

The thing about the vfs_opterror, is the error message only applies when the jail_set(2) fails (or at least it's only checked then). So If you want to still allow the jail to be created and only give a warning, then printf() is the way to go.

[rmacklem]

I think a warning is sufficient, since if mountd or nfsd
is starting in the jail, they will fail. To me, that seems preferable
to the jail failing to run at all.

However, I'm not a jail guy, so if you think the jail should
fail (and require the admin to remove "allow.nfsd" to get
the jail to work), I can change it to that.

Which would you prefer?

[jamie]

Yes, the warning is sufficient. As I mentioned, there are other cases where a jail is allowed to be created as long as its options are allowed, even if it turns out not to be "fit for purpose".

[rmacklem]

Changed the #ifdef VNET_NFSD to a #if for all
three of VNET_NFSD, VIMAGE and NFSD for
"allow.nfsd".

I did this since all three of these build options
are needed for mountd/nfsd to run in a jail.

[rmacklem]

This version has "if (!jailed())" removed from prison_check_nfsd().
This was discussed in D37519 and I think removing the
clause does make the code more readable,
It also makes prison_check_nfsd() a natural fit for the check
in prison_priv_check().

I also added PRIV_VFS_MOUNT_EXPORTED, since it now
appears that a check for it will be in vfs_domount().