Hi,
Submitting to tech@ to broader audience.
When using -P option in mfs with a directory or a block device that
doen't exist, for example when the device roams, newfs(2) leaves
leftovers of temporary mount points.
With my /etc/fstab:
ca7552589896b01e.b none swap sw
ca7552589896b01e.a / ffs rw 1 1
ca7552589896b01e.k /home ffs rw,nodev,nosuid 1 2
#ca7552589896b01e.d /tmp ffs rw,nodev,nosuid 1 2
swap /tmp mfs rw,nodev,nosuid,-s=300M 0 0
ca7552589896b01e.f /usr ffs rw,nodev 1 2
ca7552589896b01e.g /usr/X11R6 ffs rw,nodev 1 2
ca7552589896b01e.h /usr/local ffs rw,nodev,wxallowed 1 2
ca7552589896b01e.j /usr/build ffs rw,noperm,noauto 1 2
swap /usr/obj mfs rw,nodev,nosuid,noauto,-s=4G,-P=/foo/bar 0 0
ca7552589896b01e.i /usr/src ffs rw,nodev,nosuid 1 2
ca7552589896b01e.e /var ffs rw,nodev,nosuid 1 2
Result when trying to mount /usr/obj:
root@orus [rneves]# mount /usr/obj
mount_mfs: cannot stat /foo/bar: No such file or directory
root@orus [rneves]# mount
/dev/sd2a on / type ffs (local)
/dev/sd2k on /home type ffs (local, nodev, nosuid)
mfs:28249 on /tmp type mfs (asynchronous, local, nodev, nosuid,
size=614400 512-blocks)
/dev/sd2f on /usr type ffs (local, nodev)
/dev/sd2g on /usr/X11R6 type ffs (local, nodev)
/dev/sd2h on /usr/local type ffs (local, nodev, wxallowed)
/dev/sd2i on /usr/src type ffs (local, nodev, nosuid)
/dev/sd2e on /var type ffs (local, nodev, nosuid)
mfs:44634 on /tmp/mntoMG6WmZTT7 type mfs (asynchronous, local, nodev,
nosuid, size=8388608 512-blocks)
Tracking down the issue I found that:
+ When -P is specified, pop != NULL.
+ After fork, waitformount() is called. It creates the temporary
places to store the data.
+ copy() is called, and it it fails the following umount() and
rmdir() is not called, leaving the temporary mounts.
As copy() can fail in a couple of ways, I thought about the following change:
+ Make all errors a warning, but after then return to the first
caller indicating an error. Getting the erro the clean up is
done, and exit(1).
+ Make copy() return an int: -1 in fail, and 0 otherwise.
+ isdir(), gettmpmnt(): Convert errors to warnings + return(-1).
There is a change of messages printing if you don't have a /tmp
is read-only, first the error of cannot fstat, and after
"Cannot create tmp mountpoint for -P".
There still a chance to get a inconsistent state: if there is no
/bin/pax, or errors in do_exec(). But erros in do_exec seem very
critical, the same with a missing /bin/pax. So I didn't change them.
Otto@ said that another alternative is using atexit(3), but we
pointed out what it is very difficult to get it right, and almost
always has race conditions. Given that and what manpage says I have no
hope that I can get it right.
What do you think?
Note that the check in line 519 (newfs.c) was changed to add the new
possible return value. Actually, currently it is not allowed -P with a
read-only /tmp, because in this case gettmpmnt() returns 0, and by this
early check newfs(2) throws an error. Actually, gettmpmnt() must changed
to it work properly. The `created` if uses a <= by symmetry.
But this is a differente issue, that I think could be changed in a
separated diff.
Regards,
Rafael Neves
Patch:
Index: newfs.c
===================================================================
RCS file: /cvs/src/sbin/newfs/newfs.c,v
retrieving revision 1.112
diff -u -p -r1.112 newfs.c
--- newfs.c 28 Jun 2019 13:32:45 -0000 1.112
+++ newfs.c 17 Aug 2019 14:27:46 -0000
@@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
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 int copy(char *, char *);
static int gettmpmnt(char *, size_t);
#endif
@@ -179,6 +179,7 @@ main(int argc, char *argv[])
#ifdef MFS
char mountfromname[BUFSIZ];
char *pop = NULL, node[PATH_MAX];
+ int ret;
pid_t pid;
struct stat mountpoint;
#endif
@@ -516,7 +517,7 @@ havelabel:
struct mfs_args args;
char tmpnode[PATH_MAX];
- if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
+ if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
errx(1, "Cannot create tmp mountpoint for -P");
memset(&args, 0, sizeof(args));
args.base = membase;
@@ -537,9 +538,11 @@ havelabel:
default:
if (pop != NULL) {
waitformount(tmpnode, pid);
- copy(pop, tmpnode);
+ ret = copy(pop, tmpnode);
unmount(tmpnode, 0);
rmdir(tmpnode);
+ if (ret == -1)
+ exit(1);
}
waitformount(node, pid);
exit(0);
@@ -740,14 +743,18 @@ isdir(const char *path)
{
struct stat st;
- if (stat(path, &st) != 0)
- err(1, "cannot stat %s", path);
- if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode))
- errx(1, "%s: not a dir or a block device", path);
+ if (stat(path, &st) != 0) {
+ warn("cannot stat %s", path);
+ return (-1);
+ }
+ if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode)) {
+ warn("%s: not a dir or a block device", path);
+ return (-1);
+ }
return (S_ISDIR(st.st_mode));
}
-static void
+static int
copy(char *src, char *dst)
{
int ret, dir, created = 0;
@@ -755,11 +762,16 @@ copy(char *src, char *dst)
char mountpoint[MNAMELEN];
char *const argv[] = { "pax", "-rw", "-pe", ".", dst, NULL } ;
- dir = isdir(src);
+ if ((dir = isdir(src)) == -1)
+ return (-1);
+
if (dir)
strlcpy(mountpoint, src, sizeof(mountpoint));
else {
created = gettmpmnt(mountpoint, sizeof(mountpoint));
+ if (created <= 0)
+ return (-1);
+
memset(&mount_args, 0, sizeof(mount_args));
mount_args.fspec = src;
ret = mount(MOUNT_FFS, mountpoint, MNT_RDONLY, &mount_args);
@@ -769,7 +781,8 @@ copy(char *src, char *dst)
warn("rmdir %s", mountpoint);
if (unmount(dst, 0) != 0)
warn("unmount %s", dst);
- errc(1, saved_errno, "mount %s %s", src, mountpoint);
+ warnc(saved_errno, "mount %s %s", src, mountpoint);
+ return (-1);
}
}
ret = do_exec(mountpoint, "/bin/pax", argv);
@@ -780,8 +793,10 @@ copy(char *src, char *dst)
if (ret != 0) {
if (unmount(dst, 0) != 0)
warn("unmount %s", dst);
- errx(1, "copy %s to %s failed", mountpoint, dst);
+ warnx("copy %s to %s failed", mountpoint, dst);
+ return (-1);
}
+ return (0);
}
static int
@@ -792,27 +807,42 @@ gettmpmnt(char *mountpoint, size_t len)
struct statfs fs;
size_t n;
- if (statfs(tmp, &fs) != 0)
- err(1, "statfs %s", tmp);
+ if (statfs(tmp, &fs) != 0) {
+ warn("statfs %s", tmp);
+ return (-1);
+ }
+
if (fs.f_flags & MNT_RDONLY) {
- if (statfs(mnt, &fs) != 0)
- err(1, "statfs %s", mnt);
- if (strcmp(fs.f_mntonname, "/") != 0)
- errx(1, "tmp mountpoint %s busy", mnt);
- if (strlcpy(mountpoint, mnt, len) >= len)
- errx(1, "tmp mountpoint %s too long", mnt);
+ if (statfs(mnt, &fs) != 0) {
+ warn("statfs %s", mnt);
+ return (-1);
+ }
+ if (strcmp(fs.f_mntonname, "/") != 0) {
+ warnx("tmp mountpoint %s busy", mnt);
+ return (-1);
+ }
+ if (strlcpy(mountpoint, mnt, len) >= len) {
+ warnx("tmp mountpoint %s too long", mnt);
+ return (-1);
+ }
return (0);
}
n = strlcpy(mountpoint, tmp, len);
- if (n >= len)
- errx(1, "tmp mount point too long");
+ if (n >= len) {
+ warnx("tmp mount point too long");
+ return (-1);
+ }
if (mountpoint[n - 1] != '/')
strlcat(mountpoint, "/", len);
n = strlcat(mountpoint, "mntXXXXXXXXXX", len);
- if (n >= len)
- errx(1, "tmp mount point too long");
- if (mkdtemp(mountpoint) == NULL)
- err(1, "mkdtemp %s", mountpoint);
+ if (n >= len) {
+ warnx("tmp mount point too long");
+ return (-1);
+ }
+ if (mkdtemp(mountpoint) == NULL) {
+ warn("mkdtemp %s", mountpoint);
+ return (-1);
+ }
return (1);
}
