From 474d91ee85f93c16693fd275fe41df393fb94c42 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Tue, 22 Aug 2023 06:24:23 -0700 Subject: [PATCH] Remove p9.fidRef.openedMu 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 --- p9/handlers.go | 87 +++++++++++++++++++++++++++----------------------- p9/server.go | 14 +++----- 2 files changed, 51 insertions(+), 50 deletions(-) diff --git a/p9/handlers.go b/p9/handlers.go index d9d1c61..4ee6c61 100644 --- a/p9/handlers.go +++ b/p9/handlers.go @@ -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 @@ -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 @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 @@ -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 diff --git a/p9/server.go b/p9/server.go index 3ae8ec3..c6e9ff3 100644 --- a/p9/server.go +++ b/p9/server.go @@ -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 @@ -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. @@ -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)