From: Crystal Kolipe <kolipe.c@exoticsilicon.com>
Subject: Re: [patch] mount_mfs allow setting permissions manually
To: Rafael Sadowski <rafael@sizeofvoid.org>, tech@openbsd.org
Date: Mon, 21 Jul 2025 04:18:29 -0300

On Sun, Jul 20, 2025 at 12:59:09PM +0100, Stuart Henderson wrote:
> On 2025/07/20 04:52, Crystal Kolipe wrote:
> > Do we also need to clarify in the man page what happens if both -p and -P are
> > used at the same time?  In that case, -p is ignored and the permissions from
> > the directory specified with -P are used.
> > 
> > I'm wondering if that is even the desired behaviour, maybe -p should always
> > override even when -P is specified.  But if nobody has an opinion on this
> > I'm OK with the current proposal.
> 
> fwiw, I think I would expect -p to be used "over the top" of -P's
> permissions if both flags are specified.

This version should address that.  Now if both -p and -P are specified on the
command line, the permissions of '.' are set to whatever was specified as the
argument to -p.

I'm assuming that's what you meant, although "over the top" could also be
interpreted as 'a logical AND of both the permissions gained from -P and those
specified with -p', (which could in itself could be considered an over the top
suggestion in another sense, thereby causing an infinite mental loop :-) ).

The chmod of '.' is not expected to fail, so we only print a warning if it
does, and continue with the inherited permissions.  Should this be a hard
error?

--- newfs.c.orig	Mon Apr  7 04:25:02 2025
+++ newfs.c	Mon Jul 21 08:15:41 2025
@@ -147,7 +147,7 @@
 static void waitformount(char *, pid_t);
 static int do_exec(const char *, const char *, char *const[]);
 static int isdir(const char *);
-static void copy(char *, char *);
+static void copy(char *, char *, mode_t);
 static int gettmpmnt(char *, size_t);
 #endif
 
@@ -179,6 +179,7 @@
 #ifdef MFS
 	char mountfromname[BUFSIZ];
 	char *pop = NULL, node[PATH_MAX];
+	char *ep;
 	pid_t pid;
 	struct stat mountpoint;
 #endif
@@ -203,7 +204,7 @@
 		fatal("insane maxpartitions value %d", maxpartitions);
 
 	opstring = mfs ?
-	    "O:P:T:b:c:e:f:i:m:o:s:" :
+	    "O:P:T:b:c:e:f:i:m:o:p:s:" :
 	    "NO:S:T:b:c:e:f:g:h:i:m:o:qs:t:";
 	while ((ch = getopt(argc, argv, opstring)) != -1) {
 		switch (ch) {
@@ -286,6 +287,19 @@
 					    optarg);
 			}
 			break;
+		case 'p':
+			errno = 0;
+			mfsmode = strtoul(optarg, &ep, 8);
+			if (optarg[0] == '\0' || *ep != '\0')
+				fatal("invalid mode: not a number");
+			if (errno == ERANGE)
+				fatal("invalid mode: out of range");
+			if ((mfsmode & ALLPERMS) != mfsmode)
+				fatal("unrecognised permission bits");
+			if (mfsmode == 0)
+				warnx("invalid mode, will inherit "
+				      "mount point permissions");
+			break;
 		case 'q':
 			quiet = 1;
 			break;
@@ -506,7 +520,8 @@
 			err(ECANCELED, "stat %s", node);
 		mfsuid = mountpoint.st_uid;
 		mfsgid = mountpoint.st_gid;
-		mfsmode = mountpoint.st_mode & ALLPERMS;
+		if (mfsmode == 0)
+			mfsmode = mountpoint.st_mode & ALLPERMS;
 	}
 #endif
 
@@ -543,7 +558,7 @@
 		default:
 			if (pop != NULL) {
 				waitformount(tmpnode, pid);
-				copy(pop, tmpnode);
+				copy(pop, tmpnode, mfsmode);
 				unmount(tmpnode, 0);
 				rmdir(tmpnode);
 			}
@@ -754,13 +769,18 @@
 }
 
 static void
-copy(char *src, char *dst)
+copy(char *src, char *dst, mode_t mfsmode)
 {
 	int ret, dir, created = 0;
+	int fd;
 	struct ufs_args mount_args;
 	char mountpoint[MNAMELEN];
 	char *const argv[] = { "pax", "-rw", "-pe", ".", dst, NULL } ;
 
+	if ((fd = open(dst, O_RDONLY)) == -1) {
+		err(1, "failed opening %s", dst);
+	}
+
 	dir = isdir(src);
 	if (dir)
 		strlcpy(mountpoint, src, sizeof(mountpoint));
@@ -788,6 +808,10 @@
 			warn("unmount %s", dst);
 		errx(1, "copy %s to %s failed", mountpoint, dst);
 	}
+	if (fchmodat(fd, ".", mfsmode, 0) == -1) {
+		warn("failed to set requested permissions");
+	}
+	close (fd);
 }
 
 static int