| Message ID | 20190104192648.GO2217@ZenIV.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [git,pull] vfs.git mount.part1 | expand |
Al Viro <viro@zeniv.linux.org.uk> writes: > mount API prereqs. Mostly that's LSM mount options cleanups. > One trivial conflict in security/selinux/hooks.c, resolved by taking > the variant from this branch - the method has been split, leaving > only the part that used to be conditional upon "it's not an internal > mount" and check has been moved into the caller of the remaining piece. > The last commit in this pile ("mount_fs: suppress MAC on MS_SUBMOUNT as > well as MS_KERNMOUNT") is an equivalent of the conflict-creating > mainline change. > There are several minor fixes in there, but nothing > earth-shattering (leaks on failure exits, mostly). Am I completely blind or has this code never been posted for reivew? I know you stuffed something in linux-next, and I saw quite a bit of consternation from several of the security folks at the time when testing on linux-next started reporting failures. Currently on the linux-security-module list there is an unanswered question by Casey Schaufler <casey@schaufler-ca.com> of which tree this work is being performed on. I know I asked you explicitly when you would be posting this code for review and did not get an answer. I know I saw bug activity on your tree right up to the day Linus made his stable release. I keeping an eye out in the hopes that there would be something to review. Not having had a chance to review this code I can't really comment on the quality of this code. What I do know from a glance is that you have not removed FS_BINARY_MOUNTDATA. Which is the root cause of some of the crazy security mount option processing, and is an if not greater mess than what the security options have been doing with mount options. The FS_BINARY_MOUNTDATA flag is only relevant for coda and for nfs backwards compatiblity. The FS_BINARY_MOUNTDATA flag is only set on btrfs to allow calling mount_subtree. I have a set of patches that is finally reasonablly stable and cleans up all of the mess in the current internal mount apis that should allow implementing the new mount api to be much less error prone. My plan is to start posting it as soon as the merge window closes. I would really appreciate it if we can work together on this the next development cycle, and not try and rush and get things out and merged without proper review, and care. That has already led to problems. Eric > The following changes since commit 651022382c7f8da46cb4872a545ee1da6d097d2a: > > Linux 4.20-rc1 (2018-11-04 15:37:52 -0800) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git mount.part1 > > for you to fetch changes up to 718c43038f287e843c2f63d946977de90014cb11: > > mount_fs: suppress MAC on MS_SUBMOUNT as well as MS_KERNMOUNT (2018-12-21 11:51:23 -0500) > > ---------------------------------------------------------------- > Al Viro (25): > exofs_mount(): fix leaks on failure exits > selinux: expand superblock_doinit() calls > smack: make smack_parse_opts_str() clean up on failure > LSM: lift parsing LSM options into the caller of ->sb_kern_mount() > LSM: lift extracting and parsing LSM options into the caller of ->sb_remount() > new helper: security_sb_eat_lsm_opts() > LSM: split ->sb_set_mnt_opts() out of ->sb_kern_mount() > selinux; don't open-code a loop in sb_finish_set_opts() > btrfs: sanitize security_mnt_opts use > nfs_remount(): don't leak, don't ignore LSM options quietly > LSM: turn sb_eat_lsm_opts() into a method > selinux: kill selinux_sb_get_mnt_opts() > LSM: hide struct security_mnt_opts from any generic code > selinux: switch to private struct selinux_mnt_opts > smack: switch to private smack_mnt_opts > LSM: bury struct security_mnt_opts > selinux: new helper - selinux_add_opt() > selinux: switch away from match_token() > selinux: regularize Opt_... names a bit > selinux: rewrite selinux_sb_eat_lsm_opts() > LSM: new method: ->sb_add_mnt_opt() > smack: take the guts of smack_parse_opts_str() into a new helper > smack: get rid of match_token() > smack: rewrite smack_sb_eat_lsm_opts() > mount_fs: suppress MAC on MS_SUBMOUNT as well as MS_KERNMOUNT > > David Howells (2): > vfs: Suppress MS_* flag defs within the kernel unless explicitly enabled > vfs: Separate changing mount flags full remount > > arch/arc/kernel/setup.c | 1 + > arch/arm/kernel/atags_parse.c | 1 + > arch/sh/kernel/setup.c | 1 + > arch/sparc/kernel/setup_32.c | 1 + > arch/sparc/kernel/setup_64.c | 1 + > arch/x86/kernel/setup.c | 1 + > drivers/base/devtmpfs.c | 1 + > fs/btrfs/ctree.h | 4 - > fs/btrfs/super.c | 82 +---- > fs/exofs/super.c | 37 +- > fs/namespace.c | 156 ++++++--- > fs/nfs/internal.h | 2 +- > fs/nfs/super.c | 34 +- > fs/pnode.c | 1 + > fs/super.c | 24 +- > include/linux/lsm_hooks.h | 17 +- > include/linux/mount.h | 2 +- > include/linux/security.h | 82 +---- > include/uapi/linux/fs.h | 56 +-- > include/uapi/linux/mount.h | 58 +++ > init/do_mounts.c | 1 + > init/do_mounts_initrd.c | 1 + > security/apparmor/lsm.c | 1 + > security/apparmor/mount.c | 1 + > security/security.c | 39 ++- > security/selinux/hooks.c | 799 ++++++++++++++++-------------------------- > security/smack/smack_lsm.c | 359 ++++++++----------- > security/tomoyo/mount.c | 1 + > 28 files changed, 724 insertions(+), 1040 deletions(-) > create mode 100644 include/uapi/linux/mount.h
On Sat, Jan 05, 2019 at 01:31:21PM -0600, Eric W. Biederman wrote: > Not having had a chance to review this code I can't really comment on > the quality of this code. What I do know from a glance is that > you have not removed FS_BINARY_MOUNTDATA. Which is the root cause > of some of the crazy security mount option processing, and is an if > not greater mess than what the security options have been doing with > mount options. > > The FS_BINARY_MOUNTDATA flag is only relevant for coda and for nfs > backwards compatiblity. The FS_BINARY_MOUNTDATA flag is only set on > btrfs to allow calling mount_subtree. ... and thus it can't be killed without having dragged the NFS pile into the entire thing. > I have a set of patches that is finally reasonablly stable and cleans up > all of the mess in the current internal mount apis that should allow > implementing the new mount api to be much less error prone. Quick question: how do you deal with the differences in quoting for selinux options and for everything else? I've no problem with working with you, now that you've resurfaced. Fair warning: no promises of accepting your solutions. Along with a promise to reject anything that breaks existing setups, which your earlier proposals did. With NFS among the victims, IIRC.
The pull request you sent on Fri, 4 Jan 2019 19:26:48 +0000:
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git mount.part1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/505b050fdf42097883b2d37b8e796e1f11dbef50
Thank you!
mount API prereqs. Mostly that's LSM mount options cleanups. One trivial conflict in security/selinux/hooks.c, resolved by taking the variant from this branch - the method has been split, leaving only the part that used to be conditional upon "it's not an internal mount" and check has been moved into the caller of the remaining piece. The last commit in this pile ("mount_fs: suppress MAC on MS_SUBMOUNT as well as MS_KERNMOUNT") is an equivalent of the conflict-creating mainline change. There are several minor fixes in there, but nothing earth-shattering (leaks on failure exits, mostly). The following changes since commit 651022382c7f8da46cb4872a545ee1da6d097d2a: Linux 4.20-rc1 (2018-11-04 15:37:52 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git mount.part1 for you to fetch changes up to 718c43038f287e843c2f63d946977de90014cb11: mount_fs: suppress MAC on MS_SUBMOUNT as well as MS_KERNMOUNT (2018-12-21 11:51:23 -0500) ---------------------------------------------------------------- Al Viro (25): exofs_mount(): fix leaks on failure exits selinux: expand superblock_doinit() calls smack: make smack_parse_opts_str() clean up on failure LSM: lift parsing LSM options into the caller of ->sb_kern_mount() LSM: lift extracting and parsing LSM options into the caller of ->sb_remount() new helper: security_sb_eat_lsm_opts() LSM: split ->sb_set_mnt_opts() out of ->sb_kern_mount() selinux; don't open-code a loop in sb_finish_set_opts() btrfs: sanitize security_mnt_opts use nfs_remount(): don't leak, don't ignore LSM options quietly LSM: turn sb_eat_lsm_opts() into a method selinux: kill selinux_sb_get_mnt_opts() LSM: hide struct security_mnt_opts from any generic code selinux: switch to private struct selinux_mnt_opts smack: switch to private smack_mnt_opts LSM: bury struct security_mnt_opts selinux: new helper - selinux_add_opt() selinux: switch away from match_token() selinux: regularize Opt_... names a bit selinux: rewrite selinux_sb_eat_lsm_opts() LSM: new method: ->sb_add_mnt_opt() smack: take the guts of smack_parse_opts_str() into a new helper smack: get rid of match_token() smack: rewrite smack_sb_eat_lsm_opts() mount_fs: suppress MAC on MS_SUBMOUNT as well as MS_KERNMOUNT David Howells (2): vfs: Suppress MS_* flag defs within the kernel unless explicitly enabled vfs: Separate changing mount flags full remount arch/arc/kernel/setup.c | 1 + arch/arm/kernel/atags_parse.c | 1 + arch/sh/kernel/setup.c | 1 + arch/sparc/kernel/setup_32.c | 1 + arch/sparc/kernel/setup_64.c | 1 + arch/x86/kernel/setup.c | 1 + drivers/base/devtmpfs.c | 1 + fs/btrfs/ctree.h | 4 - fs/btrfs/super.c | 82 +---- fs/exofs/super.c | 37 +- fs/namespace.c | 156 ++++++--- fs/nfs/internal.h | 2 +- fs/nfs/super.c | 34 +- fs/pnode.c | 1 + fs/super.c | 24 +- include/linux/lsm_hooks.h | 17 +- include/linux/mount.h | 2 +- include/linux/security.h | 82 +---- include/uapi/linux/fs.h | 56 +-- include/uapi/linux/mount.h | 58 +++ init/do_mounts.c | 1 + init/do_mounts_initrd.c | 1 + security/apparmor/lsm.c | 1 + security/apparmor/mount.c | 1 + security/security.c | 39 ++- security/selinux/hooks.c | 799 ++++++++++++++++-------------------------- security/smack/smack_lsm.c | 359 ++++++++----------- security/tomoyo/mount.c | 1 + 28 files changed, 724 insertions(+), 1040 deletions(-) create mode 100644 include/uapi/linux/mount.h