Date: Wed, 19 Jan 2011 00:34:07 -0800 From: Craig Rodrigues <rodrigc@crodrigues.org> To: Jaakko Heinonen <jh@FreeBSD.org> Cc: freebsd-hackers@FreeBSD.org Subject: Re: [patch] nmount ro, rw and negated option handling Message-ID: <20110119083407.GA51372@crodrigues.org> In-Reply-To: <20110114122454.GA4805@jh> References: <20110114122454.GA4805@jh>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi,
I disagree with your patch and do not approve it.
I prefer something simpler:
Index: vfs_mount.c
===================================================================
--- vfs_mount.c (revision 216607)
+++ vfs_mount.c (working copy)
@@ -522,10 +522,9 @@
struct vfsopt *opt, *noro_opt, *tmp_opt;
char *fstype, *fspath, *errmsg;
int error, fstypelen, fspathlen, errmsg_len, errmsg_pos;
- int has_rw, has_noro;
errmsg = fspath = NULL;
- errmsg_len = has_noro = has_rw = fspathlen = 0;
+ errmsg_len = fspathlen = 0;
errmsg_pos = -1;
error = vfs_buildopts(fsoptions, &optlist);
@@ -624,7 +623,8 @@
}
else if (strcmp(opt->name, "rw") == 0) {
fsflags &= ~MNT_RDONLY;
- has_rw = 1;
+ free(opt->name, M_MOUNT);
+ opt->name = strdup("noro", M_MOUNT);
}
else if (strcmp(opt->name, "ro") == 0)
fsflags |= MNT_RDONLY;
@@ -642,22 +642,6 @@
}
/*
- * If "rw" was specified as a mount option, and we
- * are trying to update a mount-point from "ro" to "rw",
- * we need a mount option "noro", since in vfs_mergeopts(),
- * "noro" will cancel "ro", but "rw" will not do anything.
- */
- if (has_rw && !has_noro) {
- noro_opt = malloc(sizeof(struct vfsopt), M_MOUNT, M_WAITOK);
- noro_opt->name = strdup("noro", M_MOUNT);
- noro_opt->value = NULL;
- noro_opt->len = 0;
- noro_opt->pos = -1;
- noro_opt->seen = 1;
- TAILQ_INSERT_TAIL(optlist, noro_opt, link);
- }
-
- /*
* Be ultra-paranoid about making sure the type and fspath
* variables will fit in our mp buffers, including the
* terminating NUL.
ZFS can be changed to check for "rw" or "noro".
--
Craig Rodrigues
rodrigc@crodrigues.org
On Fri, Jan 14, 2011 at 02:24:54PM +0200, Jaakko Heinonen wrote:
>
> Hi,
>
> Currently nmount(2) allows a mount point to have "ro", "rw", and "noro"
> string options concurrently active. This can cause erratic behavior
> demonstrated by this example:
>
> 1. Have mountd(8) running.
> 2. # mdconfig -a -t vnode -f ufsimg
> 3. # mount -o ro,rw /dev/md0 /mnt
>
> After these steps the mount point has string options "ro", "rw" and
> "noro" active but the MNT_RDONLY flag is not set. Eventually this will
> lead to "ffs_sync: rofs mod" (or similar) panic because the ffs code
> marks the file system read-only due to the "ro" string option.
> (MNT_RDONLY flag is used in most places for read-only check.)
>
> I wrote a patch to do following changes:
>
> - vfs_equalopts() now recognizes "ro" and "rw" as equal options
> - vfs_mergeopts() uses vfs_sanitizeopts() to merge options. This ensures
> that if the same option shows up several times (negated or not), only
> the last one is taken in account. There is still a problem when for
> example option "foo" and "nofoo" are merged: the "nofoo" option will
> become an active option. This is not a regression however and
> currently I don't know an easy way to solve this because the list of
> valid options is not available in vfs_mergeopts().
> - vfs_donmount() always converts "norw"/"rdonly" to "ro" and "noro" to
> "rw". Thus the mount point will always have either "rw" or "ro"
> option. I haven't seen any in-tree file system to test for "noro" but
> at least ZFS tests for "rw". That's why I chose "rw" instead or
> "noro".
>
> The patch is available here:
>
> http://people.freebsd.org/~jh/patches/nmount-ro-rw.diff
>
> Reviews and testing would be appreciated.
>
> Here are some references to bug reports which the patch attempts to
> resolve:
>
> http://lists.freebsd.org/pipermail/freebsd-current/2009-September/thread.html#11385
> http://lists.freebsd.org/pipermail/freebsd-questions/2009-August/204124.html
> http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/133614
> http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/150206
>
> --
> Jaakko
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110119083407.GA51372>