Date: Thu, 30 Nov 2023 00:22:37 GMT From: Jamie Gritton <jamie@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: ed31b3f4a146 - main - jail: Don't allow jail_set(2) to resurrect dying jails. Message-ID: <202311300022.3AU0Mb3x034136@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by jamie: URL: https://cgit.FreeBSD.org/src/commit/?id=ed31b3f4a146b3aa38f416954b6dbffad02efa5d commit ed31b3f4a146b3aa38f416954b6dbffad02efa5d Author: Jamie Gritton <jamie@FreeBSD.org> AuthorDate: 2023-11-30 00:12:13 +0000 Commit: Jamie Gritton <jamie@FreeBSD.org> CommitDate: 2023-11-30 00:12:13 +0000 jail: Don't allow jail_set(2) to resurrect dying jails. Currently, a prison in "dying" state (removed but still holding resources) can be brought back to alive state via "jail -d", or the JAIL_DYING flag to jail_set(2). This seemed like a good idea at the time. Its main use was to improve support for specifying the jid when creating a jail, which also seemed like a good idea at the time. But resurrecting a jail that was partway through thr process of shutting down is trouble waiting to happen. This patch deprecates that flag, leaving it as a no-op for creating jails (but still useful for looking at dying jails). It sill allows creating a new jail with the same jid as a dying one, but will renumber the old one in that case. That's imperfect, but allows for current behavior. Reviewed by: bz Differential Revision: https://reviews.freebsd.org/D28150 --- lib/libc/sys/jail.2 | 6 +- sys/kern/kern_jail.c | 259 ++++++++++++++++++++++++++++----------------------- sys/sys/jail.h | 2 +- usr.sbin/jail/jail.8 | 24 +++-- usr.sbin/jail/jail.c | 86 ++++------------- 5 files changed, 186 insertions(+), 191 deletions(-) diff --git a/lib/libc/sys/jail.2 b/lib/libc/sys/jail.2 index 4bb4a6dde7d2..8f8b9925c712 100644 --- a/lib/libc/sys/jail.2 +++ b/lib/libc/sys/jail.2 @@ -23,7 +23,7 @@ .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF .\" SUCH DAMAGE. .\" -.Dd February 19, 2021 +.Dd November 29, 2023 .Dt JAIL 2 .Os .Sh NAME @@ -185,7 +185,9 @@ it, as with the .Fn jail_attach system call. .It Dv JAIL_DYING -Allow setting a jail that is in the process of being removed. +This is deprecated in +.Fn jail_set +and has no effect. .El .Pp The diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c index 66bcd77ca8d2..c7b07d5762fa 100644 --- a/sys/kern/kern_jail.c +++ b/sys/kern/kern_jail.c @@ -136,8 +136,10 @@ SX_SYSINIT(allprison_lock, &allprison_lock, "allprison"); struct prisonlist allprison = TAILQ_HEAD_INITIALIZER(allprison); LIST_HEAD(, prison_racct) allprison_racct; int lastprid = 0; +int lastdeadid = 0; static int get_next_prid(struct prison **insprp); +static int get_next_deadid(struct prison **insprp); static int do_jail_attach(struct thread *td, struct prison *pr, int drflags); static void prison_complete(void *context, int pending); static void prison_deref(struct prison *pr, int flags); @@ -977,7 +979,7 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) #endif struct vfsopt *opt; struct vfsoptlist *opts; - struct prison *pr, *deadpr, *inspr, *mypr, *ppr, *tpr; + struct prison *pr, *deadpr, *dinspr, *inspr, *mypr, *ppr, *tpr; struct vnode *root; char *domain, *errmsg, *host, *name, *namelc, *p, *path, *uuid; char *g_path, *osrelstr; @@ -988,10 +990,10 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) #endif unsigned long hid; size_t namelen, onamelen, pnamelen; - int born, created, cuflags, descend, drflags, enforce; + int created, cuflags, descend, drflags, enforce; int error, errmsg_len, errmsg_pos; int gotchildmax, gotenforce, gothid, gotrsnum, gotslevel; - int jid, jsys, len, level; + int deadid, jid, jsys, len, level; int childmax, osreldt, rsnum, slevel; #ifdef INET int ip4s; @@ -1404,6 +1406,7 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) */ pr = NULL; inspr = NULL; + deadpr = NULL; if (cuflags == JAIL_CREATE && jid == 0 && name != NULL) { namelc = strrchr(name, '.'); jid = strtoul(namelc != NULL ? namelc + 1 : name, &p, 10); @@ -1433,69 +1436,38 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) continue; if (inspr->pr_id > jid) break; - pr = inspr; - mtx_lock(&pr->pr_mtx); - drflags |= PD_LOCKED; + if (prison_isalive(inspr)) { + pr = inspr; + mtx_lock(&pr->pr_mtx); + drflags |= PD_LOCKED; + } else { + /* Note a dying jail to handle later. */ + deadpr = inspr; + } inspr = NULL; break; } - if (pr != NULL) { - /* Create: jid must not exist. */ - if (cuflags == JAIL_CREATE) { - /* - * Even creators that cannot see the jail will - * get EEXIST. - */ - error = EEXIST; - vfs_opterror(opts, "jail %d already exists", - jid); - goto done_deref; - } - if (!prison_ischild(mypr, pr)) { - /* - * Updaters get ENOENT if they cannot see the - * jail. This is true even for CREATE | UPDATE, - * which normally cannot give this error. - */ - error = ENOENT; - vfs_opterror(opts, "jail %d not found", jid); - goto done_deref; - } - ppr = pr->pr_parent; - if (!prison_isalive(ppr)) { - error = ENOENT; - vfs_opterror(opts, "jail %d is dying", - ppr->pr_id); - goto done_deref; - } - if (!prison_isalive(pr)) { - if (!(flags & JAIL_DYING)) { - error = ENOENT; - vfs_opterror(opts, "jail %d is dying", - jid); - goto done_deref; - } - if ((flags & JAIL_ATTACH) || - (pr_flags & PR_PERSIST)) { - /* - * A dying jail might be resurrected - * (via attach or persist), but first - * it must determine if another jail - * has claimed its name. Accomplish - * this by implicitly re-setting the - * name. - */ - if (name == NULL) - name = prison_name(mypr, pr); - } - } - } else { - /* Update: jid must exist. */ - if (cuflags == JAIL_UPDATE) { - error = ENOENT; - vfs_opterror(opts, "jail %d not found", jid); - goto done_deref; - } + if (cuflags == JAIL_CREATE && pr != NULL) { + /* + * Even creators that cannot see the jail will + * get EEXIST. + */ + error = EEXIST; + vfs_opterror(opts, "jail %d already exists", jid); + goto done_deref; + } + if ((pr == NULL) + ? cuflags == JAIL_UPDATE + : !prison_ischild(mypr, pr)) { + /* + * Updaters get ENOENT for nonexistent jails, + * or for jails they cannot see. The latter + * case is true even for CREATE | UPDATE, + * which normally cannot give this error. + */ + error = ENOENT; + vfs_opterror(opts, "jail %d not found", jid); + goto done_deref; } } /* @@ -1547,54 +1519,35 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) if (namelc[0] != '\0') { pnamelen = (ppr == &prison0) ? 0 : strlen(ppr->pr_name) + 1; - deadpr = NULL; FOREACH_PRISON_CHILD(ppr, tpr) { - if (tpr != pr && - !strcmp(tpr->pr_name + pnamelen, namelc)) { - if (prison_isalive(tpr)) { - if (pr == NULL && - cuflags != JAIL_CREATE) { - /* - * Use this jail - * for updates. - */ - pr = tpr; - mtx_lock(&pr->pr_mtx); - drflags |= PD_LOCKED; - break; - } - /* - * Create, or update(jid): - * name must not exist in an - * active sibling jail. - */ - error = EEXIST; - vfs_opterror(opts, - "jail \"%s\" already exists", - name); - goto done_deref; - } - if (pr == NULL && - cuflags != JAIL_CREATE) { - deadpr = tpr; - } - } - } - /* If no active jail is found, use a dying one. */ - if (deadpr != NULL && pr == NULL) { - if (flags & JAIL_DYING) { - pr = deadpr; - mtx_lock(&pr->pr_mtx); - drflags |= PD_LOCKED; - } else if (cuflags == JAIL_UPDATE) { - error = ENOENT; + if (tpr == pr || !prison_isalive(tpr) || + strcmp(tpr->pr_name + pnamelen, namelc)) + continue; + if (cuflags == JAIL_CREATE || pr != NULL) { + /* + * Create, or update(jid): name must + * not exist in an active sibling jail. + */ + error = EEXIST; vfs_opterror(opts, - "jail \"%s\" is dying", name); + "jail \"%s\" already exists", name); goto done_deref; } + /* Use this jail for updates. */ + pr = tpr; + mtx_lock(&pr->pr_mtx); + drflags |= PD_LOCKED; + break; } - /* Update: name must exist if no jid. */ - else if (cuflags == JAIL_UPDATE && pr == NULL) { + /* + * Update: name must exist if no jid is specified. + * As with the jid case, the jail must be currently + * visible, or else even CREATE | UPDATE will get + * an error. + */ + if ((pr == NULL) + ? cuflags == JAIL_UPDATE + : !prison_isalive(pr)) { error = ENOENT; vfs_opterror(opts, "jail \"%s\" not found", name); @@ -1618,6 +1571,36 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) vfs_opterror(opts, "prison limit exceeded"); goto done_deref; } + + if (deadpr != NULL) { + /* + * The prison being created has the same ID as a dying + * one. Handle this by giving the dying jail a new ID. + * This may cause some confusion to user space, but + * only to those listing dying jails. + */ + deadid = get_next_deadid(&dinspr); + if (deadid == 0) { + error = EAGAIN; + vfs_opterror(opts, "no available jail IDs"); + goto done_deref; + } + mtx_lock(&deadpr->pr_mtx); + deadpr->pr_id = deadid; + mtx_unlock(&deadpr->pr_mtx); + if (dinspr == deadpr) + inspr = deadpr; + else { + inspr = TAILQ_NEXT(deadpr, pr_list); + TAILQ_REMOVE(&allprison, deadpr, pr_list); + if (dinspr != NULL) + TAILQ_INSERT_AFTER(&allprison, dinspr, + deadpr, pr_list); + else + TAILQ_INSERT_HEAD(&allprison, deadpr, + pr_list); + } + } if (jid == 0 && (jid = get_next_prid(&inspr)) == 0) { error = EAGAIN; vfs_opterror(opts, "no available jail IDs"); @@ -2017,14 +2000,13 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) * Persistent prisons get an extra reference, and prisons losing their * persist flag lose that reference. */ - born = !prison_isalive(pr); if (ch_flags & PR_PERSIST & (pr_flags ^ pr->pr_flags)) { if (pr_flags & PR_PERSIST) { prison_hold(pr); /* - * This may make a dead prison alive again, but wait - * to label it as such until after OSD calls have had - * a chance to run (and perhaps to fail). + * This may be a new prison's first user reference, + * but wait to call it alive until after OSD calls + * have had a chance to run (and perhaps to fail). */ refcount_acquire(&pr->pr_uref); } else { @@ -2039,7 +2021,7 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) * Any errors past this point will need to de-persist newly created * prisons, as well as call remove methods. */ - if (born) + if (created) drflags |= PD_KILL; #ifdef RACCT @@ -2092,7 +2074,7 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) #endif /* Let the modules do their work. */ - if (born) { + if (created) { error = osd_jail_call(pr, PR_METHOD_CREATE, opts); if (error) goto done_deref; @@ -2105,7 +2087,7 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) * A new prison is now ready to be seen; either it has gained a user * reference via persistence, or is about to gain one via attachment. */ - if (born) { + if (created) { drflags = prison_lock_xlock(pr, drflags); pr->pr_state = PRISON_STATE_ALIVE; } @@ -2135,7 +2117,7 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) } #endif - if (born && pr != &prison0 && (pr->pr_allow & PR_ALLOW_NFSD) != 0 && + if (created && pr != &prison0 && (pr->pr_allow & PR_ALLOW_NFSD) != 0 && (pr->pr_root->v_vflag & VV_ROOT) == 0) printf("Warning jail jid=%d: mountd/nfsd requires a separate" " file system\n", pr->pr_id); @@ -2242,6 +2224,55 @@ get_next_prid(struct prison **insprp) return (jid); } +/* + * Find the next available ID for a renumbered dead prison. This is the same + * as get_next_prid, but counting backward from the end of the range. + */ +static int +get_next_deadid(struct prison **dinsprp) +{ + struct prison *dinspr; + int deadid, minid; + + deadid = lastdeadid ? lastdeadid - 1 : JAIL_MAX; + /* + * Take two reverse passes through the allprison list: first + * starting with the proposed deadid, then ending with it. + */ + for (minid = 1; minid != 0; ) { + TAILQ_FOREACH_REVERSE(dinspr, &allprison, prisonlist, pr_list) { + if (dinspr->pr_id > deadid) + continue; + if (dinspr->pr_id < deadid) { + /* Found an opening. */ + minid = 0; + break; + } + if (--deadid < minid) { + if (lastdeadid == minid || lastdeadid == 0) + { + /* + * The entire legal range + * has been traversed + */ + return 0; + } + /* Try again from the end. */ + deadid = JAIL_MAX; + minid = lastdeadid; + break; + } + } + if (dinspr == NULL) { + /* Found room at the beginning of the list. */ + break; + } + } + *dinsprp = dinspr; + lastdeadid = deadid; + return (deadid); +} + /* * struct jail_get_args { * struct iovec *iovp; diff --git a/sys/sys/jail.h b/sys/sys/jail.h index fb8858f73453..6e7b6cc9ad6a 100644 --- a/sys/sys/jail.h +++ b/sys/sys/jail.h @@ -99,7 +99,7 @@ enum prison_state { #define JAIL_UPDATE 0x02 /* Update parameters of existing jail */ #define JAIL_ATTACH 0x04 /* Attach to jail upon creation */ #define JAIL_DYING 0x08 /* Allow getting a dying jail */ -#define JAIL_SET_MASK 0x0f +#define JAIL_SET_MASK 0x0f /* JAIL_DYING is deprecated/ignored here */ #define JAIL_GET_MASK 0x08 #define JAIL_SYS_DISABLE 0 diff --git a/usr.sbin/jail/jail.8 b/usr.sbin/jail/jail.8 index 21f395609bb5..1f745caa5e7c 100644 --- a/usr.sbin/jail/jail.8 +++ b/usr.sbin/jail/jail.8 @@ -23,7 +23,7 @@ .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF .\" SUCH DAMAGE. .\" -.Dd September 1, 2023 +.Dd November 29, 2023 .Dt JAIL 8 .Os .Sh NAME @@ -144,10 +144,6 @@ jail if it does exist. .Pp Other available options are: .Bl -tag -width indent -.It Fl d -Allow making changes to a dying jail, equivalent to the -.Va allow.dying -parameter. .It Fl f Ar conf_file Use configuration file .Ar conf_file @@ -215,6 +211,17 @@ parameter. .It Fl v Print a message on every operation, such as running commands and mounting filesystems. +.It Fl d +This is deprecated and is equivalent to the +.Va allow.dying +parameter, which is also deprecated. +It used to allow making changes to a +.Va dying +jail. +Now such jails are always replaced when a new jail is created with the same +.Va jid +or +.Va name . .El .Pp If no arguments are given after the options, the operation (except @@ -955,9 +962,14 @@ filesystem on the chrooted .Pa /proc directory. .It Va allow.dying -Allow making changes to a +This is deprecated and has no effect. +It used to allow making changes to a .Va dying jail. +Now such jails are always replaced when a new jail is created with the same +.Va jid +or +.Va name . .It Va depend Specify a jail (or jails) that this jail depends on. When this jail is to be created, any jail(s) it depends on must already exist. diff --git a/usr.sbin/jail/jail.c b/usr.sbin/jail/jail.c index db5918a32744..ad9505353f63 100644 --- a/usr.sbin/jail/jail.c +++ b/usr.sbin/jail/jail.c @@ -61,7 +61,7 @@ const char *separator = "\t"; static void clear_persist(struct cfjail *j); static int update_jail(struct cfjail *j); static int rdtun_params(struct cfjail *j, int dofail); -static void running_jid(struct cfjail *j, int dflag); +static void running_jid(struct cfjail *j); static void jail_quoted_warnx(const struct cfjail *j, const char *name_msg, const char *noname_msg); static int jailparam_set_note(const struct cfjail *j, struct jailparam *jp, @@ -137,7 +137,7 @@ main(int argc, char **argv) const char *cfname; size_t sysvallen; unsigned op, pi; - int ch, docf, error, i, oldcl, sysval; + int ch, docf, dying_warned, error, i, oldcl, sysval; int dflag, eflag, Rflag; #if defined(INET) || defined(INET6) char *cs, *ncs; @@ -377,6 +377,7 @@ main(int argc, char **argv) * operation on it. When that is done, the jail may be finished, * or it may go back for the next step. */ + dying_warned = 0; while ((j = next_jail())) { if (j->flags & JF_FAILED) { @@ -397,11 +398,13 @@ main(int argc, char **argv) import_params(j) < 0) continue; } + if (j->intparams[IP_ALLOW_DYING] && !dying_warned) { + warnx("%s", "the 'allow.dying' parameter and '-d' flag" + "are deprecated and have no effect."); + dying_warned = 1; + } if (!j->jid) - running_jid(j, - (j->flags & (JF_SET | JF_DEPEND)) == JF_SET - ? dflag || bool_param(j->intparams[IP_ALLOW_DYING]) - : 0); + running_jid(j); if (finish_command(j)) continue; @@ -615,9 +618,9 @@ create_jail(struct cfjail *j) { struct iovec jiov[4]; struct stat st; - struct jailparam *jp, *setparams, *setparams2, *sjp; + struct jailparam *jp, *setparams, *sjp; const char *path; - int dopersist, ns, jid, dying, didfail; + int dopersist, ns; /* * Check the jail's path, with a better error message than jail_set @@ -657,57 +660,8 @@ create_jail(struct cfjail *j) *sjp++ = *jp; ns = sjp - setparams; - didfail = 0; j->jid = jailparam_set_note(j, setparams, ns, JAIL_CREATE); - if (j->jid < 0 && errno == EEXIST && - bool_param(j->intparams[IP_ALLOW_DYING]) && - int_param(j->intparams[KP_JID], &jid) && jid != 0) { - /* - * The jail already exists, but may be dying. - * Make sure it is, in which case an update is appropriate. - */ - jiov[0].iov_base = __DECONST(char *, "jid"); - jiov[0].iov_len = sizeof("jid"); - jiov[1].iov_base = &jid; - jiov[1].iov_len = sizeof(jid); - jiov[2].iov_base = __DECONST(char *, "dying"); - jiov[2].iov_len = sizeof("dying"); - jiov[3].iov_base = &dying; - jiov[3].iov_len = sizeof(dying); - if (jail_get(jiov, 4, JAIL_DYING) < 0) { - /* - * It could be that the jail just barely finished - * dying, or it could be that the jid never existed - * but the name does. In either case, another try - * at creating the jail should do the right thing. - */ - if (errno == ENOENT) - j->jid = jailparam_set_note(j, setparams, ns, - JAIL_CREATE); - } else if (dying) { - j->jid = jid; - if (rdtun_params(j, 1) < 0) { - j->jid = -1; - didfail = 1; - } else { - sjp = setparams2 = alloca((j->njp + dopersist) * - sizeof(struct jailparam)); - for (jp = setparams; jp < setparams + ns; jp++) - if (!JP_RDTUN(jp) || - !strcmp(jp->jp_name, "jid")) - *sjp++ = *jp; - j->jid = jailparam_set_note(j, setparams2, - sjp - setparams2, JAIL_UPDATE | JAIL_DYING); - /* - * Again, perhaps the jail just finished dying. - */ - if (j->jid < 0 && errno == ENOENT) - j->jid = jailparam_set_note(j, - setparams, ns, JAIL_CREATE); - } - } - } - if (j->jid < 0 && !didfail) { + if (j->jid < 0) { jail_warnx(j, "%s", jail_errmsg); failed(j); } @@ -772,9 +726,7 @@ update_jail(struct cfjail *j) if (!JP_RDTUN(jp)) *++sjp = *jp; - jid = jailparam_set_note(j, setparams, ns, - bool_param(j->intparams[IP_ALLOW_DYING]) - ? JAIL_UPDATE | JAIL_DYING : JAIL_UPDATE); + jid = jailparam_set_note(j, setparams, ns, JAIL_UPDATE); if (jid < 0) { jail_warnx(j, "%s", jail_errmsg); failed(j); @@ -815,8 +767,7 @@ rdtun_params(struct cfjail *j, int dofail) rtjp->jp_value = NULL; } rval = 0; - if (jailparam_get(rtparams, nrt, - bool_param(j->intparams[IP_ALLOW_DYING]) ? JAIL_DYING : 0) > 0) { + if (jailparam_get(rtparams, nrt, 0) > 0) { rtjp = rtparams + 1; for (jp = j->jp; rtjp < rtparams + nrt; jp++) { if (JP_RDTUN(jp) && strcmp(jp->jp_name, "jid")) { @@ -863,7 +814,7 @@ rdtun_params(struct cfjail *j, int dofail) * Get the jail's jid if it is running. */ static void -running_jid(struct cfjail *j, int dflag) +running_jid(struct cfjail *j) { struct iovec jiov[2]; const char *pval; @@ -889,7 +840,7 @@ running_jid(struct cfjail *j, int dflag) j->jid = -1; return; } - j->jid = jail_get(jiov, 2, dflag ? JAIL_DYING : 0); + j->jid = jail_get(jiov, 2, 0); } static void @@ -918,10 +869,9 @@ jailparam_set_note(const struct cfjail *j, struct jailparam *jp, unsigned njp, jid = jailparam_set(jp, njp, flags); if (verbose > 0) { - jail_note(j, "jail_set(%s%s)", + jail_note(j, "jail_set(%s)", (flags & (JAIL_CREATE | JAIL_UPDATE)) == JAIL_CREATE - ? "JAIL_CREATE" : "JAIL_UPDATE", - (flags & JAIL_DYING) ? " | JAIL_DYING" : ""); + ? "JAIL_CREATE" : "JAIL_UPDATE"); for (i = 0; i < njp; i++) { printf(" %s", jp[i].jp_name); if (jp[i].jp_value == NULL)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202311300022.3AU0Mb3x034136>