Re: [PATCH 04/17] mount_service: use the new mount api for the mount service

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Mar 30, 2026 at 11:40:22PM +0200, Bernd Schubert wrote:
> 
> 
> On 3/30/26 23:18, Darrick J. Wong wrote:
> > On Mon, Mar 30, 2026 at 09:06:05PM +0000, Bernd Schubert wrote:
> >> On 3/27/26 02:25, Darrick J. Wong wrote:
> >>> From: Darrick J. Wong <djwong@kernel.org>
> >>>
> >>> Use the new fsopen/fsmount system calls to mount the filesystem so that
> >>> we get somewhat better diagnostics if something gets screwed up.
> >>>
> >>> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> >>> ---
> >>>  meson.build          |   15 +++
> >>>  util/mount_service.c |  299 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  2 files changed, 314 insertions(+)
> >>>
> >>>
> >>> diff --git a/meson.build b/meson.build
> >>> index d6ba8740effd5c..b0988548bf806a 100644
> >>> --- a/meson.build
> >>> +++ b/meson.build
> >>> @@ -130,6 +130,21 @@ special_funcs = {
> >>>  	int main(int argc, char *argv[]) {
> >>>            return SD_LISTEN_FDS_START;
> >>>  	}
> >>> +    ''',
> >>> +    'new_mount_api': '''
> >>> +       #define _GNU_SOURCE
> >>> +       #include <sys/mount.h>
> >>> +       #include <linux/mount.h>
> >>> +       #include <unistd.h>
> >>> +       #include <fcntl.h>
> >>> +
> >>> +       int main(void) {
> >>> +           int fsfd = fsopen("fuse", FSOPEN_CLOEXEC);
> >>> +           int res = fsconfig(fsfd, FSCONFIG_SET_STRING, "source", "test", 0);
> >>> +           int mntfd = fsmount(fsfd, FSMOUNT_CLOEXEC, 0);
> >>> +           res = move_mount(mntfd, "", AT_FDCWD, "/mnt", MOVE_MOUNT_F_EMPTY_PATH);
> >>> +           return 0;
> >>> +       }
> >>>      '''
> >>>  }
> >>>  
> >>> diff --git a/util/mount_service.c b/util/mount_service.c
> >>> index ae078e537dc560..4a2c1111b66b91 100644
> >>> --- a/util/mount_service.c
> >>> +++ b/util/mount_service.c
> >>> @@ -65,6 +65,9 @@ struct mount_service {
> >>>  
> >>>  	/* O_PATH fd for mount point */
> >>>  	int mountfd;
> >>> +
> >>> +	/* fd for fsopen */
> >>> +	int fsopenfd;
> >>>  };
> >>>  
> >>>  /* Filter out the subtype of the filesystem (e.g. fuse.Y -> Y) */
> >>> @@ -87,6 +90,7 @@ static int mount_service_init(struct mount_service *mo, int argc, char *argv[])
> >>>  	mo->argvfd = -1;
> >>>  	mo->fusedevfd = -1;
> >>>  	mo->mountfd = -1;
> >>> +	mo->fsopenfd = -1;
> >>>  
> >>>  	for (i = 0; i < argc; i++) {
> >>>  		if (!strcmp(argv[i], "-t") && i + 1 < argc) {
> >>> @@ -690,9 +694,50 @@ static int mount_service_handle_fsopen_cmd(struct mount_service *mo,
> >>>  		return mount_service_send_reply(mo, error);
> >>>  	}
> >>>  
> >>> +#ifdef HAVE_NEW_MOUNT_API
> >>> +	/* If this fails we fall back on mount() */
> >>> +	mo->fsopenfd = fsopen(oc->value, FSOPEN_CLOEXEC);
> >>> +#endif
> >>> +
> >>>  	return mount_service_send_reply(mo, 0);
> >>>  }
> >>>  
> >>> +#ifdef HAVE_NEW_MOUNT_API
> >>> +/* callers must preserve errno */
> >>> +static void emit_fsconfig_messages(const struct mount_service *mo)
> >>> +{
> >>> +	uint8_t buf[BUFSIZ];
> >>> +	ssize_t sz;
> >>> +
> >>> +	while ((sz = read(mo->fsopenfd, buf, sizeof(buf) - 1)) != -1) {
> >>> +		if (sz <= 0)
> >>> +			continue;
> >>> +		if (buf[sz - 1] == '\n')
> >>> +			buf[--sz] = '\0';
> >>> +		else
> >>> +			buf[sz] = '\0';
> >>> +
> >>> +		if (!*buf)
> >>> +			continue;
> >>> +
> >>> +		switch (buf[0]) {
> >>> +		case 'e':
> >>> +			fprintf(stderr, "Error: %s\n", buf + 2);
> >>> +			break;
> >>> +		case 'w':
> >>> +			fprintf(stderr, "Warning: %s\n", buf + 2);
> >>> +			break;
> >>> +		case 'i':
> >>> +			fprintf(stderr, "Info: %s\n", buf + 2);
> >>> +			break;
> >>> +		default:
> >>> +			fprintf(stderr, " %s\n", buf);
> >>> +			break;
> >>> +		}
> >>> +	}
> >>> +}
> >>> +#endif
> >>
> >> How about unifying that with my series and then use log_fsconfig_kmsg()?
> >> We can also do it independently, but should not forget to use a common
> >> function later on. That applies to all these functions for the new mount
> >> API. Part of the headache I have with my series is that fusermount.c
> >> duplicates quite some code.
> >> For me it looks a bit like we can unify most of util/mount_service.c and
> >> lib/mount_fsmount.c.
> > 
> > <nod> The mount option string parsing (i.e. all that MS_ -> MOUNT_ATTR
> > translation) and error reporting parts look trivially shareable.
> > fuse_kern_fsmount isn't so easy to share since the fuse server sends
> > over all the relevant pieces in separate commands.
> > 
> >>> +
> >>>  static int mount_service_handle_source_cmd(struct mount_service *mo,
> >>>  					   const struct fuse_service_packet *p)
> >>>  {
> >>> @@ -714,6 +759,21 @@ static int mount_service_handle_source_cmd(struct mount_service *mo,
> >>>  		return mount_service_send_reply(mo, error);
> >>>  	}
> >>>  
> >>> +#ifdef HAVE_NEW_MOUNT_API
> >>> +	if (mo->fsopenfd >= 0) {
> >>> +		int ret = fsconfig(mo->fsopenfd, FSCONFIG_SET_STRING, "source",
> >>> +			       oc->value, 0);
> >>> +		if (ret) {
> >>> +			int error = errno;
> >>> +
> >>> +			fprintf(stderr, "%s: fsconfig source: %s\n",
> >>> +				mo->msgtag, strerror(error));
> >>> +			emit_fsconfig_messages(mo);
> >>> +			return mount_service_send_reply(mo, error);
> >>> +		}
> >>> +	}
> >>> +#endif
> >>> +
> >>>  	return mount_service_send_reply(mo, 0);
> >>>  }
> >>>  
> >>> @@ -722,6 +782,8 @@ static int mount_service_handle_mntopts_cmd(struct mount_service *mo,
> >>>  {
> >>>  	struct fuse_service_string_command *oc =
> >>>  			container_of(p, struct fuse_service_string_command, p);
> >>> +	char *tokstr = oc->value;
> >>> +	char *tok, *savetok;
> >>>  
> >>>  	if (mo->mntopts) {
> >>>  		fprintf(stderr, "%s: mount options respecified!\n",
> >>> @@ -738,6 +800,45 @@ static int mount_service_handle_mntopts_cmd(struct mount_service *mo,
> >>>  		return mount_service_send_reply(mo, error);
> >>>  	}
> >>>  
> >>> +	/* strtok_r mutates tokstr aka oc->value */
> >>> +	while ((tok = strtok_r(tokstr, ",", &savetok)) != NULL) {
> >>> +		char *equals = strchr(tok, '=');
> >>> +		char oldchar = 0;
> >>> +
> >>> +		if (equals) {
> >>> +			oldchar = *equals;
> >>> +			*equals = 0;
> >>> +		}
> >>> +
> >>> +#ifdef HAVE_NEW_MOUNT_API
> >>> +		if (mo->fsopenfd >= 0) {
> >>> +			int ret;
> >>> +
> >>> +			if (equals)
> >>> +				ret = fsconfig(mo->fsopenfd,
> >>> +					       FSCONFIG_SET_STRING, tok,
> >>> +					       equals + 1, 0);
> >>> +			else
> >>> +				ret = fsconfig(mo->fsopenfd,
> >>> +					       FSCONFIG_SET_FLAG, tok,
> >>> +					       NULL, 0);
> >>> +			if (ret) {
> >>> +				int error = errno;
> >>> +
> >>> +				fprintf(stderr, "%s: set mount option: %s\n",
> >>> +					mo->msgtag, strerror(error));
> >>> +				emit_fsconfig_messages(mo);
> >>> +				return mount_service_send_reply(mo, error);
> >>> +			}
> >>> +		}
> >>> +#endif
> >>> +
> >>> +		if (equals)
> >>> +			*equals = oldchar;
> >>> +
> >>> +		tokstr = NULL;
> >>> +	}
> >>> +
> >>>  	return mount_service_send_reply(mo, 0);
> >>>  }
> >>>  
> >>> @@ -875,6 +976,196 @@ static int mount_service_regular_mount(struct mount_service *mo,
> >>>  	return mount_service_send_reply(mo, 0);
> >>>  }
> >>>  
> >>> +#ifdef HAVE_NEW_MOUNT_API
> >>> +struct ms_to_mount_map {
> >>> +	unsigned long ms_flag;
> >>> +	unsigned int mount_attr_flag;
> >>> +};
> >>> +
> >>> +static const struct ms_to_mount_map attrs[] = {
> >>> +	{ MS_RDONLY,		MOUNT_ATTR_RDONLY },
> >>> +	{ MS_NOSUID,		MOUNT_ATTR_NOSUID },
> >>> +	{ MS_NODEV,		MOUNT_ATTR_NODEV },
> >>> +	{ MS_NOEXEC,		MOUNT_ATTR_NOEXEC },
> >>> +	{ MS_RELATIME,		MOUNT_ATTR_RELATIME },
> >>> +	{ MS_NOATIME,		MOUNT_ATTR_NOATIME },
> >>> +	{ MS_STRICTATIME,	MOUNT_ATTR_STRICTATIME },
> >>> +	{ MS_NODIRATIME,	MOUNT_ATTR_NODIRATIME },
> >>> +#ifdef MOUNT_ATTR_NOSYMFOLLOW
> >>> +	{ MS_NOSYMFOLLOW,	MOUNT_ATTR_NOSYMFOLLOW },
> >>> +#endif
> >>> +	{ 0, 0 },
> >>> +};
> >>> +
> >>> +static void get_mount_attr_flags(const struct fuse_service_mount_command *oc,
> >>> +				 unsigned int *attr_flags,
> >>> +				 unsigned long *leftover_ms_flags)
> >>> +{
> >>> +	const struct ms_to_mount_map *i;
> >>> +	unsigned int ms_flags = ntohl(oc->ms_flags);
> >>> +	unsigned int mount_attr_flags = 0;
> >>> +
> >>> +	for (i = attrs; i->ms_flag != 0; i++) {
> >>> +		if (ms_flags & i->ms_flag)
> >>> +			mount_attr_flags |= i->mount_attr_flag;
> >>> +		ms_flags &= ~i->ms_flag;
> >>> +	}
> >>> +
> >>> +	*leftover_ms_flags = ms_flags;
> >>> +	*attr_flags = mount_attr_flags;
> >>> +}
> >>> +
> >>> +struct ms_to_str_map {
> >>> +	unsigned long ms_flag;
> >>> +	const char *string;
> >>> +};
> >>> +
> >>> +static const struct ms_to_str_map strflags[] = {
> >>> +	{ MS_SYNCHRONOUS,	"sync" },
> >>> +	{ MS_DIRSYNC,		"dirsync" },
> >>> +	{ MS_LAZYTIME,		"lazytime" },
> >>> +	{ 0, 0 },
> >>> +};
> >>> +
> >>> +static int set_ms_flags(struct mount_service *mo, unsigned long ms_flags)
> >>> +{
> >>> +	const struct ms_to_str_map *i;
> >>> +	int ret;
> >>> +
> >>> +	for (i = strflags; i->ms_flag != 0; i++) {
> >>> +		if (!(ms_flags & i->ms_flag))
> >>> +			continue;
> >>> +
> >>> +		ret = fsconfig(mo->fsopenfd, FSCONFIG_SET_FLAG, i->string,
> >>> +			       NULL, 0);
> >>> +		if (ret) {
> >>> +			int error = errno;
> >>> +
> >>> +			fprintf(stderr, "%s: set %s option: %s\n",
> >>> +				mo->msgtag, i->string, strerror(error));
> >>> +			emit_fsconfig_messages(mo);
> >>> +
> >>> +			errno = error;
> >>> +			return -1;
> >>> +		}
> >>> +		ms_flags &= ~i->ms_flag;
> >>> +	}
> >>> +
> >>> +	/*
> >>> +	 * We can't translate all the supplied MS_ flags into MOUNT_ATTR_ flags
> >>> +	 * or string flags!  Return a magic code so the caller will fall back
> >>> +	 * to regular mount(2).
> >>> +	 */
> >>> +	return ms_flags ? -2 : 0;
> >>> +}
> >>
> >> Let's please not forget later on to use the new define.
> > 
> > Er... which new define?  HAVE_NEW_MOUNT_API?  I think we're still inside
> > the #ifdef for that at this point.
> > 
> > <confused>
> 
> 
> Sorry, I meant FUSE_MOUNT_FALLBACK_NEEDED.

Fixed.

--D