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

[Alan Somers]

On Fri, Nov 16, 2018 at 7:16 PM James Gritton <jamie at freebsd.org> wrote:

> On 2018-11-16 16:30, Alan Somers wrote:
>
> On Fri, Nov 16, 2018 at 2:28 PM James Gritton <jamie at freebsd.org> wrote:
>
>> On 2018-11-16 10:34, Alan Somers wrote:
>>
>> On Fri, May 4, 2018 at 2:54 PM Jamie Gritton <jamie at 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