Re: extended attributes (need help)

[Emmanuel Dreyfus <manu%netbsd.org@localhost>]

The patch I posted yesterday has a race condition, if a user process 
quickly sets two attributes on two different filesystems, then the second
one will panic on VFS_ROOT() in namei() because the root vnode is already 
locked. 

I tried an alternative version that does not resolve a path from the root, 
but I get various hang, crashes and panic. I struggle to find what is wrong
here. Any hint anyone?


/*
 * Autocreate an attribute storage
 */
static struct ufs_extattr_list_entry *
ufs_extattr_autocreate_attr(struct ufsmount *ump, int attrnamespace,
    const char *attrname, struct lwp *l)
{
        struct vnode *root_vp;
        struct vnode *backing_vp;
        struct nameidata nd;
        struct vattr va;
        char path[PATH_MAX + 1];
        struct ufs_extattr_fileheader uef;
        struct ufs_extattr_list_entry *uele;
        int need_close;
        int error;

        root_vp = NULL;
        backing_vp = NULL;
        nd.ni_vp = NULL;
        nd.ni_dvp = NULL;
        uele = NULL;
        need_close = 0;

        /* 
         * We only support system and user namespace autocreation
         */ 
        switch (attrnamespace) {
        case EXTATTR_NAMESPACE_SYSTEM:
                (void)snprintf(path, PATH_MAX, "%s/%s/%s", 
                               UFS_EXTATTR_FSROOTSUBDIR,
                               UFS_EXTATTR_SUBDIR_SYSTEM,
                               attrname);
                break;
        case EXTATTR_NAMESPACE_USER:
                (void)snprintf(path, PATH_MAX, "%s/%s/%s", 
                               UFS_EXTATTR_FSROOTSUBDIR,
                               UFS_EXTATTR_SUBDIR_USER,
                               attrname);
                break;
        default:
                return NULL;
                break;
        }

        /*
         * Get filesystem root
         */
        error = vfs_busy(ump->um_mountp, NULL);
        if (error != 0) {
                printf("%s: vfs_busy() returned %d\n", __func__, error);
                return NULL;
        }

        if ((error = VFS_ROOT(ump->um_mountp, &root_vp)) != 0)
                printf("%s: VFS_ROOT() returned %d\n", __func__, error);

        vfs_unbusy(ump->um_mountp, false, NULL);

        if (error != 0)
                return NULL;

        VOP_UNLOCK(root_vp, 0);

        /*
         * Lookup backing store for creation
         */
        NDINIT(&nd, CREATE, LOCKPARENT|LOCKLEAF, UIO_SYSSPACE, path);
        nd.ni_startdir = root_vp;
        nd.ni_rootdir = root_vp;

        if ((error = lookup(&nd)) != 0)
                goto out;

        KASSERT(nd.ni_vp == NULL);
        KASSERT(nd.ni_dvp != NULL);
        KASSERT(VOP_ISLOCKED(nd.ni_dvp) == LK_EXCLUSIVE);
        KASSERT(VOP_ISLOCKED(root_vp) == 0);

        /*
         * Create backing store
         */
        VATTR_NULL(&va);
        va.va_type = VREG;
        va.va_mode = 0600;
        va.va_vaflags |= VA_EXCLUSIVE;

        error = VOP_CREATE(nd.ni_dvp, &backing_vp, &nd.ni_cnd, &va);
        if (error != 0) {
                printf("%s: cannot create %s, error = %d\n", 
                       __func__, path, error);
                goto out;
        }

        KASSERT(backing_vp != NULL);
        KASSERT(VOP_ISLOCKED(backing_vp) == LK_EXCLUSIVE);
        KASSERT(VOP_ISLOCKED(nd.ni_dvp) == LK_EXCLUSIVE);

        /*
         * Open backing store for writing
         */
        error = VOP_OPEN(backing_vp, FREAD|FWRITE, l->l_cred);
        if (error != 0) {
                printf("%s: cannot open %s, error = %d\n",
                       __func__, path, error);
                goto out;
        }

        /*
         * Remember we have to close the file on failure
         */
        need_close = 1;

        /*
         * This is decreased by vn_close
         */
        mutex_enter(&backing_vp->v_interlock);
        backing_vp->v_writecount++;
        mutex_exit(&backing_vp->v_interlock);

        /*
         * Now write extended attribute header in backing store fiie
         */     
        uef.uef_magic = UFS_EXTATTR_MAGIC;
        uef.uef_version = UFS_EXTATTR_VERSION;
        uef.uef_size = UFS_EXTATTR_AUTOCREATE;

        error = vn_rdwr(UIO_WRITE, backing_vp, &uef, sizeof(uef), 0,
                        UIO_SYSSPACE, IO_NODELOCKED|IO_APPEND, 
                        l->l_cred, NULL, l);

        KASSERT(VOP_ISLOCKED(backing_vp) == LK_EXCLUSIVE);
        VOP_UNLOCK(backing_vp, 0);

        if (error != 0) {
                printf("%s: write uef header failed for %s, error = %d\n", 
                       __func__, attrname, error);
                goto out;
        }

        /*
         * ufs_extattr_enable_with_open does it, we do it too
         * but I am not sure of the purpose
         */
        vref(backing_vp);

        /*
         * Enable the attribute. 
         */
        error = ufs_extattr_enable(ump, attrnamespace,
                                   attrname, backing_vp, l);
        if (error != 0) {
                printf("%s: enable %s failed, error %d\n", 
                       __func__, attrname, error);
                goto out;
        }

        uele = ufs_extattr_find_attr(ump, attrnamespace, attrname);
        if (uele == NULL) {
                printf("%s: attribute %s created but not found!\n",
                       __func__, attrname);
                goto out;
        }

out:
        if (backing_vp != NULL) {
                /*
                 * Success, we keep the vnode open and unlocked
                 */
                if (uele != NULL) {
                        KASSERT(VOP_ISLOCKED(backing_vp) == 0);
                        printf("%s: UFS autocreated EA backing store "
                               "for attribute %s\n", 
                               ump->um_mountp->mnt_stat.f_mntonname,
                               attrname);
                }
                /*
                 * Failure and it was left open, close it. 
                 */
                else if (need_close) {
                        KASSERT(VOP_ISLOCKED(backing_vp) == 0);
                        vn_close(backing_vp, FREAD|FWRITE, l->l_cred);
                }
                /*
                 * Failure and it was unopen (but locked), just forget it
                 */
                else {
                        KASSERT(VOP_ISLOCKED(backing_vp) == LK_EXCLUSIVE);
                        vput(backing_vp);
                }
        }

        if (nd.ni_dvp != NULL) {
                KASSERT(VOP_ISLOCKED(nd.ni_dvp) == LK_EXCLUSIVE);
                vput(nd.ni_dvp);
        }
        
        if (root_vp != NULL) {
                KASSERT(VOP_ISLOCKED(root_vp) == 0);
                vrele(root_vp);
        }

        return uele;
}

-- 
Emmanuel Dreyfus
manu%netbsd.org@localhost