Page MenuHomeFreeBSD

sys/jail.h: expose JAIL_MAX constant to applications
AbandonedPublic

Authored by igoro on Jan 17 2024, 1:31 PM.
Tags
None
Referenced Files
F102800749: D43476.diff
Sun, Nov 17, 8:48 AM
F102747423: D43476.id132871.diff
Sat, Nov 16, 4:07 PM
Unknown Object (File)
Mon, Oct 21, 8:00 PM
Unknown Object (File)
Mon, Oct 21, 8:00 PM
Unknown Object (File)
Mon, Oct 21, 7:42 PM
Unknown Object (File)
Sun, Oct 20, 10:34 PM
Unknown Object (File)
Sun, Oct 20, 10:34 PM
Unknown Object (File)
Sep 24 2024, 4:33 AM

Details

Reviewers
kevans
jamie
Group Reviewers
Jails
Summary

There is at least one application I know, kyua, which by design knows about this limit and needs to use exactly the same value as the system does -- not to get "permission denied" if a wrong one is used, e.g. if it's set to equal or greater than prison0 one. The respective kyua jail support patch is work in progress -- https://reviews.freebsd.org/D42350.

If the constant is exposed we could avoid its definition duplication within the src tree.

Do we have objections?

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

igoro requested review of this revision.Jan 17 2024, 1:31 PM
igoro edited the summary of this revision. (Show Details)
kevans added a subscriber: kevans.

I don't have any technical objection to it, though I do wonder now that I'm looking at it if we want to consider breaking this out as a rdonly sysctl instead. I'm somewhat wondering if one would have a use in the future to make the overall max tunable (namely, *decreasing* the max # jails for some reason).

This revision is now accepted and ready to land.Jan 17 2024, 4:46 PM
jamie added a subscriber: jamie.

I'm not sure why this limit exists in the first place (it predates me). I suppose it's just for neatness' sake, with the idea no one would have more jails than that anyway. But as long as it's around, it might as well be known.

this macro should be eliminated, not exposed

the idea that kyua should look at it does not make sense either -- what if there are some jails already present?

instead, the actual thing you are working on should have a sensible limit(tm) in place (and configurable by the user), which is definitely below what happens to be jail max today.

in absolute worst case a RO sysctl could be added as mentioned by @kevans

Thank you all for your time and the review.

In D43476#991440, @mjg wrote:

this macro should be eliminated, not exposed
the idea that kyua should look at it does not make sense either -- what if there are some jails already present?
instead, the actual thing you are working on should have a sensible limit(tm) in place (and configurable by the user), which is definitely below what happens to be jail max today.
in absolute worst case a RO sysctl could be added as mentioned by @kevans

Thank you very much, you have taken me out of the deep flow state. Working on the kyua patch I stuck with the vision of very simple cases like CI test run or very usual development workflow, when everything is run beneath the prison0. But our test suite is not limited to be launched only within the prison0, i.e. parent prison can already be something below with its own children.max, e.g.:

prison0                         children.max=999999
    some_jail_below             children.max=10
        kyua_test_case_jail     children.max=?

Yes, the whole test suite is not ready to be run within a jail, but we should not limit options for no reason, it's quite a frequent case to run only part of the suite. Moreover, kyua jail support does not want to limit it, but it has to set something due to the default is not practical, it's 0. We've already agreed with @markj that actually kyua wants to ask for the available maximum for a test case jail. So, it looks like it's not about picking a number at compile time and/or having it manually configured at run time -- the best kyua's wish here is to get the available maximum provided dynamically, i.e. ? is expected to be 9 in the given example above.

I have a quick idea to check on my side first, it could be another Differential to review.

Just to comment on the jail thing... I'd love it if we could get kyua running well in a jail... it's the best test we have for qemu bsd-user testing...

In D43476#991829, @imp wrote:

Just to comment on the jail thing... I'd love it if we could get kyua running well in a jail... it's the best test we have for qemu bsd-user testing...

Thanks, it only confirms the reasoning.

I've quickly checked different vectors of how children.max could be retrieved by an app for its current prison:

  • Jail library. I see no way to do it, it requires jail name or jid, which are unknown for an app. And even if it's known, the current (parent) jail is "invisible". No surprise here -- all according to the jail design.
  • Jail syscalls. As long as the libjail is merely a wrapper around the syscalls it leaves an app with the same dilemma.
  • sysctl security.jail.param.*. I see that the CTLTYPE_INT leafs are not implemented, they are always 0. As the code comments state it's simply a "menu" of existing params.

It looks like if we had security.jail.children.max sysctl then it would fulfill kyua needs. Thus, it would yield currproc->p_ucred->cr_prison->p_childmax value -- similar to existing security.jail.jailed or security.jail.vnet which work in the context of current prison.
A quick example of its behavior to make sure we're on the same page:

# sysctl -n security.jail.children.max
999999
# jail -c children.max=3 command=sysctl -n security.jail.children.max
3

It would close the dilemma when we are allowed to set children.max for a child jail up to parent's children.max minus 1, but we do not know parent's children.max value itself.

@jamie, does it look as an acceptable feature to introduce security.jail.children.max RO sysctl which reflects the current prison's p_childmax? If it does then I will help to implement it.

P.S. Probably you know existing "legal" ways to retrieve current prison's children.max w/o additional code to introduce.

In D43476#992839, @igor.ostapenko_pm.me wrote:

@jamie, does it look as an acceptable feature to introduce security.jail.children.max RO sysctl which reflects the current prison's p_childmax? If it does then I will help to implement it.
P.S. Probably you know existing "legal" ways to retrieve current prison's children.max w/o additional code to introduce.

That would be acceptable, though I would prefer a more general solution. There are many useful bits of information about the current jail that don't need to be hidden from users, and few that are worth keeping hidden. I've been thinking of allowing jail_get(jid=0, ...) to see a sanitized version of the current jail (no path, jid, name, other things that only make sense in the parent context).

But in the meantime, there are already many sysctls that are just windows to current-jail values, so adding this fits into that scheme. Also security.jail.children.current, since they could use the same function.

That would be acceptable, though I would prefer a more general solution. There are many useful bits of information about the current jail that don't need to be hidden from users, and few that are worth keeping hidden. I've been thinking of allowing jail_get(jid=0, ...) to see a sanitized version of the current jail (no path, jid, name, other things that only make sense in the parent context).

Yeah, I was thinking about something similar, but I quickly realized that it could be very controversial with possible breakage of the established design. Thus, it could prolong the kyua jail support patch development.

I've also got a feeling that jid=0 is like a reserved value in fact, which could be used for such opportunities. As long as you confirm such approach then it could be interesting to implement it in future if there is a production need. Added to my project ideas list.

But in the meantime, there are already many sysctls that are just windows to current-jail values, so adding this fits into that scheme. Also security.jail.children.current, since they could use the same function.

Great, please find the initial version of the patch here: https://reviews.freebsd.org/D43565.

I think we can abandon this proposal in favor of the dynamic security.jail.children.max sysctl. Landing this patch has a non-zero chance to break something, it looks better to keep the things as they are.