Date: Mon, 19 Nov 2018 10:26:13 -0700 From: Alan Somers <asomers@freebsd.org> To: jamie@freebsd.org Cc: src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org>, Ross Williams <ross@ross-williams.net> Subject: Re: svn commit: r333263 - in head: lib/libjail sys/cddl/contrib/opensolaris/uts/common/fs/zfs sys/compat/linprocfs sys/compat/linsysfs sys/fs/devfs sys/fs/fdescfs sys/fs/nullfs sys/fs/procfs sys/fs/pse... Message-ID: <CAOtMX2gtnRq38TAThgNRtk7EPJknvoTfQK6x9yBu28FWJH_7LA@mail.gmail.com> In-Reply-To: <c9cc4b86158ff55c12e10da94de95127@freebsd.org> References: <201805042054.w44KsRtc038808@repo.freebsd.org> <CAOtMX2jBiyRm_bZ%2B9OohqZXK%2Bxq--q24p4MWpjudRc=HKQUmrg@mail.gmail.com> <f379b6c84ad858d39ec96bf470f928b2@freebsd.org> <CAOtMX2jEKAVJNYX2mJt9FnB=VngaWB=%2BwDXSg2hFD6rO7bFG4A@mail.gmail.com> <c9cc4b86158ff55c12e10da94de95127@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Nov 16, 2018 at 7:16 PM James Gritton <jamie@freebsd.org> wrote: > On 2018-11-16 16:30, Alan Somers wrote: > > On Fri, Nov 16, 2018 at 2:28 PM James Gritton <jamie@freebsd.org> wrote: > >> On 2018-11-16 10:34, Alan Somers wrote: >> >> On Fri, May 4, 2018 at 2:54 PM Jamie Gritton <jamie@freebsd.org> wrote: >> >>> Author: jamie >>> Date: Fri May 4 20:54:27 2018 >>> New Revision: 333263 >>> URL: https://svnweb.freebsd.org/changeset/base/333263 >>> >>> Log: >>> Make it easier for filesystems to count themselves as jail-enabled, >>> by doing most of the work in a new function prison_add_vfs in >>> kern_jail.c >>> Now a jail-enabled filesystem need only mark itself with VFCF_JAIL, and >>> the rest is taken care of. This includes adding a jail parameter like >>> allow.mount.foofs, and a sysctl like security.jail.mount_foofs_allowed. >>> Both of these used to be a static list of known filesystems, with >>> predefined permission bits. >>> >>> Reviewed by: kib >>> Differential Revision: D14681 >>> >>> Modified: >>> head/lib/libjail/jail.c >>> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c >>> head/sys/compat/linprocfs/linprocfs.c >>> head/sys/compat/linsysfs/linsysfs.c >>> head/sys/fs/devfs/devfs_vfsops.c >>> head/sys/fs/fdescfs/fdesc_vfsops.c >>> head/sys/fs/nullfs/null_vfsops.c >>> head/sys/fs/procfs/procfs.c >>> head/sys/fs/pseudofs/pseudofs.h >>> head/sys/fs/tmpfs/tmpfs_vfsops.c >>> head/sys/kern/kern_jail.c >>> head/sys/kern/vfs_init.c >>> head/sys/kern/vfs_mount.c >>> head/sys/kern/vfs_subr.c >>> head/sys/sys/jail.h >>> head/sys/sys/mount.h >>> head/usr.sbin/jail/jail.8 >>> >>> Modified: head/lib/libjail/jail.c >>> >>> ============================================================================== >>> --- head/lib/libjail/jail.c Fri May 4 20:38:26 2018 (r333262) >>> +++ head/lib/libjail/jail.c Fri May 4 20:54:27 2018 (r333263) >>> @@ -1048,7 +1048,13 @@ kldload_param(const char *name) >>> else if (strcmp(name, "sysvmsg") == 0 || strcmp(name, "sysvsem") >>> == 0 || >>> strcmp(name, "sysvshm") == 0) >>> kl = kldload(name); >>> - else { >>> + else if (strncmp(name, "allow.mount.", 12) == 0) { >>> + /* Load the matching filesystem */ >>> + kl = kldload(name + 12); >>> + if (kl < 0 && errno == ENOENT && >>> + strncmp(name + 12, "no", 2) == 0) >>> + kl = kldload(name + 14); >>> + } else { >>> errno = ENOENT; >>> return (-1); >>> } >>> >> I'm curious about this part of the change. Why is it necessary to load >> the module in the "allow.mount.noXXXfs" case, when the jail is forbidden to >> mount the filesystem? It seems like that would just load modules that >> aren't going to be used. >> Additional discussion at https://github.com/iocage/iocage/issues/689 . >> -Alan >> >> Presumably such a parameter would be included in some jails in >> conjunction with the positive being included in others (perhaps as a >> default). The truth is I never really considered whether the "no" option >> would be used, I just always treat these option as pairs. >> It may be reasonable (at least in the allow.mount.* case) to silently >> disregard a "no" option that doesn't exist, but I don't know how many >> places would need to be modified for that to go smoothly. Though I don't >> expect that there would be too many people who bother to include a jail >> parameter about a filesystem which they're not planning to use. >> - Jamie >> > > Well, many people use the "no" option because one of the most popular jail > managers, iocage, uses it under the hood. But since "no" is the default, > its presence on the command line is a noop. Are there any situations in > which the "no" option has an effect? The only two possibilities I could > think of were: > > 1) Somebody puts both the positive and negative options on the same > command line. From experiment, it seems like the last option takes > effect. In this case, the presence of the positive option would cause the > kld to be loaded, regardless of the presence of the negative option. > 2) When using hierarchical jails, it might make sense to use the positive > option for the outer jail and the negative option for the inner jail. But > this would only be important if the inner jail inherited the outer jail's > parameters, which doesn't seem to be the case. > > So I can't think of any reason to continue to mount the kld for "no" > options. Can you? > > > 3) There's allow.mount.foofs as a global parameter, with some jails > overriding that with a jail-specific allow.mount.nofoofs. In that case, > KLD loading shouldn't be a problem as global parameters typically come > first. > > It makes sense not to load a KLD for a "no" option, as long as that option > is then silently ignored. I wouldn't want it to error out with "unknown > parameter". > See also https://github.com/iocage/iocage/issues/689
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2gtnRq38TAThgNRtk7EPJknvoTfQK6x9yBu28FWJH_7LA>