Why would we need this @jamie if we ever only run with or w/o vnet but never allow this in a plain-old-jail?

That's exactly what we're testing for here, no? I'll admit I don't see the logic in prison_check_nfsd() disallowing non-jailed callers (this requiring the explicit jailed() check here). But then I don't know just how separated the vnet-nfsd and regular-nfsd code paths are.

If you want me to change it, I can.

I think for consistency it should change then. prison_xxx() checks are typically "return true if the prison/cred is allowed to do the thing." Granted, that's not always what you want to know, but this seems like a case where it is.

@jamie I never envisioned this to possibly work without vnets.

And if that is what we want, plain-old-jail support as well, then some if these VNET changes highly likely need to be handled differently as well.

I just think we need to be very clear about this very quickly as a lot of abstractions and security checks will highly depend on one or the other.

The first test:

if (!jailed(cred))
     return (false);

was only there to simplify the code that calls it:
without this clause, all calling cases change from:

if (...!prison_check_nfsd())

to

if (... jailed() && !prison_check_nfs())

I'm all for simplifying the code in multiple locations, so I'll just let this rest. It was only ever a minor nit at most.

@jamie I never envisioned this to possibly work without vnets.

And if that is what we want, plain-old-jail support as well, then some if these VNET changes highly likely need to be handled differently as well.

No, I don't want it to work without vnets - that part of prison_check_nfsd() would remain as it. The only thing I was considering was if it allows prison0.

Okay, the comment has long been moved with updates; if we only do vnet can someone explain to me why we need OSD? Why are vnet abstractions not good enough?

If the jail went away entirely (and wasn't stuck dying on other things which makes jls by default no longer to show it but it can with an option) the VNET_SYSUNINT function should have been called. If it wasn't there is a problem somewhere which must be solved.

@jamie forgive me for asking as all of the #ifdef options and loadable of the module shouldn't make a difference, was the real problem that the NFS code would hold references somewhere which prevented the jail from actually dying?

You two seem to have an implicit understanding of something I missed (never learnt) in the original patch and I am trying to understand that.

@jamie forgive me for asking as all of the #ifdef options and loadable of the module shouldn't make a difference, was the real problem that the NFS code would hold references somewhere which prevented the jail from actually dying?

You two seem to have an implicit understanding of something I missed (never learnt) in the original patch and I am trying to understand that.

Yes, it all comes to holding references somewhere, and I don't think there's anything you're not getting - I don't know what reference this might be or anything like that.

So part of why I suggested an earlier cleanup was partly practical: it seems to solve the issue. It could be that whatever reference is held open shouldn't be at that point, or it could be that there's something working completely as it should, which includes that reference not going away until the NFS stuff is shut down. Either way, moving the NFS shutdown earlier avoids the problem, even if it doesn't really identify it.

The other half of my reasoning is jail philosophy: I'd like to get rid of this idea of a dying jail being anything other than the last blip while it finishes disposing of itself, and as long as something isn't needed to exist during the orderly shutdown, I'd like it to go away as soon as possible (when it enters dying state). That's what prison_cleanup() is about.

The solution to the jail sticking around was to move the nfsd cleanup from the end of prison_deref() to prison_cleanup(), which happens when the jail starts dying instead of when it finishes.

It would have sufficed to just add ifdef'd code to prison_cleanup itself, but since nfsd is dynamically loadable, OSD was the canonical way to go. It's not necessary to do it that way, given that VNET_NFSD already has its fingers all over the place; and since the dynamically loading part seems not to work, it may no longer be the right paradigm. But it still works.

Okay, then my main concern probably is (was) mixing the two techniques to abstract things (I would still hope the vnet bits were enough but that's investigate this once we are done here).
@rmacklem can you add a /* XXX-BZ OSD to VNET? */ comment to the osd_jail_register() call for me?

After this, a "ps ax" does not show any processes
marked with "J", so how do I see what is still holding
a reference, since you both seem to think that is why
VNET_SYSUNINT() doesn't work?

jls -d

That will list "dying" jails in addition to others. The definition of a dying jail is one that's not yet gone only because there are still references held to it somewhere. The "somewhere" is almost always a cred, but the list of creds that may be sitting around on a system is huge.

When there are no processes left (or child jails, or "persist"), i.e. when there are no user references (pr_uref), that's when the jail starts to die. Or you can force that with jail_remove(2). Then when there are no references at all (pr_ref), it is finished dying.