Re: git: df670d26ad5b - stable/14 - jail: consistently populate the KP_JID and KP_NAME parameters
Date: Fri, 24 Apr 2026 06:44:26 UTC
> On Apr 23, 2026, at 11:49 AM, Kyle Evans <kevans@freebsd.org> wrote:
>
> The branch stable/14 has been updated by kevans:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=df670d26ad5bebce71f39fc3bdc3ef11ed0f1ea8
>
> commit df670d26ad5bebce71f39fc3bdc3ef11ed0f1ea8
> Author: Kyle Evans <kevans@FreeBSD.org>
> AuthorDate: 2025-07-26 03:13:44 +0000
> Commit: Kyle Evans <kevans@FreeBSD.org>
> CommitDate: 2026-04-23 03:22:59 +0000
>
> jail: consistently populate the KP_JID and KP_NAME parameters
>
> The gaps here, specifically, were:
> - When we have to discover a running jail's jid from name, we should
> populate the missing jid param
> - When we populate jid/name from the config, if the name is a jid we
> wouldn't populate the name; now we do both.
> - When we create a jail, we should populate jid and name with whatever
> details we have now that we didn't both.
>
> As a consequence, we can cleanup a few things:
> - vnet.interface and zfs.dataset can just always use the jid
> - Trying to populate JNAME should always work now, where it would be
> a little crashy before if you create a jail that didn't have a name
> or jid on the command line
> - We can simplify the just-prior JID population now that we'll keep a
> stringified jid in our intparams.
>
> This primarily fixes the below, but the issues with vnet.interface and
> zfs.dataset were pre-existing.
Thanks for doing this !
>
> Fixes: d8f021add40c3 ("jail: add JID, JNAME and JPATH to env [...]")
> Reviewed by: jamie
>
> (cherry picked from commit 02944d8c4969ffe97fcf84cb2ccb672e828c1d04)
> ---
> usr.sbin/jail/command.c | 36 ++++++---
> usr.sbin/jail/config.c | 13 +++-
> usr.sbin/jail/jail.c | 7 ++
> usr.sbin/jail/tests/jail_basic_test.sh | 129 +++++++++++++++++++++++++++++++++
> 4 files changed, 173 insertions(+), 12 deletions(-)
>
> diff --git a/usr.sbin/jail/command.c b/usr.sbin/jail/command.c
> index 20f28abc6706..26eaeca442a9 100644
> --- a/usr.sbin/jail/command.c
> +++ b/usr.sbin/jail/command.c
> @@ -291,8 +291,8 @@ run_command(struct cfjail *j)
> const struct cfstring *comstring, *s;
> login_cap_t *lcap;
> const char **argv;
> - char *acs, *ajidstr, *cs, *comcs, *devpath;
> - const char *jidstr, *conslog, *path, *ruleset, *term, *username;
> + char *acs, *cs, *comcs, *devpath;
> + const char *conslog, *path, *ruleset, *term, *username;
> enum intparam comparam;
> size_t comlen;
> pid_t pid;
> @@ -333,6 +333,25 @@ run_command(struct cfjail *j)
> printf("%d\n", j->jid);
> if (verbose >= 0 && (j->name || verbose > 0))
> jail_note(j, "created\n");
> +
> + /*
> + * Populate our jid and name parameters if they were not
> + * provided. This simplifies later logic that wants to
> + * use the jid or name to be able to do so reliably.
> + */
> + if (j->intparams[KP_JID] == NULL) {
> + char ljidstr[16];
> +
> + (void)snprintf(ljidstr, sizeof(ljidstr), "%d",
> + j->jid);
> + add_param(j, NULL, KP_JID, ljidstr);
> + }
> +
> + /* This matches the kernel behavior. */
> + if (j->intparams[KP_NAME] == NULL)
> + add_param(j, j->intparams[KP_JID], KP_NAME,
> + NULL);
> +
> dep_done(j, DF_LIGHT);
> }
> return 0;
> @@ -457,8 +476,7 @@ run_command(struct cfjail *j)
> argv[0] = _PATH_IFCONFIG;
> argv[1] = comstring->s;
> argv[2] = down ? "-vnet" : "vnet";
> - jidstr = string_param(j->intparams[KP_JID]);
> - argv[3] = jidstr ? jidstr : string_param(j->intparams[KP_NAME]);
> + argv[3] = string_param(j->intparams[KP_JID]);
> argv[4] = NULL;
> break;
>
> @@ -772,14 +790,10 @@ run_command(struct cfjail *j)
> endpwent();
> }
> if (!injail) {
> - if (asprintf(&ajidstr, "%d", j->jid) == -1) {
> - jail_warnx(j, "asprintf jid=%d: %s", j->jid,
> - strerror(errno));
> - exit(1);
> - }
> - setenv("JID", ajidstr, 1);
> - free(ajidstr);
> + if (string_param(j->intparams[KP_JID]))
> + setenv("JID", string_param(j->intparams[KP_JID]), 1);
> setenv("JNAME", string_param(j->intparams[KP_NAME]), 1);
> +
> path = string_param(j->intparams[KP_PATH]);
> setenv("JPATH", path ? path : "", 1);
> }
> diff --git a/usr.sbin/jail/config.c b/usr.sbin/jail/config.c
> index 8c9ff0a7bd09..42498f01efd0 100644
> --- a/usr.sbin/jail/config.c
> +++ b/usr.sbin/jail/config.c
> @@ -156,11 +156,14 @@ load_config(const char *cfname)
> TAILQ_CONCAT(&opp, &j->params, tq);
> /*
> * The jail name implies its "name" or "jid" parameter,
> - * though they may also be explicitly set later on.
> + * though they may also be explicitly set later on. After we
> + * collect other parameters, we'll go back and ensure they're
> + * both set if we need to do so here.
> */
> add_param(j, NULL,
> strtol(j->name, &ep, 10) && !*ep ? KP_JID : KP_NAME,
> j->name);
> +
> /*
> * Collect parameters for the jail, global parameters/variables,
> * and any matching wildcard jails.
> @@ -180,6 +183,14 @@ load_config(const char *cfname)
> TAILQ_FOREACH(p, &opp, tq)
> add_param(j, p, 0, NULL);
>
> + /*
> + * We only backfill if it's the name that wasn't set; if it was
> + * the jid, we can assume that will be populated later when the
> + * jail is created or found.
> + */
> + if (j->intparams[KP_NAME] == NULL)
> + add_param(j, j->intparams[KP_JID], KP_NAME, NULL);
> +
> /* Resolve any variable substitutions. */
> pgen = 0;
> TAILQ_FOREACH(p, &j->params, tq) {
> diff --git a/usr.sbin/jail/jail.c b/usr.sbin/jail/jail.c
> index 9e443c9f3f1d..c99fb162786b 100644
> --- a/usr.sbin/jail/jail.c
> +++ b/usr.sbin/jail/jail.c
> @@ -887,7 +887,14 @@ running_jid(struct cfjail *j, int dflag)
> j->jid = -1;
> return;
> }
> +
> j->jid = jail_get(jiov, 2, dflag ? JAIL_DYING : 0);
> + if (j->jid > 0 && j->intparams[KP_JID] == NULL) {
> + char jidstr[16];
> +
> + (void)snprintf(jidstr, sizeof(jidstr), "%d", j->jid);
> + add_param(j, NULL, KP_JID, jidstr);
> + }
> }
>
> static void
> diff --git a/usr.sbin/jail/tests/jail_basic_test.sh b/usr.sbin/jail/tests/jail_basic_test.sh
> index 51ccafb45f60..a6859b4dc7f2 100755
> --- a/usr.sbin/jail/tests/jail_basic_test.sh
> +++ b/usr.sbin/jail/tests/jail_basic_test.sh
> @@ -137,10 +137,139 @@ commands_cleanup()
> fi
> }
>
> +atf_test_case "jid_name_set" "cleanup"
> +jid_name_set_head()
> +{
> + atf_set descr 'Test that one can set both the jid and name in a config file'
> + atf_set require.user root
> +}
> +
> +find_unused_jid()
> +{
> + : ${JAIL_MAX=999999}
> +
> + # We'll start at a higher jid number and roll through the space until
> + # we find one that isn't taken. We start high to avoid racing parallel
> + # activity for the 'next available', though ideally we don't have a lot
> + # of parallel jail activity like that.
> + jid=5309
> + while jls -cj "$jid"; do
> + if [ "$jid" -eq "$JAIL_MAX" ]; then
> + atf_skip "System has too many jail, cannot find free slot"
> + fi
> +
> + jid=$((jid + 1))
> + done
> +
> + echo "$jid" | tee -a jails.lst
> +}
> +clean_jails()
> +{
> + if [ ! -s jails.lst ]; then
> + return 0
> + fi
> +
> + while read jail; do
> + if jls -e -j "$jail"; then
> + jail -r "$jail"
> + fi
> + done < jails.lst
> +}
> +
> +jid_name_set_body()
> +{
> + local jid=$(find_unused_jid)
> +
> + echo "basejail" >> jails.lst
> + echo "$jid { name = basejail; persist; }" > jail.conf
> + atf_check -o match:"$jid: created" jail -f jail.conf -c "$jid"
> + atf_check -o match:"$jid: removed" jail -f jail.conf -r "$jid"
> +
> + echo "basejail { jid = $jid; persist; }" > jail.conf
> + atf_check -o match:"basejail: created" jail -f jail.conf -c basejail
> + atf_check -o match:"basejail: removed" jail -f jail.conf -r basejail
> +}
> +
> +jid_name_set_cleanup()
> +{
> + clean_jails
> +}
> +
> +atf_test_case "param_consistency" "cleanup"
> +param_consistency_head()
> +{
> + atf_set descr 'Test for consistency in jid/name params being set implicitly'
> + atf_set require.user root
> +}
> +
> +param_consistency_body()
> +{
> + local iface jid
> +
> + echo "basejail" >> jails.lst
> +
> + # Most basic test: exec.poststart running a command without a jail
> + # config. This would previously crash as we only had the jid and name
> + # as populated at creation time.
> + atf_check jail -c path=/ exec.poststart="true" command=/usr/bin/true
> +
> + iface=$(ifconfig lo create)
> + atf_check test -n "$iface"
> + echo "$iface" >> interfaces.lst
> +
> + # Now do it again but exercising IP_VNET_INTERFACE, which is an
> + # implied command that wants to use the jid or name. This would crash
> + # as neither KP_JID or KP_NAME are populated when a jail is created,
> + # just as above- just at a different spot.
> + atf_check jail -c \
> + path=/ vnet=new vnet.interface="$iface" command=/usr/bin/true
> +
> + # Test that a jail that we only know by name will have its jid resolved
> + # and added to its param set.
> + echo "basejail {path = /; exec.prestop = 'echo STOP'; persist; }" > jail.conf
> +
> + atf_check -o ignore jail -f jail.conf -c basejail
> + atf_check -o match:"STOP" jail -f jail.conf -r basejail
> +
> + # Do the same sequence as above, but use a jail with a jid-ish name.
> + jid=$(find_unused_jid)
> + echo "$jid {path = /; exec.prestop = 'echo STOP'; persist; }" > jail.conf
> +
> + atf_check -o ignore jail -f jail.conf -c "$jid"
> + atf_check -o match:"STOP" jail -f jail.conf -r "$jid"
> +
> + # Ditto, but now we set a name for that jid-jail.
> + echo "$jid {name = basejail; path = /; exec.prestop = 'echo STOP'; persist; }" > jail.conf
> +
> + atf_check -o ignore jail -f jail.conf -c "$jid"
> + atf_check -o match:"STOP" jail -f jail.conf -r "$jid"
> +
> + # Confirm that we have a valid jid available in exec.poststop. It's
> + # probably debatable whether we should or not.
> + echo "basejail {path = /; exec.poststop = 'echo JID=\$JID'; persist; }" > jail.conf
> + atf_check -o ignore jail -f jail.conf -c basejail
> + jid=$(jls -j basejail jid)
> +
> + atf_check -o match:"JID=$jid" jail -f jail.conf -r basejail
> +
> +}
> +
> +param_consistency_cleanup()
> +{
> + clean_jails
> +
> + if [ -f "interfaces.lst" ]; then
> + while read iface; do
> + ifconfig "$iface" destroy
> + done < interfaces.lst
> + fi
> +}
>
> atf_init_test_cases()
> {
> atf_add_test_case "basic"
> atf_add_test_case "nested"
> atf_add_test_case "commands"
> + atf_add_test_case "jid_name_set"
> + atf_add_test_case "param_consistency"
> }
>