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

@@ -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

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,70 +1436,39 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags)
 				continue;
 			if (inspr->pr_id > jid)
 				break;
+			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) {
+		if (cuflags == JAIL_CREATE && 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",
+			vfs_opterror(opts, "jail %d already exists", jid);
-				    jid);
 			goto done_deref;
 		}
-			if (!prison_ischild(mypr, pr)) {
+		if ((pr == NULL)
+		    ? cuflags == JAIL_UPDATE
+		    : !prison_ischild(mypr, pr)) {
 			/*
-				 * Updaters get ENOENT if they cannot see the
+			 * Updaters get ENOENT for nonexistent jails,
-				 * jail.  This is true even for CREATE | UPDATE,
+			 * 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;
 		}
-			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 the caller provided a name, look for a jail by that name.
@@ -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 &&
+				if (tpr == pr || !prison_isalive(tpr) ||
-				    !strcmp(tpr->pr_name + pnamelen, namelc)) {
+				    strcmp(tpr->pr_name + pnamelen, namelc))
-					if (prison_isalive(tpr)) {
+					continue;
-						if (pr == NULL &&
+				if (cuflags == JAIL_CREATE || pr != NULL) {
-						    cuflags != JAIL_CREATE) {
 					/*
-							 * Use this jail
+					 * Create, or update(jid): name must
-							 * for updates.
+					 * not exist in an active sibling jail.
 					 */
+					error = EEXIST;
+					vfs_opterror(opts,
+					    "jail \"%s\" already exists", name);
+					goto done_deref;
+				}
+				/* Use this jail for updates. */
 				pr = tpr;
 				mtx_lock(&pr->pr_mtx);
 				drflags |= PD_LOCKED;
 				break;
 			}
 			/*
-						 * Create, or update(jid):
+			 * Update: name must exist if no jid is specified.
-						 * name must not exist in an
+			 * As with the jid case, the jail must be currently
-						 * active sibling jail.
+			 * visible, or else even CREATE | UPDATE will get
+			 * an error.
 			 */
-						error = EEXIST;
+			if ((pr == NULL)
-						vfs_opterror(opts,
+			    ? cuflags == JAIL_UPDATE
-						   "jail \"%s\" already exists",
+			    : !prison_isalive(pr)) {
-						   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;
-					vfs_opterror(opts,
-					    "jail \"%s\" is dying", name);
-					goto done_deref;
-				}
-			}
-			/* Update: name must exist if no jid. */
-			else if (cuflags == JAIL_UPDATE && pr == NULL) {
 				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
+			 * This may be a new prison's first user reference,
-			 * to label it as such until after OSD calls have had
+			 * but wait to call it alive until after OSD calls
-			 * a chance to run (and perhaps to fail).
+			 * 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;

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

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.

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,
+			running_jid(j);
-			    (j->flags & (JF_SET | JF_DEPEND)) == JF_SET
-			    ? dflag || bool_param(j->intparams[IP_ALLOW_DYING])
-			    : 0);
 		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 &&
+	if (j->jid < 0) {
-	    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) {
 		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,
+	jid = jailparam_set_note(j, setparams, ns, JAIL_UPDATE);
-	    bool_param(j->intparams[IP_ALLOW_DYING])
-	    ? JAIL_UPDATE | JAIL_DYING : 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,
+	if (jailparam_get(rtparams, nrt, 0) > 0) {
-	    bool_param(j->intparams[IP_ALLOW_DYING]) ? JAIL_DYING : 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",
+		    ? "JAIL_CREATE" : "JAIL_UPDATE");
-		    (flags & JAIL_DYING) ? " | JAIL_DYING" : "");
 		for (i = 0; i < njp; i++) {
 			printf(" %s", jp[i].jp_name);
 			if (jp[i].jp_value == NULL)