Skip to content

Commit

Permalink
Remove p9.fidRef.openedMu
Browse files Browse the repository at this point in the history 
openedMu has lock ordering violations. Most locks go through OpenedFlag(),
which is usually taken after renameMu and opMu. On the other hand, Tlopen takes
openedMu before renameMu and opMu (via safelyRead).

Resolving this violation is simple: just drop openedMu. The opened and
openFlags fields are already protected by opMu in most cases, renameMu (for
write) in one case (via safelyGlobal), and only in doWalk by neither.

This is a bit ugly because opMu is supposed to be a "semantic" lock, but it
works. I'm open to other suggestions.

Note that doWalk has a race condition where a FID may open after the open check
but before actually walking. This race existed before this change as well; it
is not clear if it is problematic.

Signed-off-by: Chris Koch <chrisko@google.com>
  • Loading branch information
@prattmic @hugelgupf
prattmic authored and hugelgupf committed Aug 22, 2023
1 parent 94ba285 commit 474d91e
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 50 deletions.
87 changes: 47 additions & 40 deletions p9/handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,19 +308,6 @@ func (t *tlopen) handle(cs *connState) message {
}
defer ref.DecRef()

ref.openedMu.Lock()
defer ref.openedMu.Unlock()

// Has it been opened already?
if ref.opened || !CanOpen(ref.mode) {
return newErr(linux.EINVAL)
}

// Is this an attempt to open a directory as writable? Don't accept.
if ref.mode.IsDir() && t.Flags.Mode() != ReadOnly {
return newErr(linux.EINVAL)
}

var (
qid QID
ioUnit uint32
Expand All @@ -331,6 +318,16 @@ func (t *tlopen) handle(cs *connState) message {
return linux.EINVAL
}

// Has it been opened already?
if ref.opened || !CanOpen(ref.mode) {
return linux.EINVAL
}

// Is this an attempt to open a directory as writable? Don't accept.
if ref.mode.IsDir() && t.Flags.Mode() != ReadOnly {
return linux.EISDIR
}

// Do the open.
qid, ioUnit, err = ref.file.Open(t.Flags)
return err
Expand Down Expand Up @@ -371,7 +368,7 @@ func (t *tlcreate) do(cs *connState, uid UID) (*rlcreate, error) {
}

// Not allowed on open directories.
if _, opened := ref.OpenFlags(); opened {
if ref.opened {
return linux.EINVAL
}

Expand Down Expand Up @@ -442,7 +439,7 @@ func (t *tsymlink) do(cs *connState, uid UID) (*rsymlink, error) {
}

// Not allowed on open directories.
if _, opened := ref.OpenFlags(); opened {
if ref.opened {
return linux.EINVAL
}

Expand Down Expand Up @@ -484,7 +481,7 @@ func (t *tlink) handle(cs *connState) message {
}

// Not allowed on open directories.
if _, opened := ref.OpenFlags(); opened {
if ref.opened {
return linux.EINVAL
}

Expand Down Expand Up @@ -529,7 +526,7 @@ func (t *trenameat) handle(cs *connState) message {
}

// Not allowed on open directories.
if _, opened := ref.OpenFlags(); opened {
if ref.opened {
return linux.EINVAL
}

Expand Down Expand Up @@ -574,7 +571,7 @@ func (t *tunlinkat) handle(cs *connState) message {
}

// Not allowed on open directories.
if _, opened := ref.OpenFlags(); opened {
if ref.opened {
return linux.EINVAL
}

Expand Down Expand Up @@ -721,13 +718,12 @@ func (t *tread) handle(cs *connState) message {
switch ref.pendingXattr.op {
case xattrNone:
// Has it been opened already?
openFlags, opened := ref.OpenFlags()
if !opened {
if !ref.opened {
return linux.EINVAL
}

// Can it be read? Check permissions.
if openFlags&OpenFlagsModeMask == WriteOnly {
if ref.openFlags&OpenFlagsModeMask == WriteOnly {
return linux.EPERM
}

Expand Down Expand Up @@ -786,13 +782,12 @@ func (t *twrite) handle(cs *connState) message {
switch ref.pendingXattr.op {
case xattrNone:
// Has it been opened already?
openFlags, opened := ref.OpenFlags()
if !opened {
if !ref.opened {
return linux.EINVAL
}

// Can it be written? Check permissions.
if openFlags&OpenFlagsModeMask == ReadOnly {
if ref.openFlags&OpenFlagsModeMask == ReadOnly {
return linux.EPERM
}

Expand Down Expand Up @@ -849,7 +844,7 @@ func (t *tmknod) do(cs *connState, uid UID) (*rmknod, error) {
}

// Not allowed on open directories.
if _, opened := ref.OpenFlags(); opened {
if ref.opened {
return linux.EINVAL
}

Expand Down Expand Up @@ -893,7 +888,7 @@ func (t *tmkdir) do(cs *connState, uid UID) (*rmkdir, error) {
}

// Not allowed on open directories.
if _, opened := ref.OpenFlags(); opened {
if ref.opened {
return linux.EINVAL
}

Expand Down Expand Up @@ -1054,7 +1049,7 @@ func (t *treaddir) handle(cs *connState) message {
}

// Has it been opened already?
if _, opened := ref.OpenFlags(); !opened {
if !ref.opened {
return linux.EINVAL
}

Expand Down Expand Up @@ -1082,7 +1077,7 @@ func (t *tfsync) handle(cs *connState) message {

if err := ref.safelyRead(func() (err error) {
// Has it been opened already?
if _, opened := ref.OpenFlags(); !opened {
if !ref.opened {
return linux.EINVAL
}

Expand Down Expand Up @@ -1295,12 +1290,18 @@ func (t *twalk) handle(cs *connState) message {
}
defer ref.DecRef()

// Has it been opened already?
// That as OK as long as newFID is different.
// Note this violates the spec, but the Linux client
// does too, so we have little choice.
if _, opened := ref.OpenFlags(); opened && t.fid == t.newFID {
return newErr(linux.EBUSY)
if err := ref.safelyRead(func() error {
// Has it been opened already?
//
// That as OK as long as newFID is different. Note this
// violates the spec, but the Linux client does too, so we have
// little choice.
if ref.opened && t.fid == t.newFID {
return linux.EBUSY
}
return nil
}); err != nil {
return newErr(err)
}

// Is this an empty list? Handle specially. We don't actually need to
Expand All @@ -1325,12 +1326,18 @@ func (t *twalkgetattr) handle(cs *connState) message {
}
defer ref.DecRef()

// Has it been opened already?
// That as OK as long as newFID is different.
// Note this violates the spec, but the Linux client
// does too, so we have little choice.
if _, opened := ref.OpenFlags(); opened && t.fid == t.newFID {
return newErr(linux.EBUSY)
if err := ref.safelyRead(func() error {
// Has it been opened already?
//
// That as OK as long as newFID is different. Note this
// violates the spec, but the Linux client does too, so we have
// little choice.
if ref.opened && t.fid == t.newFID {
return linux.EBUSY
}
return nil
}); err != nil {
return newErr(err)
}

// Is this an empty list? Handle specially. We don't actually need to
Expand Down
14 changes: 4 additions & 10 deletions p9/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,11 @@ type fidRef struct {
// The node above will be closed only when refs reaches zero.
refs int64

// openedMu protects opened and openFlags.
openedMu sync.Mutex

// opened indicates whether this has been opened already.
//
// This is updated in handlers.go.
//
// opened is protected by pathNode.opMu or renameMu (for write).
opened bool

// mode is the fidRef's mode from the walk. Only the type bits are
Expand All @@ -194,6 +193,8 @@ type fidRef struct {
// openFlags is the mode used in the open.
//
// This is updated in handlers.go.
//
// opened is protected by pathNode.opMu or renameMu (for write).
openFlags OpenFlags

// pathNode is the current pathNode for this fid.
Expand All @@ -215,13 +216,6 @@ type fidRef struct {
parent *fidRef
}

// OpenFlags returns the flags the file was opened with and true iff the fid was opened previously.
func (f *fidRef) OpenFlags() (OpenFlags, bool) {
f.openedMu.Lock()
defer f.openedMu.Unlock()
return f.openFlags, f.opened
}

// IncRef increases the references on a fid.
func (f *fidRef) IncRef() {
atomic.AddInt64(&f.refs, 1)
Expand Down

0 comments on commit 474d91e

Please sign in to comment.