Re: kern/42348

[Antti Kantee <pooka%cs.hut.fi@localhost>]

The following reply was made to PR kern/42348; it has been noted by GNATS.

From: Antti Kantee <pooka%cs.hut.fi@localhost>
To: dholland%netbsd.org@localhost
Cc: gnats-bugs%netbsd.org@localhost
Subject: Re: kern/42348
Date: Thu, 19 Nov 2009 21:13:49 +0200

 I think this should do the trick (lightly tested, i.e. i'm running the
 patch on my desktop now).  I consider the path buffer to be owned by the
 namei caller in cases relating to SAVESTART.  VOP_ABORTOP on the other
 hand clears file system status.  So we can relookup with SAVESTART as
 long as we release the extra reference.  The only question is if the file
 system is allowed to damage componentname in abortop so it couldn't be
 used for relookup.  It's a gray area, but I don't think it's any more gray
 than the subject in general.  Maybe we can NDINIT it again to be sure?
 
 The only thing I can't remember is why SAVESTART was originally handled
 like this for rename relookup.  I remember we discussed it, but I can't
 remember why.
 
 (nfsd also needs a similar adjustment if this is deemed a sensible approach)
 
 Index: vfs_syscalls.c
 ===================================================================
 RCS file: /cvsroot/src/sys/kern/vfs_syscalls.c,v
 retrieving revision 1.376.4.3
 diff -p -u -r1.376.4.3 vfs_syscalls.c
 --- vfs_syscalls.c     1 Jul 2009 22:47:05 -0000       1.376.4.3
 +++ vfs_syscalls.c     19 Nov 2009 19:04:12 -0000
 @@ -3314,13 +3314,14 @@ do_sys_rename(const char *from, const ch
        struct mount *fs;
        struct lwp *l = curlwp;
        struct proc *p;
 -      uint32_t saveflag;
        int error;
  
        NDINIT(&fromnd, DELETE, LOCKPARENT | SAVESTART | TRYEMULROOT,
            seg, from);
        if ((error = namei(&fromnd)) != 0)
                return (error);
 +      VOP_ABORTOP(fromnd.ni_dvp, &fromnd.ni_cnd);
 +
        if (fromnd.ni_dvp != fromnd.ni_vp)
                VOP_UNLOCK(fromnd.ni_dvp, 0);
        fvp = fromnd.ni_vp;
 @@ -3328,7 +3329,6 @@ do_sys_rename(const char *from, const ch
        fs = fvp->v_mount;
        error = VFS_RENAMELOCK_ENTER(fs);
        if (error) {
 -              VOP_ABORTOP(fromnd.ni_dvp, &fromnd.ni_cnd);
                vrele(fromnd.ni_dvp);
                vrele(fvp);
                goto out1;
 @@ -3359,30 +3359,26 @@ do_sys_rename(const char *from, const ch
             fromnd.ni_cnd.cn_nameptr[1] == '.')) {
                error = EINVAL;
                VFS_RENAMELOCK_EXIT(fs);
 -              VOP_ABORTOP(fromnd.ni_dvp, &fromnd.ni_cnd);
                vrele(fromnd.ni_dvp);
                goto out1;
        }
 -      saveflag = fromnd.ni_cnd.cn_flags & SAVESTART;
 -      fromnd.ni_cnd.cn_flags &= ~SAVESTART;
        vn_lock(fromnd.ni_dvp, LK_EXCLUSIVE | LK_RETRY);
        error = relookup(fromnd.ni_dvp, &fromnd.ni_vp, &fromnd.ni_cnd);
 -      fromnd.ni_cnd.cn_flags |= saveflag;
        if (error) {
                VOP_UNLOCK(fromnd.ni_dvp, 0);
                VFS_RENAMELOCK_EXIT(fs);
 -              VOP_ABORTOP(fromnd.ni_dvp, &fromnd.ni_cnd);
                vrele(fromnd.ni_dvp);
                goto out1;
        }
        VOP_UNLOCK(fromnd.ni_vp, 0);
        if (fromnd.ni_dvp != fromnd.ni_vp)
                VOP_UNLOCK(fromnd.ni_dvp, 0);
 +      vrele(fromnd.ni_dvp); /* from relookup with SAVESTART */
        fvp = fromnd.ni_vp;
  
        NDINIT(&tond, RENAME,
            LOCKPARENT | LOCKLEAF | NOCACHE | SAVESTART | TRYEMULROOT