On Mon, Sep 02, 2019 at 03:59:48PM -0300, Rafael Neves wrote:
> On Mon, Sep 02, 2019 at 07:26:27AM +0200, Otto Moerbeek wrote:
> > On Sun, Sep 01, 2019 at 05:01:55PM -0300, Rafael Neves wrote:
> >
> > > 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.
> >
> > I don't code undoing the mount below...
> >
> > -Otto
> > >
> > >
> > > 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);
> >
> > Would expect it here...
> >
> > [snip]
>
> Sure! Updated patch below.
>
>
> 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 2 Sep 2019 18:44:04 -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,11 +538,15 @@ havelabel:
> default:
> if (pop != NULL) {
> waitformount(tmpnode, pid);
> - copy(pop, tmpnode);
> + ret = copy(pop, tmpnode);
> unmount(tmpnode, 0);
> rmdir(tmpnode);
> }
> waitformount(node, pid);
> + if (ret != 0) {
> + unmount(node, 0);
> + exit(1);
> + }
> exit(0);
> /* NOTREACHED */
> }
> @@ -740,14 +745,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 +764,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 +783,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 +795,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 +809,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);
> }
>
>
Updated patch: It includes Otto's requests and remove unnecessary
unmount(dst, 0) from copy(), because the caller unmount tmpnode.
While there, I adjusted the return value of gettmpmnt() to return 0
on success, and changed the checks according.
Regards,
Rafael Neves
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 8 Sep 2019 22:49:11 -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,11 +538,15 @@ havelabel:
default:
if (pop != NULL) {
waitformount(tmpnode, pid);
- copy(pop, tmpnode);
+ ret = copy(pop, tmpnode);
unmount(tmpnode, 0);
rmdir(tmpnode);
}
waitformount(node, pid);
+ if (ret != 0) {
+ unmount(node, 0);
+ exit(1);
+ }
exit(0);
/* NOTREACHED */
}
@@ -740,48 +745,55 @@ 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;
+ int ret, dir;
struct ufs_args mount_args;
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 (gettmpmnt(mountpoint, sizeof(mountpoint)) != 0)
+ return (-1);
+
memset(&mount_args, 0, sizeof(mount_args));
mount_args.fspec = src;
ret = mount(MOUNT_FFS, mountpoint, MNT_RDONLY, &mount_args);
if (ret != 0) {
int saved_errno = errno;
- if (created && rmdir(mountpoint) != 0)
+ if (rmdir(mountpoint) != 0)
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);
if (!dir && unmount(mountpoint, 0) != 0)
warn("unmount %s", mountpoint);
- if (created && rmdir(mountpoint) != 0)
+ if (!dir && rmdir(mountpoint) != 0)
warn("rmdir %s", mountpoint);
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,28 +804,43 @@ 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);
- return (0);
+ 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 (-1);
}
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);
- return (1);
+ if (n >= len) {
+ warnx("tmp mount point too long");
+ return (-1);
+ }
+ if (mkdtemp(mountpoint) == NULL) {
+ warn("mkdtemp %s", mountpoint);
+ return (-1);
+ }
+ return (0);
}
#endif /* MFS */
