CVS commit: src

["Taylor R Campbell" <riastradh%netbsd.org@localhost>]

Module Name:    src
Committed By:   riastradh
Date:           Sat Sep  5 02:47:04 UTC 2020

Modified Files:
        src/sys/miscfs/genfs: genfs_rename.c
        src/tests/fs/vfs: t_renamerace.c

Log Message:
genfs_rename: Fix deadlocks in cross-directory cyclic rename.

Reproducer:

A: for (;;) { mkdir("c", 0600); mkdir("c/d", 0600); mkdir("c/d/e", 0600);
    rmdir("c/d/e"); rmdir("c/d"); }
B: for (;;) { mkdir("c", 0600); mkdir("c/d", 0600); mkdir("c/d/e", 0600);
    rename("c", "c/d/e"); }
C: for (;;) { mkdir("c", 0600); mkdir("c/d", 0600); mkdir("c/d/e", 0600);
    rename("c/d/e", "c"); }

Deadlock:

- A holds c and wants to lock d; and either
- B holds . and d and wants to lock c, or
- C holds . and d and wants to lock c.

The problem with these is that genfs_rename_enter_separate in B or C
tried lock order .->d->c->e (in A/B, fdvp->tdvp->fvp->tvp; in A/C,
tdvp->fdvp->tvp->fvp) which violates the ancestor->descendant order
.->c->d->e.

The resolution is to change B to do fdvp->fvp->tdvp->tvp and C to do
tdvp->tvp->fdvp->fvp.  But there's an edge case: tvp and fvp might be
the same (hard links), and we can't detect that until after we've
looked them both up -- and in some file systems (I'm looking at you,
ufs), there is no mere lookup operation, only lookup-and-lock, so we
can't even hold the lock on one of tvp or fvp when we look up the
other one if there's a chance they might be the same.

Fortunately the cases
(a) tvp = fvp
(b) tvp or fvp is a directory
are mutually exclusive as long as directories cannot be hard-linked.
In case (a) we can just defer locking {tvp, fvp} until the end, because
it can't possibly have {fdvp or fvp, tdvp or tvp} as descendants.  In
case (b) we can just lock them in the order fdvp->fvp->tdvp->tvp or
tdvp->tvp->fdvp->fvp if the first one of {fvp, tvp} is a directory,
because it can't possibly coincide with the second one of {fvp, tvp}.

With this change, we can now prove that the locking order is consistent
with the ancestor->descendant partial ordering.  Where two nodes are
incommensurate under that partial ordering, they are only ever locked
by rename and there is only ever one rename at a time.

Proof:

- For same-directory renames, genfs_rename_enter_common locks the
  directory first and then the children.  The order
  directory->child[i] is consistent with ancestor->descendant and
  child[0]/child[1] are incommensurate.

- For cross-directory renames:

  . While a rename is in progress and the fs-wide rename lock is held,
    directories can be created or removed but not changed, so the
    outcome of gro_genealogy -- which, given fdvp and tdvp, returns
    the node N relating fdvp/N/.../tdvp or null if there is none --
    can only transition from finding N to not finding N, if one of
    the directories is removed while any of the vnodes are unlocked.
    Merely creating directories cannot change the ancestry of tdvp,
    and concurrent renames are not possible.

    Thus, if a gro_genealogy determined the operation to have the
    form fdvp/N/.../tdvp, then it might cease to have that form, but
    only because tdvp was removed which will harmlessly cause the
    rename to fail later on.  Similarly, if gro_genealogy determined
    the operation _not_ to have the form fdvp/N/.../tdvp then it
    can't begin to have that form until after the rename has
    completed.

    The lock order is,

    => for fdvp/.../tdvp:
       1. lock fdvp
       2. lookup(/lock/unlock) fvp (consistent with fdvp->fvp)
       3. lock fvp if a directory (consistent with fdvp->fvp)
       4. lock tdvp (consistent with fdvp->tdvp and possibly fvp->tdvp)
       5. lookup(/lock/unlock) tvp (consistent with tdvp->tvp)
       6. lock fvp if a nondirectory (fvp->t* or fvp->fdvp is impossible)
       7. lock tvp if not fvp (tvp->f* is impossible unless tvp=fvp)

    => for incommensurate fdvp & tdvp, or for tdvp/.../fdvp:
       1. lock tdvp
       2. lookup(/lock/unlock) tvp (consistent with tdvp->tvp)
       3. lock tvp if a directory (consistent with tdvp->tvp)
       4. lock fdvp (either incommensurate with tdvp and/or tvp, or
          consistent with tdvp(->tvp)->fdvp)
       5. lookup(/lock/unlock) fvp (consistent with fdvp->fvp)
       6. lock tvp if a nondirectory (tvp->f* or tvp->tdvp is impossible)
       7. lock fvp if not tvp (fvp->t* is impossible unless fvp=tvp)

Deadlocks found by hannken@; resolution worked out with dholland@.

XXX I think we could improve concurrency somewhat -- with a likely
big win for applications like tar and rsync that create many files
with temporary names and then rename them to the permanent one in the
same directory -- by making vfs_renamelock a reader/writer lock: any
number of same-directory renames, or exactly one cross-directory
rename, at any one time.


To generate a diff of this commit:
cvs rdiff -u -r1.4 -r1.5 src/sys/miscfs/genfs/genfs_rename.c
cvs rdiff -u -r1.37 -r1.38 src/tests/fs/vfs/t_renamerace.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.