Module Name: src
Committed By: snj
Date: Thu Dec 4 05:43:55 UTC 2014
Modified Files:
src/sys/ufs/ufs [netbsd-6]: ufs_extattr.c
Log Message:
Pull up following revision(s) (requested by manu in ticket #1197):
sys/ufs/ufs/ufs_extattr.c: revision 1.41, 1.45
Remove always-true condition and note that the current code is
suboptimal.
--
Fix UFS1 extended attribute backend autocreation deadlock
UFS1 extended attribute backend autocration goes through a vn_open()
to create the backend file, and this forces us to release the lock
on the target node, in case the target is within the parents of the
backend file. That created a window within which another thread could
acquire a lock on the target vnode and deadlock awaiting for the
mount extended attribute lock.
We fix the problem by also releasing the mount extended attribute lock
when calling vn_open(), but that lets another thread race us for backend
creation. We just detect this using O_EXCL for vn_open() and by checking
for EEXIST return code. If we are raced, we fail backend creation but
this is not a problem since another thread succeeded on it: we just have
to use the result.
To generate a diff of this commit:
cvs rdiff -u -r1.36.2.3 -r1.36.2.4 src/sys/ufs/ufs/ufs_extattr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/ufs/ufs/ufs_extattr.c
diff -u src/sys/ufs/ufs/ufs_extattr.c:1.36.2.3 src/sys/ufs/ufs/ufs_extattr.c:1.36.2.4
--- src/sys/ufs/ufs/ufs_extattr.c:1.36.2.3 Thu Dec 4 05:38:54 2014
+++ src/sys/ufs/ufs/ufs_extattr.c Thu Dec 4 05:43:55 2014
@@ -1,4 +1,4 @@
-/* $NetBSD: ufs_extattr.c,v 1.36.2.3 2014/12/04 05:38:54 snj Exp $ */
+/* $NetBSD: ufs_extattr.c,v 1.36.2.4 2014/12/04 05:43:55 snj Exp $ */
/*-
* Copyright (c) 1999-2002 Robert N. M. Watson
@@ -48,7 +48,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ufs_extattr.c,v 1.36.2.3 2014/12/04 05:38:54 snj Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ufs_extattr.c,v 1.36.2.4 2014/12/04 05:43:55 snj Exp $");
#ifdef _KERNEL_OPT
#include "opt_ffs.h"
@@ -158,9 +158,9 @@ ufs_extattr_valid_attrname(int attrnames
/*
* Autocreate an attribute storage
*/
-static struct ufs_extattr_list_entry *
+static int
ufs_extattr_autocreate_attr(struct vnode *vp, int attrnamespace,
- const char *attrname, struct lwp *l)
+ const char *attrname, struct lwp *l, struct ufs_extattr_list_entry **uelep)
{
struct mount *mp = vp->v_mount;
struct ufsmount *ump = VFSTOUFS(mp);
@@ -194,38 +194,51 @@ ufs_extattr_autocreate_attr(struct vnode
break;
default:
PNBUF_PUT(path);
- return NULL;
+ *uelep = NULL;
+ return EINVAL;
break;
}
/*
- * When setting attribute on the root vnode, we get it
- * already locked, and vn_open/namei/VFS_ROOT will try to
- * look it, causing a panic. Unlock it first.
+ * Release extended attribute mount lock, otherwise
+ * we can deadlock with another thread that would lock
+ * vp after we unlock it below, and call
+ * ufs_extattr_uepm_lock(ump), for instance
+ * in ufs_getextattr().
+ */
+ ufs_extattr_uepm_unlock(ump);
+
+ /*
+ * XXX unlock/lock should only be done when setting extattr
+ * on backing store or one of its parent directory
+ * including root, but we always do it for now.
*/
- if (vp->v_vflag && VV_ROOT) {
- KASSERT(VOP_ISLOCKED(vp) == LK_EXCLUSIVE);
- VOP_UNLOCK(vp);
- }
- KASSERT(VOP_ISLOCKED(vp) == 0);
+ KASSERT(VOP_ISLOCKED(vp) == LK_EXCLUSIVE);
+ VOP_UNLOCK(vp);
pb = pathbuf_create(path);
NDINIT(&nd, CREATE, LOCKPARENT, pb);
- error = vn_open(&nd, O_CREAT|O_RDWR, 0600);
+ /*
+ * Since we do not hold ufs_extattr_uepm_lock anymore,
+ * another thread may race with us for backend creation,
+ * but only one can succeed here thanks to O_EXCL
+ */
+ error = vn_open(&nd, O_CREAT|O_EXCL|O_RDWR, 0600);
/*
- * Reacquire the lock on the vnode if it was root.
+ * Reacquire the lock on the vnode
*/
KASSERT(VOP_ISLOCKED(vp) == 0);
- if (vp->v_vflag && VV_ROOT)
- vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
- KASSERT(VOP_ISLOCKED(vp) == LK_EXCLUSIVE);
+ vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
+
+ ufs_extattr_uepm_lock(ump);
if (error != 0) {
pathbuf_destroy(pb);
PNBUF_PUT(path);
- return NULL;
+ *uelep = NULL;
+ return error;
}
KASSERT(nd.ni_vp != NULL);
@@ -253,7 +266,8 @@ ufs_extattr_autocreate_attr(struct vnode
printf("%s: write uef header failed for %s, error = %d\n",
__func__, attrname, error);
vn_close(backing_vp, FREAD|FWRITE, l->l_cred);
- return NULL;
+ *uelep = NULL;
+ return error;
}
/*
@@ -266,7 +280,8 @@ ufs_extattr_autocreate_attr(struct vnode
printf("%s: enable %s failed, error %d\n",
__func__, attrname, error);
vn_close(backing_vp, FREAD|FWRITE, l->l_cred);
- return NULL;
+ *uelep = NULL;
+ return error;
}
uele = ufs_extattr_find_attr(ump, attrnamespace, attrname);
@@ -274,13 +289,15 @@ ufs_extattr_autocreate_attr(struct vnode
printf("%s: atttribute %s created but not found!\n",
__func__, attrname);
vn_close(backing_vp, FREAD|FWRITE, l->l_cred);
- return NULL;
+ *uelep = NULL;
+ return ESRCH; /* really internal error */
}
printf("%s: EA backing store autocreated for %s\n",
mp->mnt_stat.f_mntonname, attrname);
- return uele;
+ *uelep = uele;
+ return 0;
}
/*
@@ -1343,10 +1360,17 @@ ufs_extattr_set(struct vnode *vp, int at
attribute = ufs_extattr_find_attr(ump, attrnamespace, name);
if (!attribute) {
- attribute = ufs_extattr_autocreate_attr(vp, attrnamespace,
- name, l);
- if (!attribute)
- return (ENODATA);
+ error = ufs_extattr_autocreate_attr(vp, attrnamespace,
+ name, l, &attribute);
+ if (error == EEXIST) {
+ /* Another thread raced us for backend creation */
+ error = 0;
+ attribute =
+ ufs_extattr_find_attr(ump, attrnamespace, name);
+ }
+
+ if (error || !attribute)
+ return ENODATA;
}
/*
