[PATCH 0/4] listmount changes

[PATCH 0/4] listmount changes

[Miklos Szeredi]

This came out from me thinking about the best libc API.  It contains a few
changes that simplify and (I think) improve the interface. 

Tree:

  git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git#vfs.mount
  commit 229dc17d71b0 ("listmount: guard against speculation")

Miklos Szeredi (4):
  listmount: rip out flags
  listmount: list mounts in ID order
  listmount: small changes in semantics
  listmount: allow continuing

 fs/namespace.c             | 93 +++++++++++++++-----------------------
 include/uapi/linux/mount.h | 13 ++++--
 2 files changed, 45 insertions(+), 61 deletions(-)

-- 
2.41.0

[PATCH 1/4] listmount: rip out flags

[Miklos Szeredi]

LISTMOUNT_UNREACHABLE will be achieved differently in a following patch.

LISTMOUNT_RECURSIVE becomes the default.  If non-recursive listing turns
out to be needed, it can be added later.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/namespace.c             | 49 +++++++++++++-------------------------
 include/uapi/linux/mount.h |  4 ----
 2 files changed, 16 insertions(+), 37 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index cb338ab18db9..9b4cb25c25ed 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5004,18 +5004,13 @@ static struct mount *listmnt_first(struct mount *root)
 	return list_first_entry_or_null(&root->mnt_mounts, struct mount, mnt_child);
 }
 
-static struct mount *listmnt_next(struct mount *curr, struct mount *root, bool recurse)
+static struct mount *listmnt_next(struct mount *curr, struct mount *root)
 {
-	if (recurse)
-		return next_mnt(curr, root);
-	if (!list_is_head(curr->mnt_child.next, &root->mnt_mounts))
-		return list_next_entry(curr, mnt_child);
-	return NULL;
+	return next_mnt(curr, root);
 }
 
 static ssize_t do_listmount(struct vfsmount *mnt, u64 __user *buf,
-			    size_t bufsize, const struct path *root,
-			    unsigned int flags)
+			    size_t bufsize, const struct path *root)
 {
 	struct mount *r, *m = real_mount(mnt);
 	struct path rootmnt = {
@@ -5023,26 +5018,17 @@ static ssize_t do_listmount(struct vfsmount *mnt, u64 __user *buf,
 		.dentry = root->mnt->mnt_root
 	};
 	ssize_t ctr;
-	bool reachable_only = true;
-	bool recurse = flags & LISTMOUNT_RECURSIVE;
 	int err;
 
-	if (flags & LISTMOUNT_UNREACHABLE) {
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
-		reachable_only = false;
-	}
-
-	if (reachable_only && !is_path_reachable(m, mnt->mnt_root, &rootmnt))
+	if (!is_path_reachable(m, mnt->mnt_root, &rootmnt))
 		return capable(CAP_SYS_ADMIN) ? 0 : -EPERM;
 
 	err = security_sb_statfs(mnt->mnt_root);
 	if (err)
 		return err;
 
-	for (ctr = 0, r = listmnt_first(m); r; r = listmnt_next(r, m, recurse)) {
-		if (reachable_only &&
-		    !is_path_reachable(r, r->mnt.mnt_root, root))
+	for (ctr = 0, r = listmnt_first(m); r; r = listmnt_next(r, m)) {
+		if (!is_path_reachable(r, r->mnt.mnt_root, root))
 			continue;
 
 		if (ctr >= bufsize)
@@ -5065,7 +5051,7 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
 	u64 mnt_id;
 	ssize_t ret;
 
-	if (flags & ~(LISTMOUNT_UNREACHABLE | LISTMOUNT_RECURSIVE))
+	if (flags)
 		return -EINVAL;
 
 	if (copy_from_user(&kreq, req, sizeof(kreq)))
@@ -5075,20 +5061,17 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
 	mnt_id = kreq.mnt_id;
 
 	down_read(&namespace_sem);
-	if (mnt_id == LSMT_ROOT)
-		mnt = &current->nsproxy->mnt_ns->root->mnt;
-	else
-		mnt = lookup_mnt_in_ns(mnt_id, current->nsproxy->mnt_ns);
-	if (!mnt) {
-		up_read(&namespace_sem);
-		return -ENOENT;
-	}
-
 	get_fs_root(current->fs, &root);
-	/* Skip unreachable for LSMT_ROOT */
-	if (mnt_id == LSMT_ROOT && !(flags & LISTMOUNT_UNREACHABLE))
+	if (mnt_id == LSMT_ROOT) {
 		mnt = root.mnt;
-	ret = do_listmount(mnt, buf, bufsize, &root, flags);
+	} else {
+		ret = -ENOENT;
+		mnt = lookup_mnt_in_ns(mnt_id, current->nsproxy->mnt_ns);
+		if (!mnt)
+			goto err;
+	}
+	ret = do_listmount(mnt, buf, bufsize, &root);
+err:
 	path_put(&root);
 	up_read(&namespace_sem);
 	return ret;
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index 7a5bd0b24a62..f6b35a15b7dd 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -191,10 +191,6 @@ struct mnt_id_req {
 #define STATMOUNT_MNT_POINT		0x00000010U	/* Want/got mnt_point */
 #define STATMOUNT_FS_TYPE		0x00000020U	/* Want/got fs_type */
 
-/* listmount(2) flags */
-#define LISTMOUNT_UNREACHABLE	0x01U	/* List unreachable mounts too */
-#define LISTMOUNT_RECURSIVE	0x02U	/* List a mount tree */
-
 /*
  * Special @mnt_id values that can be passed to listmount
  */
-- 
2.41.0

[PATCH 2/4] listmount: list mounts in ID order

[Miklos Szeredi]

This is needed to allow continuing from a midpoint.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/namespace.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 9b4cb25c25ed..ad62cf7ee334 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1010,7 +1010,7 @@ void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp, struct m
 
 static inline struct mount *node_to_mount(struct rb_node *node)
 {
-	return rb_entry(node, struct mount, mnt_node);
+	return node ? rb_entry(node, struct mount, mnt_node) : NULL;
 }
 
 static void mnt_add_to_ns(struct mnt_namespace *ns, struct mount *mnt)
@@ -4999,24 +4999,21 @@ SYSCALL_DEFINE4(statmount, const struct mnt_id_req __user *, req,
 	return ret;
 }
 
-static struct mount *listmnt_first(struct mount *root)
+static struct mount *listmnt_next(struct mount *curr)
 {
-	return list_first_entry_or_null(&root->mnt_mounts, struct mount, mnt_child);
+	return node_to_mount(rb_next(&curr->mnt_node));
 }
 
-static struct mount *listmnt_next(struct mount *curr, struct mount *root)
-{
-	return next_mnt(curr, root);
-}
-
-static ssize_t do_listmount(struct vfsmount *mnt, u64 __user *buf,
-			    size_t bufsize, const struct path *root)
+static ssize_t do_listmount(struct mount *first, struct vfsmount *mnt,
+			    u64 __user *buf, size_t bufsize,
+			    const struct path *root)
 {
 	struct mount *r, *m = real_mount(mnt);
 	struct path rootmnt = {
 		.mnt = root->mnt,
 		.dentry = root->mnt->mnt_root
 	};
+	struct path orig;
 	ssize_t ctr;
 	int err;
 
@@ -5027,8 +5024,17 @@ static ssize_t do_listmount(struct vfsmount *mnt, u64 __user *buf,
 	if (err)
 		return err;
 
-	for (ctr = 0, r = listmnt_first(m); r; r = listmnt_next(r, m)) {
-		if (!is_path_reachable(r, r->mnt.mnt_root, root))
+	if (root->mnt == mnt) {
+		orig = *root;
+	} else {
+		orig.mnt = mnt;
+		orig.dentry = mnt->mnt_root;
+	}
+
+	for (ctr = 0, r = first; r; r = listmnt_next(r)) {
+		if (r == m)
+			continue;
+		if (!is_path_reachable(r, r->mnt.mnt_root, &orig))
 			continue;
 
 		if (ctr >= bufsize)
@@ -5045,8 +5051,10 @@ static ssize_t do_listmount(struct vfsmount *mnt, u64 __user *buf,
 SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
 		u64 __user *, buf, size_t, bufsize, unsigned int, flags)
 {
+	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
 	struct mnt_id_req kreq;
 	struct vfsmount *mnt;
+	struct mount *first;
 	struct path root;
 	u64 mnt_id;
 	ssize_t ret;
@@ -5066,11 +5074,13 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
 		mnt = root.mnt;
 	} else {
 		ret = -ENOENT;
-		mnt = lookup_mnt_in_ns(mnt_id, current->nsproxy->mnt_ns);
+		mnt  = lookup_mnt_in_ns(mnt_id, ns);
 		if (!mnt)
 			goto err;
 	}
-	ret = do_listmount(mnt, buf, bufsize, &root);
+	first = node_to_mount(rb_first(&ns->mounts));
+
+	ret = do_listmount(first, mnt, buf, bufsize, &root);
 err:
 	path_put(&root);
 	up_read(&namespace_sem);
-- 
2.41.0

[PATCH 3/4] listmount: small changes in semantics

[Miklos Szeredi]

1) Make permission checking consistent with statmount(2): fail if mount is
unreachable from current root.  Previously it failed if mount was
unreachable from root->mnt->mnt_root.

2) List all submounts, even if unreachable from current root.  This is
safe, since 1) will prevent listing unreachable mounts for unprivileged
users.

3) LSMT_ROOT is unchaged, it lists mounts under current root.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/namespace.c | 39 ++++++++++++++-------------------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index ad62cf7ee334..10cd651175b5 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5004,37 +5004,26 @@ static struct mount *listmnt_next(struct mount *curr)
 	return node_to_mount(rb_next(&curr->mnt_node));
 }
 
-static ssize_t do_listmount(struct mount *first, struct vfsmount *mnt,
+static ssize_t do_listmount(struct mount *first, struct path *orig, u64 mnt_id,
 			    u64 __user *buf, size_t bufsize,
 			    const struct path *root)
 {
-	struct mount *r, *m = real_mount(mnt);
-	struct path rootmnt = {
-		.mnt = root->mnt,
-		.dentry = root->mnt->mnt_root
-	};
-	struct path orig;
+	struct mount *r;
 	ssize_t ctr;
 	int err;
 
-	if (!is_path_reachable(m, mnt->mnt_root, &rootmnt))
-		return capable(CAP_SYS_ADMIN) ? 0 : -EPERM;
+	if (!capable(CAP_SYS_ADMIN) &&
+	    !is_path_reachable(real_mount(orig->mnt), orig->dentry, root))
+		return -EPERM;
 
-	err = security_sb_statfs(mnt->mnt_root);
+	err = security_sb_statfs(orig->dentry);
 	if (err)
 		return err;
 
-	if (root->mnt == mnt) {
-		orig = *root;
-	} else {
-		orig.mnt = mnt;
-		orig.dentry = mnt->mnt_root;
-	}
-
 	for (ctr = 0, r = first; r; r = listmnt_next(r)) {
-		if (r == m)
+		if (r->mnt_id_unique == mnt_id)
 			continue;
-		if (!is_path_reachable(r, r->mnt.mnt_root, &orig))
+		if (!is_path_reachable(r, r->mnt.mnt_root, orig))
 			continue;
 
 		if (ctr >= bufsize)
@@ -5053,9 +5042,8 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
 {
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
 	struct mnt_id_req kreq;
-	struct vfsmount *mnt;
 	struct mount *first;
-	struct path root;
+	struct path root, orig;
 	u64 mnt_id;
 	ssize_t ret;
 
@@ -5071,16 +5059,17 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
 	down_read(&namespace_sem);
 	get_fs_root(current->fs, &root);
 	if (mnt_id == LSMT_ROOT) {
-		mnt = root.mnt;
+		orig = root;
 	} else {
 		ret = -ENOENT;
-		mnt  = lookup_mnt_in_ns(mnt_id, ns);
-		if (!mnt)
+		orig.mnt  = lookup_mnt_in_ns(mnt_id, ns);
+		if (!orig.mnt)
 			goto err;
+		orig.dentry = orig.mnt->mnt_root;
 	}
 	first = node_to_mount(rb_first(&ns->mounts));
 
-	ret = do_listmount(first, mnt, buf, bufsize, &root);
+	ret = do_listmount(first, &orig, mnt_id, buf, bufsize, &root);
 err:
 	path_put(&root);
 	up_read(&namespace_sem);
-- 
2.41.0

[PATCH 4/4] listmount: allow continuing

[Miklos Szeredi]

Rename mnt_id_req.request_mask to .param to allow using it for listmount(2)
as well.

1) If the buffer is full don't return EOVERFLOW, instead return the buffer
size.  This still allows detecting a full buffer.

2) listing is continued after the ID contained in .param.  This allows
listing the mount IDs in multiple listmount() invocations without having to
resize buffer.  If .param is zero, then the listing is started from the
beginning, just like previously.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/namespace.c             | 17 ++++++++---------
 include/uapi/linux/mount.h |  9 ++++++++-
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 10cd651175b5..5c1455c4b53b 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -4942,7 +4942,7 @@ static int prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
 		return -EFAULT;
 
 	*ks = (struct kstatmount){
-		.mask		= kreq->request_mask,
+		.mask		= kreq->param,
 		.buf		= buf,
 		.bufsize	= bufsize,
 		.seq = {
@@ -5020,14 +5020,11 @@ static ssize_t do_listmount(struct mount *first, struct path *orig, u64 mnt_id,
 	if (err)
 		return err;
 
-	for (ctr = 0, r = first; r; r = listmnt_next(r)) {
+	for (ctr = 0, r = first; r && ctr < bufsize; r = listmnt_next(r)) {
 		if (r->mnt_id_unique == mnt_id)
 			continue;
 		if (!is_path_reachable(r, r->mnt.mnt_root, orig))
 			continue;
-
-		if (ctr >= bufsize)
-			return -EOVERFLOW;
 		ctr = array_index_nospec(ctr, bufsize);
 		if (put_user(r->mnt_id_unique, buf + ctr))
 			return -EFAULT;
@@ -5044,7 +5041,7 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
 	struct mnt_id_req kreq;
 	struct mount *first;
 	struct path root, orig;
-	u64 mnt_id;
+	u64 mnt_id, last_mnt_id;
 	ssize_t ret;
 
 	if (flags)
@@ -5052,9 +5049,8 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
 
 	if (copy_from_user(&kreq, req, sizeof(kreq)))
 		return -EFAULT;
-	if (kreq.request_mask != 0)
-		return -EINVAL;
 	mnt_id = kreq.mnt_id;
+	last_mnt_id = kreq.param;
 
 	down_read(&namespace_sem);
 	get_fs_root(current->fs, &root);
@@ -5067,7 +5063,10 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
 			goto err;
 		orig.dentry = orig.mnt->mnt_root;
 	}
-	first = node_to_mount(rb_first(&ns->mounts));
+	if (!last_mnt_id)
+		first = node_to_mount(rb_first(&ns->mounts));
+	else
+		first = mnt_find_id_at(ns, last_mnt_id + 1);
 
 	ret = do_listmount(first, &orig, mnt_id, buf, bufsize, &root);
 err:
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index f6b35a15b7dd..dc9a0112d819 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -176,9 +176,16 @@ struct statmount {
 	char str[];		/* Variable size part containing strings */
 };
 
+/*
+ * Structure for passing mount ID and miscellaneous parameters to statmount(2)
+ * and listmount(2).
+ *
+ * For statmount(2) @param represents the request mask.
+ * For listmount(2) @param represents the last listed mount id (or zero).
+ */
 struct mnt_id_req {
 	__u64 mnt_id;
-	__u64 request_mask;
+	__u64 param;
 };
 
 /*
-- 
2.41.0

Re: [PATCH 0/4] listmount changes

[Christian Brauner]

On Tue, 28 Nov 2023 17:03:31 +0100, Miklos Szeredi wrote:
> This came out from me thinking about the best libc API.  It contains a few
> changes that simplify and (I think) improve the interface.
> 
> Tree:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git#vfs.mount
> 
> [...]

Afaict, all changes as discussed. Thanks. I folded the fixes into the
main commit. Links to the patches that were folded are in the commit
message and explained in there as well. The final commit is now rather
small and easy to read.

---

Applied to the vfs.mount branch of the vfs/vfs.git tree.
Patches in the vfs.mount branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.mount

[1/4] listmount: rip out flags
      https://git.kernel.org/vfs/vfs/c/735b1c55d14a (folded)
[2/4] listmount: list mounts in ID order
      https://git.kernel.org/vfs/vfs/c/735b1c55d14a (folded)
[3/4] listmount: small changes in semantics
      https://git.kernel.org/vfs/vfs/c/735b1c55d14a (folded)
[4/4] listmount: allow continuing
      https://git.kernel.org/vfs/vfs/c/735b1c55d14a (folded)

Re: [PATCH 0/4] listmount changes

[Miklos Szeredi]

On Wed, 29 Nov 2023 at 10:53, Christian Brauner <brauner@kernel.org> wrote:
>
> On Tue, 28 Nov 2023 17:03:31 +0100, Miklos Szeredi wrote:
> > This came out from me thinking about the best libc API.  It contains a few
> > changes that simplify and (I think) improve the interface.
> >
> > Tree:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git#vfs.mount
> >
> > [...]
>
> Afaict, all changes as discussed. Thanks. I folded the fixes into the
> main commit. Links to the patches that were folded are in the commit
> message and explained in there as well. The final commit is now rather
> small and easy to read.

Looks good, thanks for folding the patches.

>    * Remove explicit LISTMOUNT_UNREACHABLE flag (cf. [1]). That
>      functionality can simply be made available by checking for required
>      privileges. If the caller is sufficiently privileged then list mounts
>      that can't be reached from the current root. If the caller isn't skip
>      mounts that can't be reached from the current root. This also makes
>      permission checking consistent with statmount() (cf. [3]).

Skipping mounts based on privileges was what the initial version did.
That inconsistency was the reason for introducing
LISTMOUNT_UNREACHABLE.  The final version doesn't skip mounts based on
privileges, either all submounts are listed or the request is rejected
with -EPERM.

For the case when some submounts are inside root and some are outside
useing LSMT_ROOT should be sufficient.  LSMT_ROOT won't fail due to
insufficient privileges, since by definition it lists only mounts that
are below root.

Thanks,
Miklos

Re: [PATCH 0/4] listmount changes

[Christian Brauner]

On Wed, Nov 29, 2023 at 11:22:03AM +0100, Miklos Szeredi wrote:
> On Wed, 29 Nov 2023 at 10:53, Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Tue, 28 Nov 2023 17:03:31 +0100, Miklos Szeredi wrote:
> > > This came out from me thinking about the best libc API.  It contains a few
> > > changes that simplify and (I think) improve the interface.
> > >
> > > Tree:
> > >
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git#vfs.mount
> > >
> > > [...]
> >
> > Afaict, all changes as discussed. Thanks. I folded the fixes into the
> > main commit. Links to the patches that were folded are in the commit
> > message and explained in there as well. The final commit is now rather
> > small and easy to read.
> 
> Looks good, thanks for folding the patches.
> 
> >    * Remove explicit LISTMOUNT_UNREACHABLE flag (cf. [1]). That
> >      functionality can simply be made available by checking for required
> >      privileges. If the caller is sufficiently privileged then list mounts
> >      that can't be reached from the current root. If the caller isn't skip
> >      mounts that can't be reached from the current root. This also makes
> >      permission checking consistent with statmount() (cf. [3]).
> 
> Skipping mounts based on privileges was what the initial version did.
> That inconsistency was the reason for introducing
> LISTMOUNT_UNREACHABLE.  The final version doesn't skip mounts based on
> privileges, either all submounts are listed or the request is rejected
> with -EPERM.

Yeah, I phrased that badly. What I meant to convey is that mounts not
reachable from the current root are not reported as in skipped in the
loop. I've simplified this down to:

* Remove explicit LISTMOUNT_UNREACHABLE flag (cf. [1]) and fail if mount
  is unreachable from current root. This also makes permission checking
  consistent with statmount() (cf. [3]).

Re: [PATCH 3/4] listmount: small changes in semantics

[Serge E. Hallyn]

On Tue, Nov 28, 2023 at 05:03:34PM +0100, Miklos Szeredi wrote:
> 1) Make permission checking consistent with statmount(2): fail if mount is
> unreachable from current root.  Previously it failed if mount was
> unreachable from root->mnt->mnt_root.
> 
> 2) List all submounts, even if unreachable from current root.  This is
> safe, since 1) will prevent listing unreachable mounts for unprivileged
> users.
> 
> 3) LSMT_ROOT is unchaged, it lists mounts under current root.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/namespace.c | 39 ++++++++++++++-------------------------
>  1 file changed, 14 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index ad62cf7ee334..10cd651175b5 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -5004,37 +5004,26 @@ static struct mount *listmnt_next(struct mount *curr)
>  	return node_to_mount(rb_next(&curr->mnt_node));
>  }
>  
> -static ssize_t do_listmount(struct mount *first, struct vfsmount *mnt,
> +static ssize_t do_listmount(struct mount *first, struct path *orig, u64 mnt_id,
>  			    u64 __user *buf, size_t bufsize,
>  			    const struct path *root)
>  {
> -	struct mount *r, *m = real_mount(mnt);
> -	struct path rootmnt = {
> -		.mnt = root->mnt,
> -		.dentry = root->mnt->mnt_root
> -	};
> -	struct path orig;
> +	struct mount *r;
>  	ssize_t ctr;
>  	int err;
>  
> -	if (!is_path_reachable(m, mnt->mnt_root, &rootmnt))
> -		return capable(CAP_SYS_ADMIN) ? 0 : -EPERM;
> +	if (!capable(CAP_SYS_ADMIN) &&

Was there a reason to do the capable check first?  In general,
checking capable() when not needed is frowned upon, as it will
set the PF_SUPERPRIV flag.

> +	    !is_path_reachable(real_mount(orig->mnt), orig->dentry, root))
> +		return -EPERM;
>  
> -	err = security_sb_statfs(mnt->mnt_root);
> +	err = security_sb_statfs(orig->dentry);
>  	if (err)
>  		return err;
>  
> -	if (root->mnt == mnt) {
> -		orig = *root;
> -	} else {
> -		orig.mnt = mnt;
> -		orig.dentry = mnt->mnt_root;
> -	}
> -
>  	for (ctr = 0, r = first; r; r = listmnt_next(r)) {
> -		if (r == m)
> +		if (r->mnt_id_unique == mnt_id)
>  			continue;
> -		if (!is_path_reachable(r, r->mnt.mnt_root, &orig))
> +		if (!is_path_reachable(r, r->mnt.mnt_root, orig))
>  			continue;
>  
>  		if (ctr >= bufsize)
> @@ -5053,9 +5042,8 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
>  {
>  	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
>  	struct mnt_id_req kreq;
> -	struct vfsmount *mnt;
>  	struct mount *first;
> -	struct path root;
> +	struct path root, orig;
>  	u64 mnt_id;
>  	ssize_t ret;
>  
> @@ -5071,16 +5059,17 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
>  	down_read(&namespace_sem);
>  	get_fs_root(current->fs, &root);
>  	if (mnt_id == LSMT_ROOT) {
> -		mnt = root.mnt;
> +		orig = root;
>  	} else {
>  		ret = -ENOENT;
> -		mnt  = lookup_mnt_in_ns(mnt_id, ns);
> -		if (!mnt)
> +		orig.mnt  = lookup_mnt_in_ns(mnt_id, ns);
> +		if (!orig.mnt)
>  			goto err;
> +		orig.dentry = orig.mnt->mnt_root;
>  	}
>  	first = node_to_mount(rb_first(&ns->mounts));
>  
> -	ret = do_listmount(first, mnt, buf, bufsize, &root);
> +	ret = do_listmount(first, &orig, mnt_id, buf, bufsize, &root);
>  err:
>  	path_put(&root);
>  	up_read(&namespace_sem);
> -- 
> 2.41.0
> 

Re: [PATCH 3/4] listmount: small changes in semantics

[Miklos Szeredi]

On Wed, 6 Dec 2023 at 20:58, Serge E. Hallyn <serge@hallyn.com> wrote:
>
> On Tue, Nov 28, 2023 at 05:03:34PM +0100, Miklos Szeredi wrote:

> > -     if (!is_path_reachable(m, mnt->mnt_root, &rootmnt))
> > -             return capable(CAP_SYS_ADMIN) ? 0 : -EPERM;
> > +     if (!capable(CAP_SYS_ADMIN) &&
>
> Was there a reason to do the capable check first?  In general,
> checking capable() when not needed is frowned upon, as it will
> set the PF_SUPERPRIV flag.
>

I synchronized the permission checking with statmount() without
thinking about the order.   I guess we can change the order back in
both syscalls?

I also don't understand the reason behind the using the _noaudit()
variant.  Christian?

Thanks,
Miklos

Re: [PATCH 3/4] listmount: small changes in semantics

[Christian Brauner]

On Wed, Dec 06, 2023 at 09:24:45PM +0100, Miklos Szeredi wrote:
> On Wed, 6 Dec 2023 at 20:58, Serge E. Hallyn <serge@hallyn.com> wrote:
> >
> > On Tue, Nov 28, 2023 at 05:03:34PM +0100, Miklos Szeredi wrote:
> 
> > > -     if (!is_path_reachable(m, mnt->mnt_root, &rootmnt))
> > > -             return capable(CAP_SYS_ADMIN) ? 0 : -EPERM;
> > > +     if (!capable(CAP_SYS_ADMIN) &&
> >
> > Was there a reason to do the capable check first?  In general,
> > checking capable() when not needed is frowned upon, as it will
> > set the PF_SUPERPRIV flag.
> >
> 
> I synchronized the permission checking with statmount() without
> thinking about the order.   I guess we can change the order back in
> both syscalls?

I can just change the order. It's mostly a question of what is more
expensive. If there's such unpleasant side-effects... then sure I'll
reorder.

> I also don't understand the reason behind the using the _noaudit()
> variant.  Christian?

The reasoning is similar to the change in commit e7eda157c407 ("fs:
don't audit the capability check in simple_xattr_list()").

    "The check being unconditional may lead to unwanted denials reported by
    LSMs when a process has the capability granted by DAC, but denied by an
    LSM. In the case of SELinux such denials are a problem, since they can't
    be effectively filtered out via the policy and when not silenced, they
    produce noise that may hide a true problem or an attack."

So for system calls like listmount() that we can expect to be called a
lot of times (findmnt etc at some point) this would needlessly spam
dmesg without any value. We can always change that to an explicit
capable() later.