[PATCH] libmount: provide tree fd even when a mount helper is used

[PATCH] libmount: provide tree fd even when a mount helper is used

[Linus Heckemann]

Previously, the X-mount.subdir option would fail (mount exited with
code 0, but the target was not mounted) when a helper was used.

In addition to fixing X-mount.subdir, this allows dropping the
fallback behaviour previously implemented specifically by the
set_vfsflags and set_propagation hooks.

I realise this patch is not acceptable as is, since I just exported
the previously private open_mount_tree symbol from hook_mount.c
without even adding it to a header (so builds with sensible warning
configurations, with -Werror=implicit-function-declaration, won't even
compile this); but I'm submitting this as is in hopes that someone who
knows their way around libmount will suggest a sensible way to shuffle
the code around.
---
 libmount/src/context_mount.c | 15 ++++++++++++++-
 libmount/src/hook_mount.c    | 20 +-------------------
 2 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/libmount/src/context_mount.c b/libmount/src/context_mount.c
index 676287733..ef66f47af 100644
--- a/libmount/src/context_mount.c
+++ b/libmount/src/context_mount.c
@@ -482,8 +482,21 @@ static int exec_helper(struct libmnt_context *cxt)
 			DBG(CXT, ul_debugobj(cxt, "%s forked [status=%d, rc=%d]",
 				cxt->helper,
 				cxt->helper_status, rc));
+
+			struct libmnt_sysapi *api = mnt_context_get_sysapi(cxt);
+			assert(api);
+
+			/* a mount helper, unlike an internal mount, won't provide the mount fd.
+			 * This is, however, needed by hooks like subdir -- so let's open the tree
+			 * that was mounted by our helper child. */
+			if (api->fd_tree < 0 && mnt_fs_get_target(cxt->fs)) {
+				api->fd_tree = open_mount_tree(cxt, NULL, (unsigned long) -1);
+				if (api->fd_tree > 0) {
+					rc = 0;
+				}
+			}
+			break;
 		}
-		break;
 	}
 
 	case -1:
diff --git a/libmount/src/hook_mount.c b/libmount/src/hook_mount.c
index 6b7caff85..b80fe4839 100644
--- a/libmount/src/hook_mount.c
+++ b/libmount/src/hook_mount.c
@@ -255,7 +255,7 @@ static int open_fs_configuration_context(struct libmnt_context *cxt,
 	return api->fd_fs;
 }
 
-static int open_mount_tree(struct libmnt_context *cxt, const char *path, unsigned long mflg)
+int open_mount_tree(struct libmnt_context *cxt, const char *path, unsigned long mflg)
 {
 	unsigned long oflg = OPEN_TREE_CLOEXEC;
 	int rc = 0, fd = -1;
@@ -416,15 +416,6 @@ static int set_vfsflags(struct libmnt_context *cxt,
 	api = get_sysapi(cxt);
 	assert(api);
 
-	/* fallback only; necessary when init_sysapi() during preparation
-	 * cannot open the tree -- for example when we call /sbin/mount.<type> */
-	if (api->fd_tree < 0 && mnt_fs_get_target(cxt->fs)) {
-		rc = api->fd_tree = open_mount_tree(cxt, NULL, (unsigned long) -1);
-		if (rc < 0)
-			return rc;
-		rc = 0;
-	}
-
 	if (recursive)
 		callflags |= AT_RECURSIVE;
 
@@ -494,15 +485,6 @@ static int hook_set_propagation(struct libmnt_context *cxt,
 	api = get_sysapi(cxt);
 	assert(api);
 
-	/* fallback only; necessary when init_sysapi() during preparation
-	 * cannot open the tree -- for example when we call /sbin/mount.<type> */
-	if (api->fd_tree < 0 && mnt_fs_get_target(cxt->fs)) {
-		rc = api->fd_tree = open_mount_tree(cxt, NULL, (unsigned long) -1);
-		if (rc < 0)
-			goto done;
-		rc = 0;
-	}
-
 	mnt_reset_iter(&itr, MNT_ITER_FORWARD);
 
 	while (mnt_optlist_next_opt(ol, &itr, &opt) == 0) {
-- 
2.42.0

Re: [PATCH] libmount: provide tree fd even when a mount helper is used

[Karel Zak]

On Thu, May 30, 2024 at 08:00:34PM +0200, Linus Heckemann wrote:
> Previously, the X-mount.subdir option would fail (mount exited with
> code 0, but the target was not mounted) when a helper was used.

Do you have any examples that can easily reproduce it?

> In addition to fixing X-mount.subdir, this allows dropping the
> fallback behaviour previously implemented specifically by the
> set_vfsflags and set_propagation hooks.
> 
> I realise this patch is not acceptable as is, since I just exported
> the previously private open_mount_tree symbol from hook_mount.c

Perhaps it is unnecessary to only keep the API file descriptors in
hook_mount.c. It is a generic feature and we may see more use
cases where it would make sense to use it in other places.

I can imagine having the file descriptors in the libmnt_context
structure and initializing them through functions in context.c (which
would involve renaming open_mount_tree() to something more
appropriate). It could potentially be a public library function so
that libmount applications can also utilize it.

I will think about it :-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] libmount: provide tree fd even when a mount helper is used

[Linus Heckemann]

Karel Zak <kzak@redhat.com> writes:

> On Thu, May 30, 2024 at 08:00:34PM +0200, Linus Heckemann wrote:
>> Previously, the X-mount.subdir option would fail (mount exited with
>> code 0, but the target was not mounted) when a helper was used.
>
> Do you have any examples that can easily reproduce it?

I encountered this issue with bcachefs (create /foo in the root of a
bcachefs filesystem and then attempt to mount that same filesystem with
X-mount.subdir=foo) but the problem should be the same with any
other filesystem that uses a mount helper.

truncate -s 1G test.img
mkfs.bcachefs test.img
mount -o loop,X-mount.mkdir test.img /tmp/mnt
mkdir /tmp/mnt/foo
umount /tmp/mnt
mount -o loop,X-mount.subdir=foo test.img /tmp/mnt

Without my patch, this exits with status code 0 but does not result in
/tmp/mnt being mounted at all.


>> In addition to fixing X-mount.subdir, this allows dropping the
>> fallback behaviour previously implemented specifically by the
>> set_vfsflags and set_propagation hooks.
>> 
>> I realise this patch is not acceptable as is, since I just exported
>> the previously private open_mount_tree symbol from hook_mount.c
>
> Perhaps it is unnecessary to only keep the API file descriptors in
> hook_mount.c. It is a generic feature and we may see more use
> cases where it would make sense to use it in other places.
>
> I can imagine having the file descriptors in the libmnt_context
> structure and initializing them through functions in context.c (which
> would involve renaming open_mount_tree() to something more
> appropriate). It could potentially be a public library function so
> that libmount applications can also utilize it.
>
> I will think about it :-)

Thanks! That sounds like it would make sense, though I'd expect (naively
-- I'm not super familiar with the overall code) it's generally not
desirable to use that function as opposed to using the mount fd from the
context once it's become available through the mount step? I don't see a
particularly strong case for using that and not open_tree directly.

Re: [PATCH] libmount: provide tree fd even when a mount helper is used

[Karel Zak]

On Fri, May 31, 2024 at 11:41:28AM +0200, Karel Zak wrote:
> On Thu, May 30, 2024 at 08:00:34PM +0200, Linus Heckemann wrote:
> > Previously, the X-mount.subdir option would fail (mount exited with
> > code 0, but the target was not mounted) when a helper was used.
> 
> I will think about it :-)

 https://github.com/util-linux/util-linux/pull/3075

 Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com