[src/trunk]: src/sys/ufs/ufs Fix UFS1 extended attribute backend autocreation...

[manu <manu%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/8fb9fe88c972
branches:  trunk
changeset: 803866:8fb9fe88c972
user:      manu <manu%NetBSD.org@localhost>
date:      Sat Nov 15 05:03:55 2014 +0000

description:
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.

diffstat:

 sys/ufs/ufs/ufs_extattr.c |  59 +++++++++++++++++++++++++++++++++++-----------
 1 files changed, 44 insertions(+), 15 deletions(-)

diffs (142 lines):

diff -r fa3586b756de -r 8fb9fe88c972 sys/ufs/ufs/ufs_extattr.c
--- a/sys/ufs/ufs/ufs_extattr.c Sat Nov 15 04:47:11 2014 +0000
+++ b/sys/ufs/ufs/ufs_extattr.c Sat Nov 15 05:03:55 2014 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ufs_extattr.c,v 1.44 2014/11/14 10:09:50 manu Exp $    */
+/*     $NetBSD: ufs_extattr.c,v 1.45 2014/11/15 05:03:55 manu 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.44 2014/11/14 10:09:50 manu Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ufs_extattr.c,v 1.45 2014/11/15 05:03:55 manu Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_ffs.h"
@@ -210,9 +210,9 @@
 /*
  * 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);
@@ -246,11 +246,21 @@
                break;
        default:
                PNBUF_PUT(path);
-               return NULL;
+               *uelep = NULL;
+               return EINVAL;
                break;
        }
 
        /*
+        * 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.
@@ -261,7 +271,12 @@
        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
@@ -269,10 +284,13 @@
        KASSERT(VOP_ISLOCKED(vp) == 0);
        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);
@@ -300,7 +318,8 @@
                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;
        }
 
        /*
@@ -313,7 +332,8 @@
                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);
@@ -321,13 +341,15 @@
                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;
 }
 
 /*
@@ -1405,10 +1427,17 @@
 
        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;
        }
 
        /*