Bug 255660 - /etc/defaults/devfs.rules rule recursion
Summary: /etc/defaults/devfs.rules rule recursion
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: conf (show other bugs)
Version: 13.0-RELEASE
Hardware: Any Any
: --- Affects Many People
Assignee: Kristof Provost
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-05-06 15:13 UTC by Gijs Peskens
Modified: 2021-11-29 10:01 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Gijs Peskens 2021-05-06 15:13:38 UTC
The default [devfsrules_jail_vnet=5] includes devfsrules_jail, however include recursion is not something that exists.
I believe the intention was to have all rules of ruleset 4 expanded, including the included rules.

So it should read like this:
[devfsrules_jail_vnet=5]
add include $devfsrules_hide_all
add include $devfsrules_unhide_basic
add include $devfsrules_unhide_login
add include $devfsrules_jail
add path pf unhide
Comment 1 Zhenlei Huang 2021-11-03 09:13:29 UTC
Steps to repeated:

1. mkdir -p /tmp/dev
2. mount -t devfs devfs /tmp/dev
3. devfs -m /tmp/dev rule -s 5 applyset
4. ls /tmp/dev
Comment 2 Zhenlei Huang 2021-11-03 10:10:53 UTC
Code review: https://reviews.freebsd.org/D32814
Comment 3 commit-hook freebsd_committer 2021-11-03 12:51:11 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=7acd322ebe2072b1d73b1d19c14ab12a300ba8e8

commit 7acd322ebe2072b1d73b1d19c14ab12a300ba8e8
Author:     Zhenlei Huang <zlei.huang@gmail.com>
AuthorDate: 2021-11-03 11:46:48 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2021-11-03 11:50:13 +0000

    devfs.rules: Correctly unhide pf in vnet jails

    Revision 9e9be081d8 introduced a new devfs rule devfsrules_jail_vnet. It
    includes rule devfsrules_jail which include other rules. Unfortunately
    devfs could not recursively parse the action include and thus
    devfsrules_jail_vnet will expose all nodes.

    PR:             255660
    Reviewed by:    kp
    Obtained from:  Gijs Peskens <gijs@peskens.net>
    MFC after:      3 weeks
    Differential Revision:  https://reviews.freebsd.org/D32814

 sbin/devfs/devfs.rules | 3 +++
 1 file changed, 3 insertions(+)
Comment 4 commit-hook freebsd_committer 2021-11-24 16:59:27 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=eaab06d53dfa7fbf926a2c19aa00a6804b5e6349

commit eaab06d53dfa7fbf926a2c19aa00a6804b5e6349
Author:     Zhenlei Huang <zlei.huang@gmail.com>
AuthorDate: 2021-11-03 11:46:48 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2021-11-24 16:45:51 +0000

    devfs.rules: Correctly unhide pf in vnet jails

    Revision 9e9be081d8 introduced a new devfs rule devfsrules_jail_vnet. It
    includes rule devfsrules_jail which include other rules. Unfortunately
    devfs could not recursively parse the action include and thus
    devfsrules_jail_vnet will expose all nodes.

    PR:             255660
    Reviewed by:    kp
    Obtained from:  Gijs Peskens <gijs@peskens.net>
    MFC after:      3 weeks
    Differential Revision:  https://reviews.freebsd.org/D32814

    (cherry picked from commit 7acd322ebe2072b1d73b1d19c14ab12a300ba8e8)

 sbin/devfs/devfs.rules | 3 +++
 1 file changed, 3 insertions(+)
Comment 5 Dmitry Wagin 2021-11-27 18:09:18 UTC
(In reply to Gijs Peskens from comment #0)

Use sysctl vfs.devfs.rule_depth=2
Comment 6 Dmitry Wagin 2021-11-27 18:10:58 UTC
(In reply to Dmitry Wagin from comment #5)

maybee need change default 1 -> 2
Comment 7 Zhenlei Huang 2021-11-29 09:18:38 UTC
(In reply to Dmitry Wagin from comment #5)
Emmm, DEVFS(8) says:
>     include ruleset  Apply all the rules in ruleset number ruleset to the
>                      node.  This does not necessarily result in any changes
>                      to the node (e.g., if none of the rules in the included
>                      ruleset match).  Include commands in the referenced
>                      ruleset are not resolved.

sadly the sysctl variable `vfs.devfs.rule_depth` is not well documented :(
Comment 8 Zhenlei Huang 2021-11-29 09:48:57 UTC
(In reply to Dmitry Wagin from comment #6)
> maybee need change default 1 -> 2

This need justification.

If the default is changed from 1 to 2, then it should be documented explicitly and we should give a good reason why it is not change from 1 to 3. There're cases user customize local devfs.rules and reference existing default ruleset. The default 2 would confuse end users.

Maybe it is better we step further to default allow recursive `include`, and the kernel checks and prevents DAG cycles, ruleset A -> B -> A, i.e.
Comment 9 Zhenlei Huang 2021-11-29 10:01:58 UTC
(In reply to Zhenlei Huang from comment #8)

> kernel checks and prevents DAG cycles, ruleset A -> B -> A, i.e.

Sorry the "prevents DAG cycles" is misleading. I meant "prevents DG cycles".