Date: Fri, 24 Apr 2026 14:44:26 +0800 From: Zhenlei Huang <zlei@FreeBSD.org> To: Kyle Evans <kevans@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-branches@freebsd.org" <dev-commits-src-branches@FreeBSD.org> Subject: Re: git: df670d26ad5b - stable/14 - jail: consistently populate the KP_JID and KP_NAME parameters Message-ID: <1B692EB3-E2C4-4AB2-9694-2F24223E0E17@FreeBSD.org> In-Reply-To: <69e996e4.3ac9c.14b0bfb4@gitrepo.freebsd.org>
index | next in thread | previous in thread | raw e-mail
> 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" > } >home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1B692EB3-E2C4-4AB2-9694-2F24223E0E17>