proposed libc interface and man page for statmount(2)

proposed libc interface and man page for statmount(2)

[Miklos Szeredi]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

Hi,

Attaching the proposed man page for the new statmount() syscall.

It describes a libc interface that is slightly different from the raw
kernel API.   The differences from the two API's are also described in
the man page.

Raw:

       long syscall(SYS_statmount, const struct mnt_id_req *req,
                    struct statmount *buf, size_t bufsize, unsigned int flags);

Libc:

       struct statmount *statmount(uint64_t mnt_id, uint64_t request_mask,
                                   struct statmount *buf, size_t bufsize,
                                   unsigned int flags);

I propose the libc one to allow automatically allocating the buffer if
the buf argument is NULL, similar to getcwd(3).

Comments?

Thanks,
Miklos

[-- Attachment #2: statmount.2 --]
[-- Type: application/x-troff-man, Size: 6750 bytes --]

Re: proposed libc interface and man page for statmount(2)

[Adhemerval Zanella Netto]

On 15/11/23 12:08, Miklos Szeredi wrote:
> Hi,
> 
> Attaching the proposed man page for the new statmount() syscall.
> 
> It describes a libc interface that is slightly different from the raw
> kernel API.   The differences from the two API's are also described in
> the man page.
> 
> Raw:
> 
>        long syscall(SYS_statmount, const struct mnt_id_req *req,
>                     struct statmount *buf, size_t bufsize, unsigned int flags);
> 
> Libc:
> 
>        struct statmount *statmount(uint64_t mnt_id, uint64_t request_mask,
>                                    struct statmount *buf, size_t bufsize,
>                                    unsigned int flags);
> 
> I propose the libc one to allow automatically allocating the buffer if
> the buf argument is NULL, similar to getcwd(3).

The glibc getcwd implementation allocates a buffer with maximum size
of max(PATH_MAX, getpagesize()) and iff getpwd syscall fails it will 
fallback to a generic implementation that keep calling openat and realloc 
the buf if required.  So for the generic case, it would require malloc
plus realloc (to free some unused memory).

Making statmount similar to getcwd would require something alike, where 
the libc will loop to reallocate the buffer if syscall returns EOVERFLOW.
I am not sure this would be the best interface, come up the initial buffer
size and the increment might be tricky and not ideal for all usage cases.

Maybe setting the initial size depending of request_mask bits, by assuming
a reasonable size for STMT_FS_TYPE and PATH_MAX for STMT_MNT_ROOT/STMT_MNT_POINT
would be a reasonable initial size. 

It also always pull malloc, which is not ideal for the static linking case
since the interface not always return the strings.

Re: proposed libc interface and man page for statmount(2)

[Florian Weimer]

* Miklos Szeredi:

> Hi,
>
> Attaching the proposed man page for the new statmount() syscall.
>
> It describes a libc interface that is slightly different from the raw
> kernel API.   The differences from the two API's are also described in
> the man page.
>
> Raw:
>
>        long syscall(SYS_statmount, const struct mnt_id_req *req,
>                     struct statmount *buf, size_t bufsize, unsigned int flags);
>
> Libc:
>
>        struct statmount *statmount(uint64_t mnt_id, uint64_t request_mask,
>                                    struct statmount *buf, size_t bufsize,
>                                    unsigned int flags);
>
> I propose the libc one to allow automatically allocating the buffer if
> the buf argument is NULL, similar to getcwd(3).

In addition to Adhemerval's observation that we'd prefer to have some
hint regarding the buffer size, it's probably better to have entirely
separate interfaces because it makes static analysis easier.  With a
unified interface, we can still convey the information with an inline
wrapper function, but we can avoid that complexity.

Thanks,
Florian

Re: proposed libc interface and man page for statmount(2)

[Miklos Szeredi]

On Thu, 16 Nov 2023 at 21:36, Florian Weimer <fweimer@redhat.com> wrote:

> In addition to Adhemerval's observation that we'd prefer to have some
> hint regarding the buffer size, it's probably better to have entirely
> separate interfaces because it makes static analysis easier.  With a
> unified interface, we can still convey the information with an inline
> wrapper function, but we can avoid that complexity.

I'm not against having separate allocating and the non-allocating interfaces.

But I don't think the allocating one needs a size hint.   Your
suggestion of passing a buffer on the stack to the syscall and then
copying to an exact sized malloc should take care of it.   If the
stack buffer is sized generously, then the loop will never need to
repeat for any real life case.

Thanks,
Miklos

Re: proposed libc interface and man page for statmount(2)

[Miklos Szeredi]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

On Thu, 16 Nov 2023 at 22:01, Miklos Szeredi <miklos@szeredi.hu> wrote:

> But I don't think the allocating one needs a size hint.   Your
> suggestion of passing a buffer on the stack to the syscall and then
> copying to an exact sized malloc should take care of it.   If the
> stack buffer is sized generously, then the loop will never need to
> repeat for any real life case.

       int statmount(uint64_t mnt_id, uint64_t request_mask,
                   struct statmount *buf, size_t bufsize, unsigned int flags);

       struct statmount *statmount_alloc(uint64_t mnt_id,
                   uint64_t request_mask, unsigned int flags);

Updated man page attached.

Thanks,
Miklos

[-- Attachment #2: statmount.2 --]
[-- Type: application/x-troff-man, Size: 6622 bytes --]

Re: proposed libc interface and man page for statmount(2)

[Florian Weimer]

* Miklos Szeredi:

> On Thu, 16 Nov 2023 at 21:36, Florian Weimer <fweimer@redhat.com> wrote:
>
>> In addition to Adhemerval's observation that we'd prefer to have some
>> hint regarding the buffer size, it's probably better to have entirely
>> separate interfaces because it makes static analysis easier.  With a
>> unified interface, we can still convey the information with an inline
>> wrapper function, but we can avoid that complexity.
>
> I'm not against having separate allocating and the non-allocating
> interfaces.

Thanks.

> But I don't think the allocating one needs a size hint.   Your
> suggestion of passing a buffer on the stack to the syscall and then
> copying to an exact sized malloc should take care of it.   If the
> stack buffer is sized generously, then the loop will never need to
> repeat for any real life case.

The strings could get fairly large if they ever contain key material,
especially for post-quantum cryptography.

We have plenty of experience with these double-buffer-and-retry
interfaces and glibc, and the failure mode once there is much more data
than initially expected can be quite bad.  For new interfaces, I want a
way to avoid that.  At least as long applications use statmount_allloc,
we have a way to switch to a different interface if that becomes
necessary just with a glibc-internal change.

Thanks,
Florian

Re: proposed libc interface and man page for statmount(2)

[Miklos Szeredi]

On Fri, 17 Nov 2023 at 15:47, Florian Weimer <fweimer@redhat.com> wrote:

> The strings could get fairly large if they ever contain key material,
> especially for post-quantum cryptography.

A bit far fetched, but okay.

> We have plenty of experience with these double-buffer-and-retry
> interfaces and glibc, and the failure mode once there is much more data
> than initially expected can be quite bad.  For new interfaces, I want a
> way to avoid that.  At least as long applications use statmount_allloc,
> we have a way to switch to a different interface if that becomes
> necessary just with a glibc-internal change.

Fair enough.

And that brings us to listmount(2) where I'm less sure that the
alloc+retry strategy is the right one.  I still think that a namespace
with millions of mounts is unlikely, but it's not completely out of
the question.   Also a listmount_alloc(3) API is less obvious since
the mount ID array as well as its size needs to be returned.    So I'm
thinking whether it's a good idea to turn this into a
open/list/.../close style of interface in libc?  We could do that in
the kernel as well, but I'm not sure it's worth it at this point.

Thanks,
Miklos

Re: proposed libc interface and man page for statmount(2)

[Miklos Szeredi]

On Fri, 17 Nov 2023 at 16:13, Miklos Szeredi <miklos@szeredi.hu> wrote:

> open/list/.../close style of interface in libc?  We could do that in
> the kernel as well, but I'm not sure it's worth it at this point.

I wonder... Is there a reason this shouldn't be done statelessly by
adding an "continue after this ID" argument to listmount(2)?  The
caller will just need to pass the last mount ID received in the array
to the next listmount(2) call and iterate until a short count is
returned.

Thanks,
Miklos

Re: proposed libc interface and man page for statmount(2)

[Miklos Szeredi]

On Fri, Nov 17, 2023 at 04:50:25PM +0100, Miklos Szeredi wrote:
> I wonder... Is there a reason this shouldn't be done statelessly by
> adding an "continue after this ID" argument to listmount(2)?  The
> caller will just need to pass the last mount ID received in the array
> to the next listmount(2) call and iterate until a short count is
> returned.

No comments so far... maybe more explanation is needed.

New signature of listmount() would be:

ssize_t listmount(uint64_t mnt_id, uint64_t last_mnt_id,
		  uint64_t *buf, size_t bufsize, unsigned int flags);

And the usage would be:

	for (last = 0; nres == bufsize; last = buf[bufsize-1]) {
		nres = listmount(parent, last, buf, bufsize, flags);
		for (i = 0; i < nres; i++) {
			/* process buf[i] */
		}
	}


Here's a kernel patch against the version in Christian's tree.  The syscall
signature doesn't need changing, since we have a spare u64 in the mnt_id_req for
listmount.

The major difference is in the order that the mount ID's are listed, which is
now strictly increasing.  Doing the recursive listing in DFS order is nicer, but
I don't think it's important enough.

Comments?

Thanks,
Miklos

---
 fs/namespace.c             |   41 +++++++++++++++++++++++++----------------
 include/uapi/linux/mount.h |    5 ++++-
 2 files changed, 29 insertions(+), 17 deletions(-)

--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1009,7 +1009,7 @@ void mnt_change_mountpoint(struct mount
 
 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)
@@ -4960,21 +4960,22 @@ SYSCALL_DEFINE4(statmount, const struct
 	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, bool recurse)
+static bool is_submount(struct mount *sub, struct mount *mnt)
 {
-	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;
+	for (; sub != mnt; sub = sub->mnt_parent) {
+		if (sub->mnt_parent == sub)
+			return false;
+	}
+	return true;
 }
 
-static long do_listmount(struct vfsmount *mnt, u64 __user *buf, size_t bufsize,
+static long do_listmount(struct vfsmount *mnt, struct mount *last,
+			 u64 __user *buf, size_t bufsize,
 			 const struct path *root, unsigned int flags)
 {
 	struct mount *r, *m = real_mount(mnt);
@@ -5000,13 +5001,16 @@ static long do_listmount(struct vfsmount
 	if (err)
 		return err;
 
-	for (r = listmnt_first(m); r; r = listmnt_next(r, m, recurse)) {
+	for (r = last; (r = listmnt_next(r)) != NULL && ctr < bufsize;) {
+		if (recurse && !is_submount(r, m))
+			continue;
+		if (!recurse && r->mnt_parent != m)
+			continue;
+
 		if (reachable_only &&
 		    !is_path_reachable(r, r->mnt.mnt_root, root))
 			continue;
 
-		if (ctr >= bufsize)
-			return -EOVERFLOW;
 		if (put_user(r->mnt_id_unique, buf + ctr))
 			return -EFAULT;
 		ctr++;
@@ -5021,6 +5025,7 @@ SYSCALL_DEFINE4(listmount, const struct
 {
 	struct mnt_id_req kreq;
 	struct vfsmount *mnt;
+	struct mount *last;
 	struct path root;
 	u64 mnt_id;
 	long err;
@@ -5030,8 +5035,6 @@ SYSCALL_DEFINE4(listmount, const struct
 
 	if (copy_from_user(&kreq, req, sizeof(kreq)))
 		return -EFAULT;
-	if (kreq.request_mask != 0)
-		return -EINVAL;
 	mnt_id = kreq.mnt_id;
 
 	down_read(&namespace_sem);
@@ -5040,13 +5043,19 @@ SYSCALL_DEFINE4(listmount, const struct
 	else
 		mnt = lookup_mnt_in_ns(mnt_id, current->nsproxy->mnt_ns);
 
+	if (!kreq.last_mnt_id) {
+		last = real_mount(mnt);
+	} else {
+		last = mnt_find_id_at(current->nsproxy->mnt_ns, kreq.last_mnt_id);
+	}
+
 	err = -ENOENT;
 	if (mnt) {
 		get_fs_root(current->fs, &root);
 		/* Skip unreachable for LSMT_ROOT */
 		if (mnt_id == LSMT_ROOT && !(flags & LISTMOUNT_UNREACHABLE))
 			mnt = root.mnt;
-		err = do_listmount(mnt, buf, bufsize, &root, flags);
+		err = do_listmount(mnt, last, buf, bufsize, &root, flags);
 		path_put(&root);
 	}
 	up_read(&namespace_sem);
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -178,7 +178,10 @@ struct statmount {
 
 struct mnt_id_req {
 	__u64 mnt_id;
-	__u64 request_mask;
+	union {
+		__u64 request_mask;
+		__u64 last_mnt_id;
+	};
 };
 
 /*

Re: proposed libc interface and man page for statmount(2)

[Florian Weimer]

* Miklos Szeredi:

> On Fri, Nov 17, 2023 at 04:50:25PM +0100, Miklos Szeredi wrote:
>> I wonder... Is there a reason this shouldn't be done statelessly by
>> adding an "continue after this ID" argument to listmount(2)?  The
>> caller will just need to pass the last mount ID received in the array
>> to the next listmount(2) call and iterate until a short count is
>> returned.
>
> No comments so far... maybe more explanation is needed.
>
> New signature of listmount() would be:
>
> ssize_t listmount(uint64_t mnt_id, uint64_t last_mnt_id,
> 		  uint64_t *buf, size_t bufsize, unsigned int flags);
>
> And the usage would be:
>
> 	for (last = 0; nres == bufsize; last = buf[bufsize-1]) {
> 		nres = listmount(parent, last, buf, bufsize, flags);
> 		for (i = 0; i < nres; i++) {
> 			/* process buf[i] */
> 		}
> 	}

Is the ID something specific to the VFS layer itself, or does it come
from file systems?

POSIX has a seekdir/telldir interface like that, I don't think file
system authors like it.  Some have added dedicated data structures for
it to implement somewhat predictable behavior in the face of concurrent
directory modification.  Would this interface suffer from similar
issues?

Thanks,
Florian

Re: proposed libc interface and man page for statmount(2)

[Miklos Szeredi]

On Mon, Nov 20, 2023 at 01:16:24PM +0100, Florian Weimer wrote:
> Is the ID something specific to the VFS layer itself, or does it come
> from file systems?

It comes from the VFS.


> POSIX has a seekdir/telldir interface like that, I don't think file
> system authors like it.  Some have added dedicated data structures for
> it to implement somewhat predictable behavior in the face of concurrent
> directory modification.  Would this interface suffer from similar
> issues?

The same issue was solved for /proc/$$/mountinfo using cursors.

This patchset removes the need for cursors, since the new unique mount ID can be
used to locate the current position without having to worry about deleted and
added mounts.

OTOH I agree that seekdir/telldir are horrible and it's probably best to at
least hide the seekability behind a stateful API:

	struct lm *lm = listmount_open(mnt_id, flags)
	do {
		num = listmount(lm, buf, bufsize);
		for (i = 0; i < num; i++) {
			/* process buf[i] */
		}
	} while (num > 0);
	listmount_close(lm);

Whether doing that in libc or in the kernel is debatable.  I don't think the
kernel API needs to be stateful, but it may allow more flexibility in the
future.  I don't know.

Thanks,
Miklos

Re: proposed libc interface and man page for statmount(2)

[Christian Brauner]

On Fri, Nov 17, 2023 at 04:13:45PM +0100, Miklos Szeredi wrote:
> On Fri, 17 Nov 2023 at 15:47, Florian Weimer <fweimer@redhat.com> wrote:
> 
> > The strings could get fairly large if they ever contain key material,
> > especially for post-quantum cryptography.
> 
> A bit far fetched, but okay.
> 
> > We have plenty of experience with these double-buffer-and-retry
> > interfaces and glibc, and the failure mode once there is much more data
> > than initially expected can be quite bad.  For new interfaces, I want a
> > way to avoid that.  At least as long applications use statmount_allloc,
> > we have a way to switch to a different interface if that becomes
> > necessary just with a glibc-internal change.
> 
> Fair enough.
> 
> And that brings us to listmount(2) where I'm less sure that the
> alloc+retry strategy is the right one.  I still think that a namespace
> with millions of mounts is unlikely, but it's not completely out of

The limit for the number of mounts per mount namespace is set in
/proc/sys/fs/mount-max. It defaults to 100000. This value is globally
configured and thus can't be changed from e.g., unprivileged containers
but it applies to each mount namespace.

What glibc could do is read that value and cache the result and use that
as a hint or upper limit. That'll waste 800kb potentially but that could
be solved with a cache.

Alternatively you could add a basic listmount() glibc function and
another one that's explicitly there for handling large mount tables.

Re: proposed libc interface and man page for statmount(2)

[Christian Brauner]

On Mon, Nov 20, 2023 at 12:55:17PM +0100, Miklos Szeredi wrote:
> On Fri, Nov 17, 2023 at 04:50:25PM +0100, Miklos Szeredi wrote:
> > I wonder... Is there a reason this shouldn't be done statelessly by
> > adding an "continue after this ID" argument to listmount(2)?  The
> > caller will just need to pass the last mount ID received in the array
> > to the next listmount(2) call and iterate until a short count is
> > returned.
> 
> No comments so far... maybe more explanation is needed.
> 
> New signature of listmount() would be:
> 
> ssize_t listmount(uint64_t mnt_id, uint64_t last_mnt_id,
> 		  uint64_t *buf, size_t bufsize, unsigned int flags);
> 
> And the usage would be:
> 
> 	for (last = 0; nres == bufsize; last = buf[bufsize-1]) {
> 		nres = listmount(parent, last, buf, bufsize, flags);
> 		for (i = 0; i < nres; i++) {
> 			/* process buf[i] */
> 		}
> 	}
> 
> 
> Here's a kernel patch against the version in Christian's tree.  The syscall
> signature doesn't need changing, since we have a spare u64 in the mnt_id_req for
> listmount.
> 
> The major difference is in the order that the mount ID's are listed, which is
> now strictly increasing.  Doing the recursive listing in DFS order is nicer, but
> I don't think it's important enough.
> 
> Comments?

Sure. We can also add a size argument to struct mnt_id_req then you can
version it by size and extend it easily later (see sched_{g,s}etattr()
that do similar things):

struct mnt_id_req {
	__u32 size;
	__u64 mnt_id;
	__u64 request_mask;
	union {
		__u64 request_mask;
		__u64 last_mnt_id;
	};
};

foo(struct mnt_id_req __user *ureq)
{
	u32 size;
	struct mnt_id_req kreq;

	ret = get_user(size, &ureq->size);
	if (ret)
		return ret;

        if (size < MNT_ID_REQ_SIZE_VER0 || size > PAGE_SIZE)
		return -EINVAL;

	ret = copy_struct_from_user(&kreq, sizeof(kreq), ureq, size);
	if (ret)
		return ret;
}

Re: proposed libc interface and man page for statmount(2)

[Ian Kent]

On 20/11/23 20:34, Miklos Szeredi wrote:
> On Mon, Nov 20, 2023 at 01:16:24PM +0100, Florian Weimer wrote:
>> Is the ID something specific to the VFS layer itself, or does it come
>> from file systems?
> It comes from the VFS.
>
>
>> POSIX has a seekdir/telldir interface like that, I don't think file
>> system authors like it.  Some have added dedicated data structures for
>> it to implement somewhat predictable behavior in the face of concurrent
>> directory modification.  Would this interface suffer from similar
>> issues?
> The same issue was solved for /proc/$$/mountinfo using cursors.

The mounts are now using an rb-tree, I think the the cursor solution can

only work for a linear list, the case is very different.


>
> This patchset removes the need for cursors, since the new unique mount ID can be
> used to locate the current position without having to worry about deleted and
> added mounts.

IIRC the problem with proc mounts traversals was because the lock was taken

and dropped between reads so that mount entries could be deleted (not sure

adding had quite the same problem) from the list in between reads.


Sounds like I'll need to look at the code but first though but an rb-tree

can have mounts removed and new mounts inserted if the locks are dropped

if the retrieval is slit between multiple calls.


So I'm struggling to see why this isn't the same problem and I don't think

introducing cursors in this case would work (thankfully, lets do this again

please).


Ian

Re: proposed libc interface and man page for statmount(2)

[Ian Kent]

On 21/11/23 07:56, Ian Kent wrote:
> On 20/11/23 20:34, Miklos Szeredi wrote:
>> On Mon, Nov 20, 2023 at 01:16:24PM +0100, Florian Weimer wrote:
>>> Is the ID something specific to the VFS layer itself, or does it come
>>> from file systems?
>> It comes from the VFS.
>>
>>
>>> POSIX has a seekdir/telldir interface like that, I don't think file
>>> system authors like it.  Some have added dedicated data structures for
>>> it to implement somewhat predictable behavior in the face of concurrent
>>> directory modification.  Would this interface suffer from similar
>>> issues?
>> The same issue was solved for /proc/$$/mountinfo using cursors.
>
> The mounts are now using an rb-tree, I think the the cursor solution can
>
> only work for a linear list, the case is very different.
>
>
>>
>> This patchset removes the need for cursors, since the new unique 
>> mount ID can be
>> used to locate the current position without having to worry about 
>> deleted and
>> added mounts.
>
> IIRC the problem with proc mounts traversals was because the lock was 
> taken
>
> and dropped between reads so that mount entries could be deleted (not 
> sure
>
> adding had quite the same problem) from the list in between reads.
>
>
> Sounds like I'll need to look at the code but first though but an rb-tree
>
> can have mounts removed and new mounts inserted if the locks are dropped
>
> if the retrieval is slit between multiple calls.
>
>
> So I'm struggling to see why this isn't the same problem and I don't 
> think
>
> introducing cursors in this case would work (thankfully, lets do this 
> again
>
> please).

Mmm ... apologies for the poor description above.

That last bit is definitely "lets 'not' do this again please!"


Ian

Re: proposed libc interface and man page for statmount(2)

[Ian Kent]

On 21/11/23 08:58, Ian Kent wrote:
> On 21/11/23 07:56, Ian Kent wrote:
>> On 20/11/23 20:34, Miklos Szeredi wrote:
>>> On Mon, Nov 20, 2023 at 01:16:24PM +0100, Florian Weimer wrote:
>>>> Is the ID something specific to the VFS layer itself, or does it come
>>>> from file systems?
>>> It comes from the VFS.
>>>
>>>
>>>> POSIX has a seekdir/telldir interface like that, I don't think file
>>>> system authors like it.  Some have added dedicated data structures for
>>>> it to implement somewhat predictable behavior in the face of 
>>>> concurrent
>>>> directory modification.  Would this interface suffer from similar
>>>> issues?
>>> The same issue was solved for /proc/$$/mountinfo using cursors.
>>
>> The mounts are now using an rb-tree, I think the the cursor solution can
>>
>> only work for a linear list, the case is very different.
>>
>>
>>>
>>> This patchset removes the need for cursors, since the new unique 
>>> mount ID can be
>>> used to locate the current position without having to worry about 
>>> deleted and
>>> added mounts.
>>
>> IIRC the problem with proc mounts traversals was because the lock was 
>> taken
>>
>> and dropped between reads so that mount entries could be deleted (not 
>> sure
>>
>> adding had quite the same problem) from the list in between reads.
>>
>>
>> Sounds like I'll need to look at the code but first though but an 
>> rb-tree
>>
>> can have mounts removed and new mounts inserted if the locks are dropped
>>
>> if the retrieval is slit between multiple calls.
>>
>>
>> So I'm struggling to see why this isn't the same problem and I don't 
>> think
>>
>> introducing cursors in this case would work (thankfully, lets do this 
>> again
>>
>> please).
>
> Mmm ... apologies for the poor description above.
>
> That last bit is definitely "lets 'not' do this again please!"

IMHO keeping it simpler is much better.


The interface where a buffer is passed and overflow returns an error so 
that one

can retry is far simpler and the in-kernel simplification of taking the 
lock over

the whole retrieval is far less problematic.


If there's anything that could be useful then it's keeping a count of 
the mounts

in a given namespace (I think we have such a count in nsproxy struct 
already) and

adding a way to get that for storage needs estimation.


Ian

Re: proposed libc interface and man page for statmount(2)

[Ian Kent]

On 21/11/23 09:12, Ian Kent wrote:
> On 21/11/23 08:58, Ian Kent wrote:
>> On 21/11/23 07:56, Ian Kent wrote:
>>> On 20/11/23 20:34, Miklos Szeredi wrote:
>>>> On Mon, Nov 20, 2023 at 01:16:24PM +0100, Florian Weimer wrote:
>>>>> Is the ID something specific to the VFS layer itself, or does it come
>>>>> from file systems?
>>>> It comes from the VFS.
>>>>
>>>>
>>>>> POSIX has a seekdir/telldir interface like that, I don't think file
>>>>> system authors like it.  Some have added dedicated data structures 
>>>>> for
>>>>> it to implement somewhat predictable behavior in the face of 
>>>>> concurrent
>>>>> directory modification.  Would this interface suffer from similar
>>>>> issues?
>>>> The same issue was solved for /proc/$$/mountinfo using cursors.
>>>
>>> The mounts are now using an rb-tree, I think the the cursor solution 
>>> can
>>>
>>> only work for a linear list, the case is very different.
>>>
>>>
>>>>
>>>> This patchset removes the need for cursors, since the new unique 
>>>> mount ID can be
>>>> used to locate the current position without having to worry about 
>>>> deleted and
>>>> added mounts.
>>>
>>> IIRC the problem with proc mounts traversals was because the lock 
>>> was taken
>>>
>>> and dropped between reads so that mount entries could be deleted 
>>> (not sure
>>>
>>> adding had quite the same problem) from the list in between reads.
>>>
>>>
>>> Sounds like I'll need to look at the code but first though but an 
>>> rb-tree
>>>
>>> can have mounts removed and new mounts inserted if the locks are 
>>> dropped
>>>
>>> if the retrieval is slit between multiple calls.
>>>
>>>
>>> So I'm struggling to see why this isn't the same problem and I don't 
>>> think
>>>
>>> introducing cursors in this case would work (thankfully, lets do 
>>> this again
>>>
>>> please).
>>
>> Mmm ... apologies for the poor description above.
>>
>> That last bit is definitely "lets 'not' do this again please!"
>
> IMHO keeping it simpler is much better.
>
>
> The interface where a buffer is passed and overflow returns an error 
> so that one
>
> can retry is far simpler and the in-kernel simplification of taking 
> the lock over
>
> the whole retrieval is far less problematic.
>
>
> If there's anything that could be useful then it's keeping a count of 
> the mounts
>
> in a given namespace (I think we have such a count in nsproxy struct 
> already) and
>
> adding a way to get that for storage needs estimation.

I've completely lost what we are talking about.


I think it was the introduction of listmount() earlier so my comment does

apply to that but it's getting just mount ids so the allocation isn't huge

even for a large number of mounts so I don't think there's a need for

complexity.


Then statmount() is retrieving the info for a single mount id, that's quite

specific info. and not that hard to estimate the needed allocation so the

error return and retry does seem like the most sensible thing.


The fact is that getting a list of mount ids and then working through them

meant you would be working with an outdated mounts list almost all the time.

That's nothing new for what this is used for and is sufficiently functional.


There are times when you do need a specific mount, such as SIGCHLD 
processing,

but then we are working with a specific mount id so it's not a problem.


>
>
> Ian
>
>

Re: proposed libc interface and man page for statmount(2)

[Miklos Szeredi]

On Tue, 21 Nov 2023 at 02:33, Ian Kent <raven@themaw.net> wrote:

> I've completely lost what we are talking about.

I started thinking about a good userspace API, and I'm skeptical about
the proposed kernel API being good for userspace as well.

Maybe something like this would be the simplest and least likely to be
misused (and also very similar to opendir/readdir/closedir):

handle = listmount_open(mnt_id, flags);
for (;;) {
    child_id = listmount_next(handle);
    if (child_id == 0)
        break;
    /* do something with child_id */
}
listmount_close(handle)

Thanks,
Miklos

Re: proposed libc interface and man page for statmount(2)

[Zack Weinberg]

On Tue, Nov 21, 2023, at 2:42 PM, Miklos Szeredi wrote:
>
> handle = listmount_open(mnt_id, flags);
> for (;;) {
>     child_id = listmount_next(handle);
>     if (child_id == 0)
>         break;
>     /* do something with child_id */
> }
> listmount_close(handle)

Why can't these be plain old open, read, and close? Starting from a pathname in /proc or /sys. Doesn't allow lseek.

zw

Re: proposed libc interface and man page for statmount(2)

[Ian Kent]

On 22/11/23 03:42, Miklos Szeredi wrote:
> On Tue, 21 Nov 2023 at 02:33, Ian Kent <raven@themaw.net> wrote:
>
>> I've completely lost what we are talking about.
> I started thinking about a good userspace API, and I'm skeptical about
> the proposed kernel API being good for userspace as well.
>
> Maybe something like this would be the simplest and least likely to be
> misused (and also very similar to opendir/readdir/closedir):
>
> handle = listmount_open(mnt_id, flags);
> for (;;) {
>      child_id = listmount_next(handle);
>      if (child_id == 0)
>          break;
>      /* do something with child_id */
> }
> listmount_close(handle)

Ahh ... yes that seems like something that would work.


Of course we will still end up working with an out of date list

but that's unavoidable, at least the list would be consistent at

the time it was fetched and if it was really needed to have a

consistent list then the above could be used.


Are there potential problems with holding locks over the

open/next/close procedure such as the close not called or the

process crashing?


Ian

Re: proposed libc interface and man page for statmount(2)

[Ian Kent]

On 22/11/23 04:42, Zack Weinberg wrote:
> On Tue, Nov 21, 2023, at 2:42 PM, Miklos Szeredi wrote:
>> handle = listmount_open(mnt_id, flags);
>> for (;;) {
>>      child_id = listmount_next(handle);
>>      if (child_id == 0)
>>          break;
>>      /* do something with child_id */
>> }
>> listmount_close(handle)
> Why can't these be plain old open, read, and close? Starting from a pathname in /proc or /sys. Doesn't allow lseek.

I'm not sure how this would work, there aren't a series of paths in proc

that represent mounts?


There are a couple of reasons for not creating a tree of directories

to represent mounts in the proc file system.


One is that open() is a fairly high overhead system call and it so it

won't cope well with traversing a large volume of mounts. Other times

I have introduced open/process/close for individual actions, rather

than keep the object open until it's no longer used, has proven to

impact performance in an unacceptable way.


Second is that, because the mount table lives in a file (actually more

than one with slightly different formats) it needs to be traversed every

time one is looking for a mount which has been shown to be high overhead,

especially if there are many change notifications from the kernel.


Ian

Re: proposed libc interface and man page for statmount(2)

[Christian Brauner]

On Tue, Nov 21, 2023 at 08:42:17PM +0100, Miklos Szeredi wrote:
> On Tue, 21 Nov 2023 at 02:33, Ian Kent <raven@themaw.net> wrote:
> 
> > I've completely lost what we are talking about.
> 
> I started thinking about a good userspace API, and I'm skeptical about
> the proposed kernel API being good for userspace as well.
> 
> Maybe something like this would be the simplest and least likely to be
> misused (and also very similar to opendir/readdir/closedir):

I want batch retrieval to be possible with the kernel interface. That's
important for userspace and was requested in the LSFMM discussion.

I would like to grab the proposal to return the last mount id in struct
mnt_id_req. In userspace it can then easily be implemented the way you
proposed below.

Re: proposed libc interface and man page for statmount(2)

[Zack Weinberg]

On Tue, Nov 21, 2023, at 6:28 PM, Ian Kent wrote:
> On 22/11/23 04:42, Zack Weinberg wrote:
>> On Tue, Nov 21, 2023, at 2:42 PM, Miklos Szeredi wrote:
>>> handle = listmount_open(mnt_id, flags);
>>> for (;;) {
>>>      child_id = listmount_next(handle);
>>>      if (child_id == 0)
>>>          break;
>>>      /* do something with child_id */
>>> }
>>> listmount_close(handle)
>>
>> Why can't these be plain old open, read, and close? Starting from a pathname
>> in /proc or /sys. Doesn't allow lseek.
>
> I'm not sure how this would work, there aren't a series of paths in proc
> that represent mounts?

It would be a new one created for this purpose.

listmount_open(mnt_id, flags) ==
   open("/proc/mount_ids", O_RDONLY) +
   ioctl(fd, LISTMNT_QUERY_ID, mnt_id) +
   ioctl(fd, LISTMNT_QUERY_FLAGS, flags)

and then read(fd, buf, sizeof(buf)) gives you sizeof(buf)/sizeof(mntid_t)
mount IDs.

Or, if you prefer, keep the new listmount_open() system call but have it
return a file descriptor that acts like a pipe filled with all the child
mount IDs.  The main point of my suggestion is that listmount_next and
listmount_close can, and should (IMO), just be read() and close().

Note that I'm _not_ suggesting a text-based interface like most of /proc.
What you read is native-endian binary mount IDs.

> One is that open() is a fairly high overhead system call and it so it
> won't cope well with traversing a large volume of mounts.
...
> Second is that, because the mount table lives in a file (actually more
> than one with slightly different formats) it needs to be traversed every
> time one is looking for a mount which has been shown to be high overhead,

Does my clarified explanation address these concerns?

zw