On Wed, Aug 28, 2019 at 03:39:17PM +0200, Otto Moerbeek wrote: > On Sat, Aug 17, 2019 at 12:13:50PM -0300, Rafael Neves wrote: > > > 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; > > Please move this one inside the scope where it is used, see below > > > pid_t pid; > > struct stat mountpoint; > > #endif > > @@ -516,7 +517,7 @@ havelabel: > > struct mfs_args args; > > char tmpnode[PATH_MAX]; > > here > > > > > - 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); > > It might be an idea to undo the mount an return 1 if ret is != 0. > For that to work ret should be initialized to 0. > > -Otto > [snip] >
Updated patch follows. I understood that return is through exit(3) as it is pattern used in other parts of the code. Index: sbin/newfs/newfs.c =================================================================== RCS file: /cvs/src/sbin/newfs/newfs.c,v retrieving revision 1.112 diff -u -p -r1.112 newfs.c --- sbin/newfs/newfs.c 28 Jun 2019 13:32:45 -0000 1.112 +++ sbin/newfs/newfs.c 1 Sep 2019 18:52:26 -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 @@ -515,8 +515,9 @@ havelabel: if (mfs) { struct mfs_args args; char tmpnode[PATH_MAX]; + int ret = 0; - 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 != 0) + 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); }