Date: Fri, 05 Aug 2011 10:18:49 +0200 From: Martin Matuska <mm@FreeBSD.org> To: Kostik Belousov <kostikbel@gmail.com> Cc: Xin Li <delphij@FreeBSD.org>, current@FreeBSD.org Subject: Re: panic: share -> excl @r224632 Message-ID: <4E3BA769.3090408@FreeBSD.org> In-Reply-To: <20110804135932.GG17489@deviant.kiev.zoral.com.ua> References: <20110804125454.GI29364@albert.catwhisker.org> <20110804135932.GG17489@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------050805060602090007070309
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
I agree to Kostik's approach, but I suggest implementing it in a
separate function and also use for the unmount() part.
Please review attached patch.
On 04.08.2011 15:59, Kostik Belousov wrote:
> On Thu, Aug 04, 2011 at 05:54:54AM -0700, David Wolfskill wrote:
>> I've only seen this on my laptop; my build machine doesn't exhibit the
>> panic.
>>
>> r224602 is the most recent point I've built that does not exhibit the
>> panic at all.
>>
>> The first few lines (hand-transcribed; I have no serial console on this
>> laptop -- the one shortcoming it has):
>>
>> shared lock of (lockmgr) devfs @/usr/src/sys/kern/vfs_cache.c:1116
>> while exclusively locked from /usr/src/sys/kern/vfs_subr.c:2134
>> panic: share -> excl
>>
>> The backtrace (which I wasn't patient enough to trto transcribe twice;
>> sorry) appeared to involve "nmount", and the panic occurred directly
>> after mounting the file systems during the transition from single-user
>> mode to multi-user mode.
>>
>> The output from "svn update" going from r224602 -> r224632 shows the
>> following files being updated:
>>
>> U usr.sbin/faithd/faithd.8
>> U usr.sbin/jail/jail.8
>> U lib/libproc/proc_create.c
>> U sys/arm/arm/irq_dispatch.S
>> U sys/arm/sa11x0/sa11x0_irq.S
>> U sys/powerpc/booke/locore.S
>> U sys/powerpc/booke/platform_bare.c
>> U sys/powerpc/booke/pmap.c
>> U sys/nfsclient/nfsnode.h
>> U sys/nfsclient/nfs_node.c
>> U sys/kern/kern_jail.c
>> U sys/kern/vfs_mount.c
>> U sys/fs/nfsclient/nfsnode.h
>> U sys/fs/nfsclient/nfs_clnode.c
>> U sys/mips/mips/exception.S
>> U sys/dev/ahci/ahci.c
>> U sys/dev/ata/chipsets/ata-nvidia.c
>> U sys/dev/ath/ath_hal/ah_eeprom_v4k.c
>> U sys/i386/ibcs2/imgact_coff.c
>> U sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c
>> Updated to revision 224632.
>>
>> Updating to r224648 (this morning) has had no apparent effect on the
>> cited panic: it occurs in the same way, at the same time, accompanied by
>> the same messages (citing the same files and lines).
>>
>> I've attached dmesg.boot from r224602.
>>
>> I will see if I can find a commit that affected at least one of the
>> affected files in the above list that I can revert to avoid the panic,
>> but I'm a bit slow for a while yet, so I figured I'd finally get around
>> to posting this, in the hope that someone cleverer than me will spot
>> the problem and suggest a circumvention to try.
>>
>> And I'm quite willing to try such things.
>>
>> Note: this is FreeBSD/i386; nothing special: no jails; running on real
>> hardware; no attempts to use ZFS....
> I am sure that this is caused by r224614.
> I forgot that vn_fullpath cannot operate on the locked vnode.
>
> Try this.
>
> diff --git a/sys/kern/vfs_mount.c b/sys/kern/vfs_mount.c
> index d601c56..54f123c 100644
> --- a/sys/kern/vfs_mount.c
> +++ b/sys/kern/vfs_mount.c
> @@ -746,13 +746,15 @@ vfs_domount_first(
> struct thread *td, /* Calling thread. */
> struct vfsconf *vfsp, /* File system type. */
> struct vnode *vp, /* Vnode to be covered. */
> + char *ufspath,
> int fsflags, /* Flags common to all filesystems. */
> struct vfsoptlist **optlist /* Options local to the filesystem. */
> )
> {
> struct vattr va;
> + struct nameidata nd;
> struct mount *mp;
> - struct vnode *newdp;
> + struct vnode *newdp, *vp1;
> char *fspath, *fbuf;
> int error;
>
> @@ -761,16 +763,45 @@ vfs_domount_first(
> KASSERT((fsflags & MNT_UPDATE) == 0, ("MNT_UPDATE shouldn't be here"));
>
> /* Construct global filesystem path from vp. */
> + VOP_UNLOCK(vp, 0);
> error = vn_fullpath_global(td, vp, &fspath, &fbuf);
> if (error != 0) {
> - vput(vp);
> + vrele(vp);
> return (error);
> }
> if (strlen(fspath) >= MNAMELEN) {
> - vput(vp);
> + vrele(vp);
> free(fbuf, M_TEMP);
> return (ENAMETOOLONG);
> }
> + if ((vp->v_iflag & VI_DOOMED) != 0) {
> + vrele(vp);
> + free(fbuf, M_TEMP);
> + return (EBADF);
> + }
> +
> + /*
> + * Re-lookup the vnode by path. As a side effect, the vnode is
> + * relocked. If vnode was renamed, return ENOENT.
> + */
> + NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF | MPSAFE | AUDITVNODE1,
> + UIO_SYSSPACE, ufspath, td);
> + error = namei(&nd);
> + if (error != 0) {
> + vrele(vp);
> + free(fbuf, M_TEMP);
> + return (error);
> + }
> + if (NDHASGIANT(&nd))
> + mtx_unlock(&Giant);
> + NDFREE(&nd, NDF_ONLY_PNBUF);
> + vp1 = nd.ni_vp;
> + vrele(vp);
> + if (vp1 != vp) {
> + vput(vp1);
> + free(fbuf, M_TEMP);
> + return (ENOENT);
> + }
>
> /*
> * If the user is not root, ensure that they own the directory
> @@ -1084,14 +1115,11 @@ vfs_domount(
> NDFREE(&nd, NDF_ONLY_PNBUF);
> vp = nd.ni_vp;
> if ((fsflags & MNT_UPDATE) == 0)
> - error = vfs_domount_first(td, vfsp, vp, fsflags, optlist);
> + error = vfs_domount_first(td, vfsp, vp, fspath, fsflags, optlist);
> else
> error = vfs_domount_update(td, vp, fsflags, optlist);
> mtx_unlock(&Giant);
>
> - ASSERT_VI_UNLOCKED(vp, __func__);
> - ASSERT_VOP_UNLOCKED(vp, __func__);
> -
> return (error);
> }
>
--
Martin Matuska
FreeBSD committer
http://blog.vx.sk
--------------050805060602090007070309
Content-Type: text/x-patch;
name="vfs_mount.c.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="vfs_mount.c.patch"
Index: sys/kern/vfs_mount.c
===================================================================
--- sys/kern/vfs_mount.c (revision 224654)
+++ sys/kern/vfs_mount.c (working copy)
@@ -362,6 +362,64 @@ vfs_mergeopts(struct vfsoptlist *toopts, struct vf
}
/*
+ * Verify vnode's global path
+ */
+static int
+vfs_verify_global_path(struct thread *td, struct vnode *vp, char *fspath)
+{
+ struct nameidata nd;
+ struct vnode *vp1;
+ char *rpath, *fbuf;
+ int error;
+
+ ASSERT_VOP_ELOCKED(vp, __func__);
+
+ /* Construct global filesystem path from vp. */
+ VOP_UNLOCK(vp, 0);
+ error = vn_fullpath_global(td, vp, &rpath, &fbuf);
+ if (error != 0) {
+ vrele(vp);
+ return (error);
+ }
+ if (strlen(rpath) >= MNAMELEN) {
+ vrele(vp);
+ free(fbuf, M_TEMP);
+ return (ENAMETOOLONG);
+ }
+ if ((vp->v_iflag & VI_DOOMED) != 0) {
+ vrele(vp);
+ free(fbuf, M_TEMP);
+ return (EBADF);
+ }
+
+ /*
+ * Re-lookup the vnode by path. As a side effect, the vnode is
+ * relocked. If vnode was renamed, return ENOENT.
+ */
+ NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF | MPSAFE | AUDITVNODE1,
+ UIO_SYSSPACE, fspath, td);
+ error = namei(&nd);
+ if (error != 0) {
+ vrele(vp);
+ free(fbuf, M_TEMP);
+ return (error);
+ }
+ NDFREE(&nd, NDF_ONLY_PNBUF);
+ vp1 = nd.ni_vp;
+ vrele(vp);
+ if (vp1 != vp) {
+ vput(vp1);
+ free(fbuf, M_TEMP);
+ return (ENOENT);
+ }
+
+ strlcpy(fspath,rpath,MNAMELEN);
+ free(fbuf, M_TEMP);
+
+ return (error);
+}
+
+/*
* Mount a filesystem.
*/
int
@@ -745,6 +803,7 @@ static int
vfs_domount_first(
struct thread *td, /* Calling thread. */
struct vfsconf *vfsp, /* File system type. */
+ char *fspath, /* Mount path. */
struct vnode *vp, /* Vnode to be covered. */
int fsflags, /* Flags common to all filesystems. */
struct vfsoptlist **optlist /* Options local to the filesystem. */
@@ -753,25 +812,12 @@ vfs_domount_first(
struct vattr va;
struct mount *mp;
struct vnode *newdp;
- char *fspath, *fbuf;
int error;
mtx_assert(&Giant, MA_OWNED);
ASSERT_VOP_ELOCKED(vp, __func__);
KASSERT((fsflags & MNT_UPDATE) == 0, ("MNT_UPDATE shouldn't be here"));
- /* Construct global filesystem path from vp. */
- error = vn_fullpath_global(td, vp, &fspath, &fbuf);
- if (error != 0) {
- vput(vp);
- return (error);
- }
- if (strlen(fspath) >= MNAMELEN) {
- vput(vp);
- free(fbuf, M_TEMP);
- return (ENAMETOOLONG);
- }
-
/*
* If the user is not root, ensure that they own the directory
* onto which we are attempting to mount.
@@ -793,14 +839,12 @@ vfs_domount_first(
}
if (error != 0) {
vput(vp);
- free(fbuf, M_TEMP);
return (error);
}
VOP_UNLOCK(vp, 0);
/* Allocate and initialize the filesystem. */
mp = vfs_mount_alloc(vp, vfsp, fspath, td->td_ucred);
- free(fbuf, M_TEMP);
/* XXXMAC: pass to vfs_mount_alloc? */
mp->mnt_optnew = *optlist;
/* Set the mount level flags. */
@@ -1083,15 +1127,15 @@ vfs_domount(
mtx_lock(&Giant);
NDFREE(&nd, NDF_ONLY_PNBUF);
vp = nd.ni_vp;
- if ((fsflags & MNT_UPDATE) == 0)
- error = vfs_domount_first(td, vfsp, vp, fsflags, optlist);
- else
+ if ((fsflags & MNT_UPDATE) == 0) {
+ error = vfs_verify_global_path(td, vp, fspath);
+ if (error == 0)
+ error = vfs_domount_first(td, vfsp, fspath, vp,
+ fsflags, optlist);
+ } else
error = vfs_domount_update(td, vp, fsflags, optlist);
mtx_unlock(&Giant);
- ASSERT_VI_UNLOCKED(vp, __func__);
- ASSERT_VOP_UNLOCKED(vp, __func__);
-
return (error);
}
@@ -1118,7 +1162,7 @@ unmount(td, uap)
{
struct mount *mp;
struct nameidata nd;
- char *pathbuf, *rpathbuf, *fbuf;
+ char *pathbuf;
int error, id0, id1;
AUDIT_ARG_VALUE(uap->flags);
@@ -1164,15 +1208,10 @@ unmount(td, uap)
UIO_SYSSPACE, pathbuf, td);
if (namei(&nd) == 0) {
NDFREE(&nd, NDF_ONLY_PNBUF);
- if (vn_fullpath_global(td, nd.ni_vp, &rpathbuf,
- &fbuf) == 0) {
- if (strlen(rpathbuf) < MNAMELEN) {
- strlcpy(pathbuf, rpathbuf,
- MNAMELEN);
- }
- free(fbuf, M_TEMP);
- }
- vput(nd.ni_vp);
+ error = vfs_verify_global_path(td, nd.ni_vp,
+ pathbuf);
+ if (error == 0)
+ vput(nd.ni_vp);
}
}
mtx_lock(&mountlist_mtx);
--------------050805060602090007070309--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4E3BA769.3090408>