[RFC 0/3] fs: kill old ms_* flags for internal sb

[RFC 0/3] fs: kill old ms_* flags for internal sb

[Luis Chamberlain]

David had started the sb flag split for internal flags through
commit e462ec50cb5 ("VFS: Differentiate mount flags (MS_*) from internal
superblock flags") but it seems we just never axed out the old flag
usage.

I found this while inspecting adding a new temporary flag for the
superblock for filesystem freeze support. This doesn't go even build
tested, hence RFC.

Luis Chamberlain (3):
  apparmor: use SB_* flags for private sb flags
  fs: use SB_NOUSER on path_mount() instead of deprecated MS_NOUSER
  fs: remove old MS_* internal flags for the superblock

 fs/namespace.c                    | 2 +-
 include/uapi/linux/mount.h        | 8 --------
 security/apparmor/include/mount.h | 3 ++-
 security/apparmor/lsm.c           | 1 +
 security/apparmor/mount.c         | 2 +-
 tools/include/uapi/linux/mount.h  | 8 --------
 6 files changed, 5 insertions(+), 19 deletions(-)

-- 
2.35.1

[RFC 1/3] apparmor: use SB_* flags for private sb flags

[Luis Chamberlain]

Commit 2ea3ffb7782 ("apparmor: add mount mediation") John Johansen
added mount mediation support. However just the day before this commit
David Howells modified the internal sb flags through commit e462ec50cb5
("VFS: Differentiate mount flags (MS_*) from internal superblock flags").

Use the modified sb flags to make things clear and avoid further uses
of the old MS_* flags for superblock internal flags. This will let us
later remove the MS_* sb internal flags as userspace should not be
using them.

This commit does not fix anything as the old flags used map to the
same bitmask, this just tidies things up. I split up the flags to
make it clearer which ones are for the superblock and used internally.

Cc: John Johansen <john.johansen@canonical.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 security/apparmor/include/mount.h | 3 ++-
 security/apparmor/lsm.c           | 1 +
 security/apparmor/mount.c         | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/apparmor/include/mount.h b/security/apparmor/include/mount.h
index a710683b2496..f90e03405e38 100644
--- a/security/apparmor/include/mount.h
+++ b/security/apparmor/include/mount.h
@@ -23,7 +23,8 @@
 #define AA_AUDIT_DATA		0x40
 #define AA_MNT_CONT_MATCH	0x40
 
-#define AA_MS_IGNORE_MASK (MS_KERNMOUNT | MS_NOSEC | MS_ACTIVE | MS_BORN)
+#define AA_MS_IGNORE_MASK (MS_KERNMOUNT)
+#define AA_SB_IGNORE_MASK (SB_NOSEC | SB_ACTIVE | SB_BORN)
 
 int aa_remount(struct aa_label *label, const struct path *path,
 	       unsigned long flags, void *data);
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index c6728a629437..f3880956bffd 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -583,6 +583,7 @@ static int apparmor_sb_mount(const char *dev_name, const struct path *path,
 		flags &= ~MS_MGC_MSK;
 
 	flags &= ~AA_MS_IGNORE_MASK;
+	flags &= ~AA_SB_IGNORE_MASK;
 
 	label = __begin_current_label_crit_section();
 	if (!unconfined(label)) {
diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c
index cdfa430ae216..c37c451e8226 100644
--- a/security/apparmor/mount.c
+++ b/security/apparmor/mount.c
@@ -74,7 +74,7 @@ static void audit_mnt_flags(struct audit_buffer *ab, unsigned long flags)
 		audit_log_format(ab, ", iversion");
 	if (flags & MS_STRICTATIME)
 		audit_log_format(ab, ", strictatime");
-	if (flags & MS_NOUSER)
+	if (flags & SB_NOUSER)
 		audit_log_format(ab, ", nouser");
 }
 
-- 
2.35.1

Re: [RFC 1/3] apparmor: use SB_* flags for private sb flags

[Al Viro]

On Mon, Jan 09, 2023 at 06:25:52PM -0800, Luis Chamberlain wrote:
> Commit 2ea3ffb7782 ("apparmor: add mount mediation") John Johansen
> added mount mediation support. However just the day before this commit
> David Howells modified the internal sb flags through commit e462ec50cb5
> ("VFS: Differentiate mount flags (MS_*) from internal superblock flags").
> 
> Use the modified sb flags to make things clear and avoid further uses
> of the old MS_* flags for superblock internal flags. This will let us
> later remove the MS_* sb internal flags as userspace should not be
> using them.
> 
> This commit does not fix anything as the old flags used map to the
> same bitmask, this just tidies things up. I split up the flags to
> make it clearer which ones are for the superblock and used internally.

I don't think that's right.  apparmor_sb_mount() gets (almost) raw flags
from mount(2); incidentally, MS_MGC_MSK removal directly above the modified
line is BS since _that_ has already been done by the caller.

Note that the same function explicitly checks for MS_MOVE, etc. in the
same argument.

> @@ -74,7 +74,7 @@ static void audit_mnt_flags(struct audit_buffer *ab, unsigned long flags)
>  		audit_log_format(ab, ", iversion");
>  	if (flags & MS_STRICTATIME)
>  		audit_log_format(ab, ", strictatime");
> -	if (flags & MS_NOUSER)
> +	if (flags & SB_NOUSER)
>  		audit_log_format(ab, ", nouser");
>  }

Umm...  How does one trigger that one?

[RFC 2/3] fs: use SB_NOUSER on path_mount() instead of deprecated MS_NOUSER

[Luis Chamberlain]

The goal behind 462ec50cb5 ("VFS: Differentiate mount flags (MS_*) from
internal superblock flags") was to phase out MS_* users for internal
uses. But we can't remove the old MS_* until we have all users out so
just use the SB_* helper for this check.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 fs/namespace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index ab467ee58341..bf1cc8527057 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3411,7 +3411,7 @@ int path_mount(const char *dev_name, struct path *path,
 	if (data_page)
 		((char *)data_page)[PAGE_SIZE - 1] = 0;
 
-	if (flags & MS_NOUSER)
+	if (flags & SB_NOUSER)
 		return -EINVAL;
 
 	ret = security_sb_mount(dev_name, path, type_page, flags, data_page);
-- 
2.35.1

Re: [RFC 2/3] fs: use SB_NOUSER on path_mount() instead of deprecated MS_NOUSER

[Al Viro]

On Mon, Jan 09, 2023 at 06:25:53PM -0800, Luis Chamberlain wrote:
> The goal behind 462ec50cb5 ("VFS: Differentiate mount flags (MS_*) from
> internal superblock flags") was to phase out MS_* users for internal
> uses. But we can't remove the old MS_* until we have all users out so
> just use the SB_* helper for this check.

No.  The goal had been to separate the places where we deal with
mount(2) argument encoding from those where we are deal with superblock
flags.  path_mount() is very much in the former class.

[RFC 3/3] fs: remove old MS_* internal flags for the superblock

[Luis Chamberlain]

During commit e462ec50cb5 ("VFS: Differentiate mount flags (MS_*) from
internal superblock flags") Christoph had suggested we should eventually
remove these old flags which were exposed to userspace but could not
be used as they were internal-only.

Nuke them.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 include/uapi/linux/mount.h       | 8 --------
 tools/include/uapi/linux/mount.h | 8 --------
 2 files changed, 16 deletions(-)

diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index 4d93967f8aea..eb6617a5426b 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -38,14 +38,6 @@
 #define MS_STRICTATIME	(1<<24) /* Always perform atime updates */
 #define MS_LAZYTIME	(1<<25) /* Update the on-disk [acm]times lazily */
 
-/* These sb flags are internal to the kernel */
-#define MS_SUBMOUNT     (1<<26)
-#define MS_NOREMOTELOCK	(1<<27)
-#define MS_NOSEC	(1<<28)
-#define MS_BORN		(1<<29)
-#define MS_ACTIVE	(1<<30)
-#define MS_NOUSER	(1<<31)
-
 /*
  * Superblock flags that can be altered by MS_REMOUNT
  */
diff --git a/tools/include/uapi/linux/mount.h b/tools/include/uapi/linux/mount.h
index 4d93967f8aea..eb6617a5426b 100644
--- a/tools/include/uapi/linux/mount.h
+++ b/tools/include/uapi/linux/mount.h
@@ -38,14 +38,6 @@
 #define MS_STRICTATIME	(1<<24) /* Always perform atime updates */
 #define MS_LAZYTIME	(1<<25) /* Update the on-disk [acm]times lazily */
 
-/* These sb flags are internal to the kernel */
-#define MS_SUBMOUNT     (1<<26)
-#define MS_NOREMOTELOCK	(1<<27)
-#define MS_NOSEC	(1<<28)
-#define MS_BORN		(1<<29)
-#define MS_ACTIVE	(1<<30)
-#define MS_NOUSER	(1<<31)
-
 /*
  * Superblock flags that can be altered by MS_REMOUNT
  */
-- 
2.35.1

Re: [RFC 3/3] fs: remove old MS_* internal flags for the superblock

[Al Viro]

On Mon, Jan 09, 2023 at 06:25:54PM -0800, Luis Chamberlain wrote:
> During commit e462ec50cb5 ("VFS: Differentiate mount flags (MS_*) from
> internal superblock flags") Christoph had suggested we should eventually
> remove these old flags which were exposed to userspace but could not
> be used as they were internal-only.
> 
> Nuke them.

Umm...  They are still exposed to userland after your series, though.
IOW, we can't change the bit assignment of e.g. SB_ACTIVE - the apparmor
part will break, etc.

If anything, it would be more honest to lift the "we ignore those bits"
logics to path_mount()...