Automated rollback of changelist 240657604

PiperOrigin-RevId: 241434161
Change-Id: I9ec734e50cef5b39203e8bf37de2d91d24943f1e

2 files changed, 212 insertions, 174 deletions

diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go
index 1d64458e5..45b455430 100644
--- a/runsc/fsgofer/fsgofer.go
+++ b/runsc/fsgofer/fsgofer.go
@@ -176,23 +176,21 @@ func (a *attachPoint) makeQID(stat syscall.Stat_t) p9.QID {
 }
 
 // localFile implements p9.File wrapping a local file. The underlying file
-// is opened during Walk() and stored in 'controlFile' to be used with other
-// operations. The control file is opened as readonly, unless it's a symlink
-// which requires O_PATH. 'controlFile' is dup'ed when Walk(nil) is called
-// to clone the file. This reduces the number of walks that need to be done by
-// the host file system when files are reused.
+// is opened during Walk() and stored in 'file' to be used with other
+// operations. The file is opened as readonly, unless it's a symlink or there is
+// no read access, which requires O_PATH. 'file' is dup'ed when Walk(nil) is
+// called to clone the file. This reduces the number of walks that need to be
+// done by the host file system when files are reused.
 //
-// 'openedFile' is assigned when Open() is called. If requested open mode is
-// a subset of controlFile's mode, it's possible to use the same file. If mode
-// is not a subset, then another file is opened. Consequently, 'openedFile'
-// could have a mode wider than requested and must be verified before read/write
-// operations. Before the file is opened and after it's closed, 'mode' is set to
-// an invalid value to prevent an unopened file from being used.
+// The file may be reopened if the requested mode in Open() is not a subset of
+// current mode. Consequently, 'file' could have a mode wider than requested and
+// must be verified before read/write operations. Before the file is opened and
+// after it's closed, 'mode' is set to an invalid value to prevent an unopened
+// file from being used.
 //
-// The reason that the control file is never opened as read-write is for better
+// The reason that the file is not opened initially as read-write is for better
 // performance with 'overlay2' storage driver. overlay2 eagerly copies the
 // entire file up when it's opened in write mode, and would perform badly when
-// multiple files are being opened for read-only (esp. startup).
 type localFile struct {
 	p9.DefaultWalkGetAttr
 
@@ -202,12 +200,9 @@ type localFile struct {
 	// hostPath will be safely updated by the Renamed hook.
 	hostPath string
 
-	// controlFile is opened when localFile is created and it's never nil.
-	controlFile *os.File
-
-	// openedFile is nil until localFile is opened. It may point to controlFile
-	// or be a new file struct. See struct comment for more details.
-	openedFile *os.File
+	// file is opened when localFile is created and it's never nil. It may be
+	// reopened...
+	file *os.File
 
 	// mode is the mode in which the file was opened. Set to invalidMode
 	// if localFile isn't opened.
@@ -220,7 +215,22 @@ type localFile struct {
 	readDirMu sync.Mutex
 }
 
-func openAnyFile(parent *localFile, name string) (*os.File, string, error) {
+func openAnyFileFromParent(parent *localFile, name string) (*os.File, string, error) {
+	path := path.Join(parent.hostPath, name)
+	f, err := openAnyFile(path, func(mode int) (*os.File, error) {
+		fd, err := syscall.Openat(parent.fd(), name, openFlags|mode, 0)
+		if err != nil {
+			return nil, err
+		}
+		return os.NewFile(uintptr(fd), path), nil
+	})
+	return f, path, err
+}
+
+// openAnyFile attempts to open the file in O_RDONLY and if it fails fallsback
+// to O_PATH. 'path' is used for logging messages only. 'fn' is what does the
+// actual file open and is customizable by the caller.
+func openAnyFile(path string, fn func(mode int) (*os.File, error)) (*os.File, error) {
 	// Attempt to open file in the following mode in order:
 	//   1. RDONLY | NONBLOCK: for all files, works for directories and ro mounts too.
 	//      Use non-blocking to prevent getting stuck inside open(2) for FIFOs. This option
@@ -229,9 +239,9 @@ func openAnyFile(parent *localFile, name string) (*os.File, string, error) {
 	modes := []int{syscall.O_RDONLY | syscall.O_NONBLOCK, unix.O_PATH}
 
 	var err error
-	var fd int
+	var file *os.File
 	for i, mode := range modes {
-		fd, err = syscall.Openat(parent.controlFD(), name, openFlags|mode, 0)
+		file, err = fn(mode)
 		if err == nil {
 			// openat succeeded, we're done.
 			break
@@ -239,20 +249,19 @@ func openAnyFile(parent *localFile, name string) (*os.File, string, error) {
 		switch e := extractErrno(err); e {
 		case syscall.ENOENT:
 			// File doesn't exist, no point in retrying.
-			return nil, "", e
+			return nil, e
 		}
 		// openat failed. Try again with next mode, preserving 'err' in case this
 		// was the last attempt.
-		log.Debugf("Attempt %d to open file failed, mode: %#x, path: %s/%s, err: %v", i, openFlags|mode, parent.controlFile.Name(), name, err)
+		log.Debugf("Attempt %d to open file failed, mode: %#x, path: %q, err: %v", i, openFlags|mode, path, err)
 	}
 	if err != nil {
 		// All attempts to open file have failed, return the last error.
-		log.Debugf("Failed to open file, path: %s/%s, err: %v", parent.controlFile.Name(), name, err)
-		return nil, "", extractErrno(err)
+		log.Debugf("Failed to open file, path: %q, err: %v", path, err)
+		return nil, extractErrno(err)
 	}
 
-	newPath := path.Join(parent.hostPath, name)
-	return os.NewFile(uintptr(fd), newPath), newPath, nil
+	return file, nil
 }
 
 func getSupportedFileType(stat syscall.Stat_t) (fileType, error) {
@@ -279,7 +288,7 @@ func newLocalFile(a *attachPoint, file *os.File, path string, stat syscall.Stat_
 	return &localFile{
 		attachPoint: a,
 		hostPath:    path,
-		controlFile: file,
+		file:        file,
 		mode:        invalidMode,
 		ft:          ft,
 	}, nil
@@ -314,33 +323,26 @@ func fchown(fd int, uid p9.UID, gid p9.GID) error {
 	return syscall.Fchownat(fd, "", int(uid), int(gid), linux.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW)
 }
 
-func (l *localFile) controlFD() int {
-	return int(l.controlFile.Fd())
-}
-
-func (l *localFile) openedFD() int {
-	if l.openedFile == nil {
-		panic(fmt.Sprintf("trying to use an unopened file: %q", l.controlFile.Name()))
-	}
-	return int(l.openedFile.Fd())
+func (l *localFile) fd() int {
+	return int(l.file.Fd())
 }
 
 // Open implements p9.File.
 func (l *localFile) Open(mode p9.OpenFlags) (*fd.FD, p9.QID, uint32, error) {
-	if l.openedFile != nil {
-		panic(fmt.Sprintf("attempting to open already opened file: %q", l.controlFile.Name()))
+	if l.isOpen() {
+		panic(fmt.Sprintf("attempting to open already opened file: %q", l.file.Name()))
 	}
 
 	// Check if control file can be used or if a new open must be created.
 	var newFile *os.File
 	if mode == p9.ReadOnly {
-		log.Debugf("Open reusing control file, mode: %v, %q", mode, l.controlFile.Name())
-		newFile = l.controlFile
+		log.Debugf("Open reusing control file, mode: %v, %q", mode, l.file.Name())
+		newFile = l.file
 	} else {
 		// Ideally reopen would call name_to_handle_at (with empty name) and
 		// open_by_handle_at to reopen the file without using 'hostPath'. However,
 		// name_to_handle_at and open_by_handle_at aren't supported by overlay2.
-		log.Debugf("Open reopening file, mode: %v, %q", mode, l.controlFile.Name())
+		log.Debugf("Open reopening file, mode: %v, %q", mode, l.file.Name())
 		var err error
 
 		newFile, err = os.OpenFile(l.hostPath, openFlags|mode.OSFlags(), 0)
@@ -351,7 +353,9 @@ func (l *localFile) Open(mode p9.OpenFlags) (*fd.FD, p9.QID, uint32, error) {
 
 	stat, err := stat(int(newFile.Fd()))
 	if err != nil {
-		newFile.Close()
+		if newFile != l.file {
+			newFile.Close()
+		}
 		return nil, p9.QID{}, 0, extractErrno(err)
 	}
 
@@ -361,8 +365,13 @@ func (l *localFile) Open(mode p9.OpenFlags) (*fd.FD, p9.QID, uint32, error) {
 		fd = newFDMaybe(newFile)
 	}
 
-	// Set fields on success
-	l.openedFile = newFile
+	// Close old file in case a new one was created.
+	if newFile != l.file {
+		if err := l.file.Close(); err != nil {
+			log.Warningf("Error closing file %q: %v", l.file.Name(), err)
+		}
+		l.file = newFile
+	}
 	l.mode = mode
 	return fd, l.attachPoint.makeQID(stat), 0, nil
 }
@@ -377,10 +386,9 @@ func (l *localFile) Create(name string, mode p9.OpenFlags, perm p9.FileMode, uid
 		return nil, nil, p9.QID{}, 0, syscall.EBADF
 	}
 
-	// Use a single file for both 'controlFile' and 'openedFile'. Mode must
-	// include read for control and whichever else was requested by caller. Note
-	// that resulting file might have a wider mode than needed for each particular
-	// case.
+	// 'file' may be used for other operations (e.g. Walk), so read access is
+	// always added to flags. Note that resulting file might have a wider mode
+	// than needed for each particular case.
 	flags := openFlags | syscall.O_CREAT | syscall.O_EXCL
 	if mode == p9.WriteOnly {
 		flags |= syscall.O_RDWR
@@ -388,14 +396,14 @@ func (l *localFile) Create(name string, mode p9.OpenFlags, perm p9.FileMode, uid
 		flags |= mode.OSFlags()
 	}
 
-	fd, err := syscall.Openat(l.controlFD(), name, flags, uint32(perm.Permissions()))
+	fd, err := syscall.Openat(l.fd(), name, flags, uint32(perm.Permissions()))
 	if err != nil {
 		return nil, nil, p9.QID{}, 0, extractErrno(err)
 	}
 	cu := specutils.MakeCleanup(func() {
 		syscall.Close(fd)
 		// Best effort attempt to remove the file in case of failure.
-		if err := syscall.Unlinkat(l.controlFD(), name); err != nil {
+		if err := syscall.Unlinkat(l.fd(), name); err != nil {
 			log.Warningf("error unlinking file %q after failure: %v", path.Join(l.hostPath, name), err)
 		}
 	})
@@ -414,13 +422,12 @@ func (l *localFile) Create(name string, mode p9.OpenFlags, perm p9.FileMode, uid
 	c := &localFile{
 		attachPoint: l.attachPoint,
 		hostPath:    cPath,
-		controlFile: f,
-		openedFile:  f,
+		file:        f,
 		mode:        mode,
 	}
 
 	cu.Release()
-	return newFDMaybe(c.openedFile), c, l.attachPoint.makeQID(stat), 0, nil
+	return newFDMaybe(c.file), c, l.attachPoint.makeQID(stat), 0, nil
 }
 
 // Mkdir implements p9.File.
@@ -433,12 +440,12 @@ func (l *localFile) Mkdir(name string, perm p9.FileMode, uid p9.UID, gid p9.GID)
 		return p9.QID{}, syscall.EBADF
 	}
 
-	if err := syscall.Mkdirat(l.controlFD(), name, uint32(perm.Permissions())); err != nil {
+	if err := syscall.Mkdirat(l.fd(), name, uint32(perm.Permissions())); err != nil {
 		return p9.QID{}, extractErrno(err)
 	}
 	cu := specutils.MakeCleanup(func() {
 		// Best effort attempt to remove the dir in case of failure.
-		if err := unix.Unlinkat(l.controlFD(), name, unix.AT_REMOVEDIR); err != nil {
+		if err := unix.Unlinkat(l.fd(), name, unix.AT_REMOVEDIR); err != nil {
 			log.Warningf("error unlinking dir %q after failure: %v", path.Join(l.hostPath, name), err)
 		}
 	})
@@ -446,7 +453,7 @@ func (l *localFile) Mkdir(name string, perm p9.FileMode, uid p9.UID, gid p9.GID)
 
 	// Open directory to change ownership and stat it.
 	flags := syscall.O_DIRECTORY | syscall.O_RDONLY | openFlags
-	fd, err := syscall.Openat(l.controlFD(), name, flags, 0)
+	fd, err := syscall.Openat(l.fd(), name, flags, 0)
 	if err != nil {
 		return p9.QID{}, extractErrno(err)
 	}
@@ -468,20 +475,34 @@ func (l *localFile) Mkdir(name string, perm p9.FileMode, uid p9.UID, gid p9.GID)
 func (l *localFile) Walk(names []string) ([]p9.QID, p9.File, error) {
 	// Duplicate current file if 'names' is empty.
 	if len(names) == 0 {
-		newFd, err := syscall.Dup(l.controlFD())
-		if err != nil {
-			return nil, nil, extractErrno(err)
+		var newFile *os.File
+		if l.isOpen() {
+			// File mode may have changed when it was opened, so open a new one.
+			var err error
+			newFile, err = openAnyFile(l.hostPath, func(mode int) (*os.File, error) {
+				return os.OpenFile(l.hostPath, openFlags|mode, 0)
+			})
+			if err != nil {
+				return nil, nil, extractErrno(err)
+			}
+		} else {
+			newFd, err := syscall.Dup(l.fd())
+			if err != nil {
+				return nil, nil, extractErrno(err)
+			}
+			newFile = os.NewFile(uintptr(newFd), l.hostPath)
 		}
-		stat, err := stat(newFd)
+
+		stat, err := stat(int(newFile.Fd()))
 		if err != nil {
-			syscall.Close(newFd)
+			newFile.Close()
 			return nil, nil, extractErrno(err)
 		}
 
 		c := &localFile{
 			attachPoint: l.attachPoint,
 			hostPath:    l.hostPath,
-			controlFile: os.NewFile(uintptr(newFd), l.hostPath),
+			file:        newFile,
 			mode:        invalidMode,
 		}
 		return []p9.QID{l.attachPoint.makeQID(stat)}, c, nil
@@ -490,7 +511,7 @@ func (l *localFile) Walk(names []string) ([]p9.QID, p9.File, error) {
 	var qids []p9.QID
 	last := l
 	for _, name := range names {
-		f, path, err := openAnyFile(last, name)
+		f, path, err := openAnyFileFromParent(last, name)
 		if err != nil {
 			return nil, nil, extractErrno(err)
 		}
@@ -514,7 +535,7 @@ func (l *localFile) Walk(names []string) ([]p9.QID, p9.File, error) {
 // StatFS implements p9.File.
 func (l *localFile) StatFS() (p9.FSStat, error) {
 	var s syscall.Statfs_t
-	if err := syscall.Fstatfs(l.controlFD(), &s); err != nil {
+	if err := syscall.Fstatfs(l.fd(), &s); err != nil {
 		return p9.FSStat{}, extractErrno(err)
 	}
 
@@ -533,10 +554,10 @@ func (l *localFile) StatFS() (p9.FSStat, error) {
 
 // FSync implements p9.File.
 func (l *localFile) FSync() error {
-	if l.openedFile == nil {
+	if !l.isOpen() {
 		return syscall.EBADF
 	}
-	if err := l.openedFile.Sync(); err != nil {
+	if err := l.file.Sync(); err != nil {
 		return extractErrno(err)
 	}
 	return nil
@@ -544,7 +565,7 @@ func (l *localFile) FSync() error {
 
 // GetAttr implements p9.File.
 func (l *localFile) GetAttr(_ p9.AttrMask) (p9.QID, p9.AttrMask, p9.Attr, error) {
-	stat, err := stat(l.controlFD())
+	stat, err := stat(l.fd())
 	if err != nil {
 		return p9.QID{}, p9.AttrMask{}, p9.Attr{}, extractErrno(err)
 	}
@@ -612,14 +633,14 @@ func (l *localFile) SetAttr(valid p9.SetAttrMask, attr p9.SetAttr) error {
 	// Handle all the sanity checks up front so that the client gets a
 	// consistent result that is not attribute dependent.
 	if !valid.IsSubsetOf(allowed) {
-		log.Warningf("SetAttr() failed for %q, mask: %v", l.controlFile.Name(), valid)
+		log.Warningf("SetAttr() failed for %q, mask: %v", l.file.Name(), valid)
 		return syscall.EPERM
 	}
 
-	fd := l.controlFD()
-	if l.ft == regular {
-		// Regular files are opened in RO mode, thus it needs to be reopened here
-		// for write.
+	// Check if it's possible to use cached file, or if another one needs to be
+	// opened for write.
+	fd := l.fd()
+	if l.ft == regular && l.mode != p9.WriteOnly && l.mode != p9.ReadWrite {
 		f, err := os.OpenFile(l.hostPath, openFlags|os.O_WRONLY, 0)
 		if err != nil {
 			return extractErrno(err)
@@ -733,7 +754,7 @@ func (l *localFile) RenameAt(oldName string, directory p9.File, newName string)
 	}
 
 	newParent := directory.(*localFile)
-	if err := renameat(l.controlFD(), oldName, newParent.controlFD(), newName); err != nil {
+	if err := renameat(l.fd(), oldName, newParent.fd(), newName); err != nil {
 		return extractErrno(err)
 	}
 	return nil
@@ -744,11 +765,11 @@ func (l *localFile) ReadAt(p []byte, offset uint64) (int, error) {
 	if l.mode != p9.ReadOnly && l.mode != p9.ReadWrite {
 		return 0, syscall.EBADF
 	}
-	if l.openedFile == nil {
+	if !l.isOpen() {
 		return 0, syscall.EBADF
 	}
 
-	r, err := l.openedFile.ReadAt(p, int64(offset))
+	r, err := l.file.ReadAt(p, int64(offset))
 	switch err {
 	case nil, io.EOF:
 		return r, nil
@@ -762,11 +783,11 @@ func (l *localFile) WriteAt(p []byte, offset uint64) (int, error) {
 	if l.mode != p9.WriteOnly && l.mode != p9.ReadWrite {
 		return 0, syscall.EBADF
 	}
-	if l.openedFile == nil {
+	if !l.isOpen() {
 		return 0, syscall.EBADF
 	}
 
-	w, err := l.openedFile.WriteAt(p, int64(offset))
+	w, err := l.file.WriteAt(p, int64(offset))
 	if err != nil {
 		return w, extractErrno(err)
 	}
@@ -783,19 +804,19 @@ func (l *localFile) Symlink(target, newName string, uid p9.UID, gid p9.GID) (p9.
 		return p9.QID{}, syscall.EBADF
 	}
 
-	if err := unix.Symlinkat(target, l.controlFD(), newName); err != nil {
+	if err := unix.Symlinkat(target, l.fd(), newName); err != nil {
 		return p9.QID{}, extractErrno(err)
 	}
 	cu := specutils.MakeCleanup(func() {
 		// Best effort attempt to remove the symlink in case of failure.
-		if err := syscall.Unlinkat(l.controlFD(), newName); err != nil {
+		if err := syscall.Unlinkat(l.fd(), newName); err != nil {
 			log.Warningf("error unlinking file %q after failure: %v", path.Join(l.hostPath, newName), err)
 		}
 	})
 	defer cu.Clean()
 
 	// Open symlink to change ownership and stat it.
-	fd, err := syscall.Openat(l.controlFD(), newName, unix.O_PATH|openFlags, 0)
+	fd, err := syscall.Openat(l.fd(), newName, unix.O_PATH|openFlags, 0)
 	if err != nil {
 		return p9.QID{}, extractErrno(err)
 	}
@@ -824,7 +845,7 @@ func (l *localFile) Link(target p9.File, newName string) error {
 	}
 
 	targetFile := target.(*localFile)
-	if err := unix.Linkat(targetFile.controlFD(), "", l.controlFD(), newName, linux.AT_EMPTY_PATH); err != nil {
+	if err := unix.Linkat(targetFile.fd(), "", l.fd(), newName, linux.AT_EMPTY_PATH); err != nil {
 		return extractErrno(err)
 	}
 	return nil
@@ -847,7 +868,7 @@ func (l *localFile) UnlinkAt(name string, flags uint32) error {
 		return syscall.EBADF
 	}
 
-	if err := unix.Unlinkat(l.controlFD(), name, int(flags)); err != nil {
+	if err := unix.Unlinkat(l.fd(), name, int(flags)); err != nil {
 		return extractErrno(err)
 	}
 	return nil
@@ -858,7 +879,7 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) {
 	if l.mode != p9.ReadOnly && l.mode != p9.ReadWrite {
 		return nil, syscall.EBADF
 	}
-	if l.openedFile == nil {
+	if !l.isOpen() {
 		return nil, syscall.EBADF
 	}
 
@@ -866,11 +887,11 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) {
 	// reading all directory contents. Take a lock because this operation is
 	// stateful.
 	l.readDirMu.Lock()
-	if _, err := l.openedFile.Seek(0, 0); err != nil {
+	if _, err := l.file.Seek(0, 0); err != nil {
 		l.readDirMu.Unlock()
 		return nil, extractErrno(err)
 	}
-	names, err := l.openedFile.Readdirnames(-1)
+	names, err := l.file.Readdirnames(-1)
 	if err != nil {
 		l.readDirMu.Unlock()
 		return nil, extractErrno(err)
@@ -879,7 +900,7 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) {
 
 	var dirents []p9.Dirent
 	for i := int(offset); i >= 0 && i < len(names); i++ {
-		stat, err := statAt(l.openedFD(), names[i])
+		stat, err := statAt(l.fd(), names[i])
 		if err != nil {
 			continue
 		}
@@ -897,9 +918,10 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) {
 // Readlink implements p9.File.
 func (l *localFile) Readlink() (string, error) {
 	// Shamelessly stolen from os.Readlink (added upper bound limit to buffer).
-	for len := 128; len < 1024*1024; len *= 2 {
+	const limit = 1024 * 1024
+	for len := 128; len < limit; len *= 2 {
 		b := make([]byte, len)
-		n, err := unix.Readlinkat(l.controlFD(), "", b)
+		n, err := unix.Readlinkat(l.fd(), "", b)
 		if err != nil {
 			return "", extractErrno(err)
 		}
@@ -922,20 +944,16 @@ func (l *localFile) Connect(p9.ConnectFlags) (*fd.FD, error) {
 
 // Close implements p9.File.
 func (l *localFile) Close() error {
-	err := l.controlFile.Close()
-
-	// Close only once in case opened and control files point to
-	// the same os.File struct.
-	if l.openedFile != nil && l.openedFile != l.controlFile {
-		err = l.openedFile.Close()
-	}
-
-	l.openedFile = nil
-	l.controlFile = nil
 	l.mode = invalidMode
+	err := l.file.Close()
+	l.file = nil
 	return err
 }
 
+func (l *localFile) isOpen() bool {
+	return l.mode != invalidMode
+}
+
 // Renamed implements p9.Renamed.
 func (l *localFile) Renamed(newDir p9.File, newName string) {
 	l.hostPath = path.Join(newDir.(*localFile).hostPath, newName)
diff --git a/runsc/fsgofer/fsgofer_test.go b/runsc/fsgofer/fsgofer_test.go
index 47b5380dc..e74df7ede 100644
--- a/runsc/fsgofer/fsgofer_test.go
+++ b/runsc/fsgofer/fsgofer_test.go
@@ -42,6 +42,48 @@ func assertPanic(t *testing.T, f func()) {
 	f()
 }
 
+func testReadWrite(f p9.File, flags p9.OpenFlags, content []byte) error {
+	want := make([]byte, len(content))
+	copy(want, content)
+
+	b := []byte("test-1-2-3")
+	w, err := f.WriteAt(b, uint64(len(content)))
+	if flags == p9.WriteOnly || flags == p9.ReadWrite {
+		if err != nil {
+			return fmt.Errorf("WriteAt(): %v", err)
+		}
+		if w != len(b) {
+			return fmt.Errorf("WriteAt() was partial, got: %d, want: %d", w, len(b))
+		}
+		want = append(want, b...)
+	} else {
+		if e, ok := err.(syscall.Errno); !ok || e != syscall.EBADF {
+			return fmt.Errorf("WriteAt() should have failed, got: %d, want: EBADFD", err)
+		}
+	}
+
+	rBuf := make([]byte, len(want))
+	r, err := f.ReadAt(rBuf, 0)
+	if flags == p9.ReadOnly || flags == p9.ReadWrite {
+		if err != nil {
+			return fmt.Errorf("ReadAt(): %v", err)
+		}
+		if r != len(rBuf) {
+			return fmt.Errorf("ReadAt() was partial, got: %d, want: %d", r, len(rBuf))
+		}
+		if string(rBuf) != string(want) {
+			return fmt.Errorf("ReadAt() wrong data, got: %s, want: %s", string(rBuf), want)
+		}
+	} else {
+		if e, ok := err.(syscall.Errno); !ok || e != syscall.EBADF {
+			return fmt.Errorf("ReadAt() should have failed, got: %d, want: EBADFD", err)
+		}
+	}
+	return nil
+}
+
+var allOpenFlags = []p9.OpenFlags{p9.ReadOnly, p9.WriteOnly, p9.ReadWrite}
+
 var (
 	allTypes = []fileType{regular, directory, symlink}
 
@@ -160,61 +202,24 @@ func TestReadWrite(t *testing.T) {
 			t.Fatalf("%v: createFile() failed, err: %v", s, err)
 		}
 		defer child.Close()
-		b := []byte("foobar")
-		w, err := child.WriteAt(b, 0)
+		want := []byte("foobar")
+		w, err := child.WriteAt(want, 0)
 		if err != nil {
 			t.Fatalf("%v: Write() failed, err: %v", s, err)
 		}
-		if w != len(b) {
-			t.Fatalf("%v: Write() was partial, got: %d, expected: %d", s, w, len(b))
-		}
-		for _, test := range []struct {
-			flags p9.OpenFlags
-			read  bool
-			write bool
-		}{
-			{flags: p9.ReadOnly, read: true, write: false},
-			{flags: p9.WriteOnly, read: false, write: true},
-			{flags: p9.ReadWrite, read: true, write: true},
-		} {
+		if w != len(want) {
+			t.Fatalf("%v: Write() was partial, got: %d, expected: %d", s, w, len(want))
+		}
+		for _, flags := range allOpenFlags {
 			_, l, err := s.file.Walk([]string{"test"})
 			if err != nil {
 				t.Fatalf("%v: Walk(%s) failed, err: %v", s, "test", err)
 			}
-			if _, _, _, err := l.Open(test.flags); err != nil {
-				t.Fatalf("%v: Open(%v) failed, err: %v", s, test.flags, err)
-			}
-
-			w, err = l.WriteAt(b, 0)
-			if test.write {
-				if err != nil {
-					t.Fatalf("%v, %v: WriteAt() failed, err: %v", s, test.flags, err)
-				}
-				if w != len(b) {
-					t.Fatalf("%v, %v: WriteAt() was partial, got: %d, expected: %d", s, test.flags, w, len(b))
-				}
-			} else {
-				if err == nil {
-					t.Fatalf("%v, %v: WriteAt() should have failed", s, test.flags)
-				}
+			if _, _, _, err := l.Open(flags); err != nil {
+				t.Fatalf("%v: Open(%v) failed, err: %v", s, flags, err)
 			}
-
-			rBuf := make([]byte, len(b))
-			r, err := l.ReadAt(rBuf, 0)
-			if test.read {
-				if err != nil {
-					t.Fatalf("%v, %v: ReadAt() failed, err: %v", s, test.flags, err)
-				}
-				if r != len(rBuf) {
-					t.Fatalf("%v, %v: ReadAt() was partial, got: %d, expected: %d", s, test.flags, r, len(rBuf))
-				}
-				if string(rBuf) != "foobar" {
-					t.Fatalf("%v, %v: ReadAt() wrong data, got: %s, expected: %s", s, test.flags, string(rBuf), "foobar")
-				}
-			} else {
-				if err == nil {
-					t.Fatalf("%v, %v: ReadAt() should have failed", s, test.flags)
-				}
+			if err := testReadWrite(l, flags, want); err != nil {
+				t.Fatalf("%v: testReadWrite(%v) failed: %v", s, flags, err)
 			}
 		}
 	})
@@ -222,42 +227,57 @@ func TestReadWrite(t *testing.T) {
 
 func TestCreate(t *testing.T) {
 	runCustom(t, []fileType{directory}, rwConfs, func(t *testing.T, s state) {
-		for i, test := range []struct {
-			flags p9.OpenFlags
-			read  bool
-		}{
-			{flags: p9.WriteOnly, read: false},
-			{flags: p9.ReadWrite, read: true},
-		} {
-			_, l, _, _, err := s.file.Create(fmt.Sprintf("test-%d", i), test.flags, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid()))
+		for i, flags := range allOpenFlags {
+			_, l, _, _, err := s.file.Create(fmt.Sprintf("test-%d", i), flags, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid()))
 			if err != nil {
-				t.Fatalf("%v, %v: WriteAt() failed, err: %v", s, test.flags, err)
+				t.Fatalf("%v, %v: WriteAt() failed, err: %v", s, flags, err)
 			}
 
-			b := []byte("foobar")
-			w, err := l.WriteAt(b, 0)
+			if err := testReadWrite(l, flags, []byte{}); err != nil {
+				t.Fatalf("%v: testReadWrite(%v) failed: %v", s, flags, err)
+			}
+		}
+	})
+}
+
+// TestReadWriteDup tests that a file opened in any mode can be dup'ed and
+// reopened in any other mode.
+func TestReadWriteDup(t *testing.T) {
+	runCustom(t, []fileType{directory}, rwConfs, func(t *testing.T, s state) {
+		child, err := createFile(s.file, "test")
+		if err != nil {
+			t.Fatalf("%v: createFile() failed, err: %v", s, err)
+		}
+		defer child.Close()
+		want := []byte("foobar")
+		w, err := child.WriteAt(want, 0)
+		if err != nil {
+			t.Fatalf("%v: Write() failed, err: %v", s, err)
+		}
+		if w != len(want) {
+			t.Fatalf("%v: Write() was partial, got: %d, expected: %d", s, w, len(want))
+		}
+		for _, flags := range allOpenFlags {
+			_, l, err := s.file.Walk([]string{"test"})
 			if err != nil {
-				t.Fatalf("%v, %v: WriteAt() failed, err: %v", s, test.flags, err)
+				t.Fatalf("%v: Walk(%s) failed, err: %v", s, "test", err)
 			}
-			if w != len(b) {
-				t.Fatalf("%v, %v: WriteAt() was partial, got: %d, expected: %d", s, test.flags, w, len(b))
+			defer l.Close()
+			if _, _, _, err := l.Open(flags); err != nil {
+				t.Fatalf("%v: Open(%v) failed, err: %v", s, flags, err)
 			}
-
-			rBuf := make([]byte, len(b))
-			r, err := l.ReadAt(rBuf, 0)
-			if test.read {
+			for _, dupFlags := range allOpenFlags {
+				t.Logf("Original flags: %v, dup flags: %v", flags, dupFlags)
+				_, dup, err := l.Walk([]string{})
 				if err != nil {
-					t.Fatalf("%v, %v: ReadAt() failed, err: %v", s, test.flags, err)
-				}
-				if r != len(rBuf) {
-					t.Fatalf("%v, %v: ReadAt() was partial, got: %d, expected: %d", s, test.flags, r, len(rBuf))
+					t.Fatalf("%v: Walk(<empty>) failed: %v", s, err)
 				}
-				if string(rBuf) != "foobar" {
-					t.Fatalf("%v, %v: ReadAt() wrong data, got: %s, expected: %s", s, test.flags, string(rBuf), "foobar")
+				defer dup.Close()
+				if _, _, _, err := dup.Open(dupFlags); err != nil {
+					t.Fatalf("%v: Open(%v) failed: %v", s, flags, err)
 				}
-			} else {
-				if err == nil {
-					t.Fatalf("%v, %v: ReadAt() should have failed", s, test.flags)
+				if err := testReadWrite(dup, dupFlags, want); err != nil {
+					t.Fatalf("%v: testReadWrite(%v) failed: %v", s, dupFlags, err)
 				}
 			}
 		}