Commited in eb5bfdd06565. I forgot to add the review to the commit message :-/

I've committed the "jails can include jails" and "use the recursive parser" bits separately. This new diff is just the part that handles the includes.

In D40188#919081, @crest_freebsd_rlwinm.de wrote:

Just a small nitpick: I would prefer a macro #define MAX_INCLUDE_DEPTH 32

Simple include-loop prevention with via a maximum depth counter.

In D40262#917102, @nyan_myuji.xyz wrote:

However right now the kern.elfXX.fallback_elf_brand are tied to 2 global variables which their name are generated by the __elfN(xxx) macro. We could technically remove them and tie all 3 to the jail one? Maybe that belongs to a separate review?

Rather than add a separate value different from the kern.fallback_elf_brand sysctl, it would make sense for the jail parameter to be tied to the sysctl itself, such as the securelevel parameter is. It's complicated somewhat by the fact that there are three similar sysctls.

True, they're not handled. I took my include inspiration from newsyslog (which has includes that also support globbing), and there it's also just a simple matter or running whatever it's told to include. It's kind of a footgun situation, where it's generally good enough to trust the administrator not to make such a loop. I did it for depend loops, but only because that's kind of elemental in building an acyclic directed graph.

New and improved diff :-)

I just created D40188, which is my take on solving this issue. It goes the include-file direction, and doesn't require and changes to rc.conf, since jail.conf is where such changes are made. The jail(8) command line also remains untouched.

s/novnetjail/nojailvnet/

This makes perfect sense to me. The original version only set redo_ip[46] provisionally , and I missed that the patch changed that.

In D38144#871980, @rmacklem wrote:

Oh, I remember jamie@ mentioning the jail cleanup
method.

Would the call to the PR_METHOD_REMOVE function
be the right place to get rid of the credential references
in the mountlist?

I defined a function called vfs_exjail_delete(), which I currently
call from the nfsd'd OSD PR_MOETHOD_REMOVE which
releases credentials and set mnt_exjail NULL for all cases
matching the prison argument.
Maybe this function should be called from within kern_jail.c
via prison_cleanup()?

In D38371#872242, @mjg wrote:

I would expect killing the jail to *fail* if there any exports active. You can keep the counter of them in struct prison to avoid the need to scan anything.

With the assumption that nfsd in jail is *disabled* by default, this would not violate POLA.

The alive check raises a question: what if instead of a loop in vfs_export that checks prisons, how about a loop in prison_cleanup that checks exports? It could go through the mount list and any exports that belong to it can be refiled for the parent.

Sure, this one's no biggie.

I don't especially like the name pr_ident, which isn't readily differentiated from pr_id. Perhaps pr_permid? In a perfect world, I wouldn't have allowed prison IDs to be reused, but it's too late to stuff that horse back into the barn. Probably - I have considered actually changing the policy, but it would have POLA problems and would need a slow rollout with proper deprecation warnings.

In D38144#867425, @rmacklem wrote:

I noticed that there are old versions of "struct prison"
in sys/jail.h. Is this necessary when "struct prison" is
revised?

It took me longer to remember what I had done in the first place than it did to understand the new logic. I like this.

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".

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.

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

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?

In D35198#797902, @dchagin wrote:

In D35198#797888, @jamie wrote:

Wow, that has been there for an embarrassingly long time!

Well, it's not a bug)) because the rpr cannot be NULL AFAIR, or Im missed something?

Wow, that has been there for an embarrassingly long time!

In D33339#756945, @glebius wrote:

@jamie I actually got one more question about this code. The code that checks for IP addresses clashing, in the old code under comment that starts "Check for conflicting IP addresses", in the new code separated into function prison_ip_conflict_check(), it would not allow to create a child that has IP address of a parent if parent has multiple addresses
...
So my patch doesn't change this behavior, but it seems counter-intuitive to me. And the comment actually says it is intended:

If there is a duplicate on a jail with more than one IP stop checking and return error.

Why so?

I take it this is for what we're seeing in Bug 260335.

This presupposes that a jail that isn't marked to persist isn't going to stick around for some other reason. A jail could be started for example to start a long-running daemon, or to be a parent of other jails. Automatically going away when its task is done is a feature of such a jail.

I'm not thrilled at the redundant call to vfs_flagopt(), which shouldn't be necessary because kern_jail_set has already looked for allow.novmm and set the permission bit accordingly. But by the time vmmdev_prison_set() is called, the old value of the permission bit is forgotten. So you're left with

In D29659#665126, @me_igalic.co wrote:

there are other candidates that return (0) or a single error that is always the same, but here we would need modifying the call-site :

• prison_check_af() 0/EAFNOSUPPORT
• prison_canseemount() 0/ENOENT
• prison_check() 0/ESRCH

Yeah, I'd been meaning to get around to that ;-).

This would work well with jexec -l, which is already somewhat like su -l but misses the parts you mention. In fact, I would recommend making clean (-l) the deciding factor instead of pwd (-u/U). And I don't see a reason why the same directory change shouldn't be done regardless of whether it's for a command or a shell.