In D50371#1150037, @markj wrote:

In D50371#1149975, @kib wrote:

So overall I do not like this, sorry. It makes very delicate semantic change for what is basically an assisted self-induced jail problem. The O_RESOLVE_BENEATH is huge complication to the lookup process already to abuse it even more.

There is no semantic change, I am just adding a "sticky" mode for O_RESOLVE_BENEATH, i.e., extending the existing semantics. It gives capsicum's semantics for a single directory file descriptor. I suspect it could be useful elsewhere.

In D50390#1150039, @markj wrote:

This doesn't fix the problem. There are no mount points involved (except for a nullfs mount point).

The issue involves what happens if 1) a process is using a nullfs vnode as the cwd, 2) something moves the corresponding lower vnode outside of the directory hierarchy bound by the nullfs mount. Suppose that the process does chdir(".."). null_lookup() does the lower vnode lookup, then calls null_nodeget() to find the corresponding nullfs node. There is none (because the lower vnode isn't covered by nullfs anymore), so null_nodeget() instantiates a new vnode and returns it.

Now keep doing that until we reach the process' root directory (i.e., the jail root). We will return a nullfs vnode where the lower vnode is the process root dir. But then the root dir check in vfs_lookup() does not catch it, because it is using pointer equality to check whether the returned vnode is the root.

In fact this is the same case that is already handled by null_lookup ("Renames in the lower mounts might create an inconsistent configuration..."), but that check is not sufficient unless the root directory is also a mount point.

To fix it, I think null_lookup() needs to check whether the /lower/ vnode is equal to ndp->ni_rootdir or ndp->ni_topdir. But, VOP_LOOKUP does not get the nameidata. Maybe we should start passing that as an extra parameter?

In D50380#1150046, @emaste wrote:

mention that for dynamic binaries rtld selects which init model to use

We do describe NT_FREEBSD_NOINIT_TAG in here already. If we add more documentation should it be here or in rtld(1) with an explicit cross reference?

Old binaries: .init and .ctors, called by csu
New binaries with NT_FREEBSD_NOINIT_TAG: .init, .ctors, and .init_array, called by rtld

So overall I do not like this, sorry. It makes very delicate semantic change for what is basically an assisted self-induced jail problem. The O_RESOLVE_BENEATH is huge complication to the lookup process already to abuse it even more.
I would very much prefer the straight fix of not allowing to externalize dirfd if it comes from a different jail (with nuances about prison0 or equal roots).

In D50376#1149626, @emaste wrote:

Is an exp-run needed? I guess this is esoteric enough that it's not, although there is a reference in GCC.

You might want to mention that for dynamic binaries rtld selects which init model to use based on the presence of the FreeBSD note NOINIT.

Fix language mistakes in the man page.

IMO the right way is much simpler, and was formulated in the PR: only allow to pass dirfd across the same jail, or from any jail to prison0. I think your addition of allowing to pass dirfd between different jails but having same rootvp is neat.

So if talking about changing (bumping) this limit, we probably should just drop the kmem part of the formula for DMAP machines or machines where uma has small_alloc.

In D50310#1147904, @olce wrote:

In D50310#1147892, @kib wrote:

Having a verbose rewrite of the formula into the text does not help in reviewing the code.

It helps because it makes it clearer what the intentions of who changes the code are, and we can then check whether the actual code matches them.

Exactly the reverse. Your comment repeats the code and does not say anything about a possible intent.

In D50310#1147865, @olce wrote:

In D50310#1147813, @kib wrote:

What is the point of making comment repeating the code in even more verbose way?

The point of the new message is to clearly spell out in plain English what the formulae mean in more "physical" terms and be extra-clear about the computation (in particular, the 98,304 cutoff is not on the final number of vnodes, and the growth rate stated is in fact not the global one, both because the previous comments do not take the maxproc contribution into account). The computation here is not overly complex, but I still think the text has value. This is a pre-requisite either for revising it (in a very minor way now, for an alternative where the number of vnodes is raised instead) or just helping evaluate it quickly where it stands asymptotically with respect to maxfiles. In particular, I think it is important to state which formula actually applies and when.

This would make the comment inconsistent with the actual code on the next whatever minor change.

There are no more numbers in the new description than in the previous one, except for the last paragraph (see below).

While I agree with your concern in general, for a system-wide setting like this, I certainly don't want anyone to revise these numbers without proper review and updates of the surrounding comments.

Having a verbose rewrite of the formula into the text does not help in reviewing the code.

In D50125#1147871, @olce wrote:

In D50125#1147853, @markj wrote:

So the NFS server in particular doesn't break this mechanism? Certainly a number of other subsystems will hold a vnode's usecount > 0 without having an open file handle: mdconfig -f vnode, ktrace, swapon, ...

For mdconfig and swapon, AFAIK, the number of such vnodes is fairly limited. ktrace I don't know. I'm actually the most worried about nullfs. NFS has a similar problem. So, yes, these are definitely dents in the mitigation presented here. I don't think it means we should not apply it though.

In D50125#1147853, @markj wrote:

Thinking about it some more, from my POV the bug is that we are applying a resource limit (maxvnodes) when there is no actual shortage (of memory).

The objections to the previous review are equally applicable to this one.

What is the point of making comment repeating the code in even more verbose way? This would make the comment inconsistent with the actual code on the next whatever minor change.

Implement andrew' suggestion.

Add needed headers for mem_aarch64.c

On armv8, generate simple-minded SError.

In D50226#1145856, @jrtc27 wrote:

In D50226#1145838, @kib wrote:

In D50226#1145834, @jrtc27 wrote:

In D50226#1145753, @kib wrote:

For me, Elf_Addr is more natural type synonym to use in the rtld code. Also, it gives us the luxury of Elf32/64_Addr typedefs, so if needed, multilib-like rtld has an easier way to express itself.

Well, Elf_Addr is about the format of addresses within the ELF file, matching the default ELF class for the current ABI. Technically that does not have to match the format of addresses at run time (LP64 IA-64 used both ELFCLASS32 and ELFCLASS64; LP46 vs ILP32 was an orthogonal EF_IA_64_ABI64), though you probably shouldn't build a system that does that these days, and of course it definitely doesn't match the format of pointers for CHERI. The values here are not addresses in ELF files, they are pointers at run time, so they should use pointer types, and using anything other than (u)intptr_t or T * does not work on CHERI. We've already had to change this code downstream and whilst refactoring things upstream I'd like to take the time to use CHERI-compatible types rather than write new(ish) code that I know does not work for CHERI.

I think this opinion should be stated in the change' summary.

Thanks. Are you happy with the updated description?

In D50226#1145834, @jrtc27 wrote:

In D50226#1145753, @kib wrote:

For me, Elf_Addr is more natural type synonym to use in the rtld code. Also, it gives us the luxury of Elf32/64_Addr typedefs, so if needed, multilib-like rtld has an easier way to express itself.

Well, Elf_Addr is about the format of addresses within the ELF file, matching the default ELF class for the current ABI. Technically that does not have to match the format of addresses at run time (LP64 IA-64 used both ELFCLASS32 and ELFCLASS64; LP46 vs ILP32 was an orthogonal EF_IA_64_ABI64), though you probably shouldn't build a system that does that these days, and of course it definitely doesn't match the format of pointers for CHERI. The values here are not addresses in ELF files, they are pointers at run time, so they should use pointer types, and using anything other than (u)intptr_t or T * does not work on CHERI. We've already had to change this code downstream and whilst refactoring things upstream I'd like to take the time to use CHERI-compatible types rather than write new(ish) code that I know does not work for CHERI.

For me, Elf_Addr is more natural type synonym to use in the rtld code. Also, it gives us the luxury of Elf32/64_Addr typedefs, so if needed, multilib-like rtld has an easier way to express itself.

(I understand that this change in its current form depends on two other rtld reviews)

In D50216#1145253, @emaste wrote:

For me __curelm is a bit more readable than _Curelm, but I'm fine with this versino as well.