From e006d7c13fed023fd15b282ea21fd7d132f6c7d8 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Thu, 16 Jul 2026 17:41:19 +0100 Subject: [PATCH] vfs: fix crash when multiple mounts or servers share the same VFS The VFS is shared between users with the same remote and options, for example two mounts created over the rc, or a mount and an NFS server. Each node has a single Sys() slot which mount, mount2 and serve nfs all used to attach their per-node data. With a shared VFS the users overwrote each other's data: at best churning the cached FUSE nodes, and since the slot was an atomic.Value, panicking with "store of inconsistently typed value" as soon as two users stored different types on the same node. This change gives each node auxiliary values keyed by owner, set with SetAux and read with Aux, so each user of the VFS has an independent slot. The mounts now cache their FUSE nodes under their own key, leaving Sys - which is read through the os.FileInfo interface - reserved for users like serve nfs which need to control what that returns. Nodes with nothing attached use less memory than before (one pointer instead of an atomic.Value) and reads remain lock free. Bug discovered while thinking about #9617 --- cmd/mount/dir.go | 8 ++--- cmd/mount2/node.go | 4 +-- vfs/aux.go | 85 ++++++++++++++++++++++++++++++++++++++++++++++ vfs/aux_test.go | 73 +++++++++++++++++++++++++++++++++++++++ vfs/dir.go | 12 +------ vfs/file.go | 12 +------ vfs/vfs.go | 2 ++ 7 files changed, 168 insertions(+), 28 deletions(-) create mode 100644 vfs/aux.go create mode 100644 vfs/aux_test.go diff --git a/cmd/mount/dir.go b/cmd/mount/dir.go index ad657bc19..272ce1329 100644 --- a/cmd/mount/dir.go +++ b/cmd/mount/dir.go @@ -81,7 +81,7 @@ func (d *Dir) Lookup(ctx context.Context, req *fuse.LookupRequest, resp *fuse.Lo resp.EntryValid = time.Duration(d.fsys.opt.AttrTimeout) // Check the mnode to see if it has a fuse Node cached // We must return the same fuse nodes for vfs Nodes - node, ok := mnode.Sys().(fusefs.Node) + node, ok := mnode.Aux(d.fsys).(fusefs.Node) if ok { return node, nil } @@ -94,7 +94,7 @@ func (d *Dir) Lookup(ctx context.Context, req *fuse.LookupRequest, resp *fuse.Lo panic("bad type") } // Cache the node for later - mnode.SetSys(node) + mnode.SetAux(d.fsys, node) return node, nil } @@ -158,7 +158,7 @@ func (d *Dir) Create(ctx context.Context, req *fuse.CreateRequest, resp *fuse.Cr return nil, nil, translateError(err) } node = &File{file, d.fsys} - file.SetSys(node) // cache the FUSE node for later + file.SetAux(d.fsys, node) // cache the FUSE node for later return node, &FileHandle{fh}, err } @@ -172,7 +172,7 @@ func (d *Dir) Mkdir(ctx context.Context, req *fuse.MkdirRequest) (node fusefs.No return nil, translateError(err) } node = &Dir{dir, d.fsys} - dir.SetSys(node) // cache the FUSE node for later + dir.SetAux(d.fsys, node) // cache the FUSE node for later return node, nil } diff --git a/cmd/mount2/node.go b/cmd/mount2/node.go index 705a14f38..3b60c651f 100644 --- a/cmd/mount2/node.go +++ b/cmd/mount2/node.go @@ -30,7 +30,7 @@ var _ fusefs.InodeEmbedder = (*Node)(nil) func newNode(fsys *FS, vfsNode vfs.Node) (node *Node) { // Check the vfsNode to see if it has a fuse Node cached // We must return the same fuse nodes for vfs Nodes - node, ok := vfsNode.Sys().(*Node) + node, ok := vfsNode.Aux(fsys).(*Node) if ok { return node } @@ -39,7 +39,7 @@ func newNode(fsys *FS, vfsNode vfs.Node) (node *Node) { fsys: fsys, } // Cache the node for later - vfsNode.SetSys(node) + vfsNode.SetAux(fsys, node) return node } diff --git a/vfs/aux.go b/vfs/aux.go new file mode 100644 index 000000000..c6a404266 --- /dev/null +++ b/vfs/aux.go @@ -0,0 +1,85 @@ +package vfs + +import "sync/atomic" + +// auxEntry associates an owner with a value attached to a node. +type auxEntry struct { + owner, value any +} + +// aux holds auxiliary values attached to a node, keyed by owner. +// +// It is embedded in Dir and File to provide the Aux, SetAux, Sys and +// SetSys methods of the Node interface. +// +// Reads are lock free. Writes copy the entry list, which is assumed +// to be very short. +type aux struct { + entries atomic.Pointer[[]auxEntry] +} + +// sysOwner is the owner Sys and SetSys attach their value under. +type sysOwner struct{} + +// Aux returns the value attached to the node for owner, or nil if +// none is attached. +func (a *aux) Aux(owner any) any { + if entries := a.entries.Load(); entries != nil { + for _, entry := range *entries { + if entry.owner == owner { + return entry.value + } + } + } + return nil +} + +// SetAux attaches value to the node for owner, replacing any value +// owner attached before. Attaching nil removes owner's value. +// +// Values attached by different owners are independent, so multiple +// users of a shared VFS do not conflict. Owner must be comparable and +// unique to the user - a pointer to the user's filesystem struct is a +// good choice. +func (a *aux) SetAux(owner, value any) { + for { + old := a.entries.Load() + var entries []auxEntry + if old != nil { + entries = make([]auxEntry, 0, len(*old)+1) + for _, entry := range *old { + if entry.owner != owner { + entries = append(entries, entry) + } + } + } + if value != nil { + entries = append(entries, auxEntry{owner: owner, value: value}) + } + var next *[]auxEntry + if len(entries) != 0 { + next = &entries + } + if a.entries.CompareAndSwap(old, next) { + return + } + } +} + +// Sys returns the value set with SetSys (can be nil) - satisfies +// os.FileInfo. +// +// This is what callers reading the node through os.FileInfo (e.g. the +// NFS server library) will see. It is reserved for users which need +// to control that; users caching their own data on the node should +// use Aux and SetAux instead. As the VFS may be shared, the value +// stored here should be derived from the node alone so that all users +// store the same value. +func (a *aux) Sys() any { + return a.Aux(sysOwner{}) +} + +// SetSys sets the value returned by Sys - see Sys for the contract. +func (a *aux) SetSys(x any) { + a.SetAux(sysOwner{}, x) +} diff --git a/vfs/aux_test.go b/vfs/aux_test.go new file mode 100644 index 000000000..d0c74c92f --- /dev/null +++ b/vfs/aux_test.go @@ -0,0 +1,73 @@ +package vfs + +import ( + "fmt" + "sync" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAux(t *testing.T) { + var a aux + owner1, owner2 := new(int), new(int) + + // Nothing attached yet + assert.Nil(t, a.Aux(owner1)) + assert.Nil(t, a.Sys()) + + // Values attached by different owners are independent even if + // they have different types + a.SetAux(owner1, "potato") + a.SetAux(owner2, 2) + assert.Equal(t, "potato", a.Aux(owner1)) + assert.Equal(t, 2, a.Aux(owner2)) + + // Replace a value + a.SetAux(owner1, "sausage") + assert.Equal(t, "sausage", a.Aux(owner1)) + + // Remove a value + a.SetAux(owner1, nil) + assert.Nil(t, a.Aux(owner1)) + assert.Equal(t, 2, a.Aux(owner2)) + + // Sys is independent of the other owners + assert.Nil(t, a.Sys()) + a.SetSys(42) + assert.Equal(t, 42, a.Sys()) + assert.Equal(t, 2, a.Aux(owner2)) + + // Changing the type of the value stored must not panic + a.SetSys("42") + assert.Equal(t, "42", a.Sys()) + + // Remove the remaining values + a.SetSys(nil) + a.SetAux(owner2, nil) + assert.Nil(t, a.entries.Load()) +} + +func TestAuxConcurrent(t *testing.T) { + const ( + owners = 4 + iterations = 100 + ) + var ( + a aux + wg sync.WaitGroup + ) + for i := range owners { + wg.Add(1) + go func() { + defer wg.Done() + owner := &i + for j := range iterations { + value := fmt.Sprintf("%d-%d", i, j) + a.SetAux(owner, value) + assert.Equal(t, value, a.Aux(owner)) + } + }() + } + wg.Wait() +} diff --git a/vfs/dir.go b/vfs/dir.go index 05fefd930..0d24195bd 100644 --- a/vfs/dir.go +++ b/vfs/dir.go @@ -24,6 +24,7 @@ import ( // Dir represents a directory entry type Dir struct { + aux // values attached by users of the VFS vfs *VFS // read only inode uint64 // read only: inode number f fs.Fs // read only @@ -36,7 +37,6 @@ type Dir struct { read time.Time // time directory entry last read items map[string]Node // directory entries - can be empty but not nil virtual map[string]vState // virtual directory entries - may be nil - sys atomic.Value // user defined info to be attached here modTimeMu sync.Mutex // protects the following modTime time.Time @@ -181,16 +181,6 @@ func (d *Dir) Path() (name string) { return d.path } -// Sys returns underlying data source (can be nil) - satisfies Node interface -func (d *Dir) Sys() any { - return d.sys.Load() -} - -// SetSys sets the underlying data source (can be nil) - satisfies Node interface -func (d *Dir) SetSys(x any) { - d.sys.Store(x) -} - // Inode returns the inode number - satisfies Node interface func (d *Dir) Inode() uint64 { return d.inode diff --git a/vfs/file.go b/vfs/file.go index 45909ab11..69a7afcd1 100644 --- a/vfs/file.go +++ b/vfs/file.go @@ -40,6 +40,7 @@ import ( // File represents a file or a symlink type File struct { + aux // values attached by users of the VFS inode uint64 // inode number - read only size atomic.Int64 // size of file ctx context.Context // context for VFS operations - read only @@ -55,7 +56,6 @@ type File struct { virtualModTime *time.Time // modtime for backends with Precision == fs.ModTimeNotSupported pendingModTime time.Time // will be applied once o becomes available, i.e. after file was written pendingRenameFun func(ctx context.Context) error // will be run/renamed after all writers close - sys atomic.Value // user defined info to be attached here nwriters atomic.Int32 // len(writers) appendMode bool // file was opened with O_APPEND isLink bool // file represents a symlink @@ -184,16 +184,6 @@ func (f *File) CachePath() string { return f._cachePath() } -// Sys returns underlying data source (can be nil) - satisfies Node interface -func (f *File) Sys() any { - return f.sys.Load() -} - -// SetSys sets the underlying data source (can be nil) - satisfies Node interface -func (f *File) SetSys(x any) { - f.sys.Store(x) -} - // Inode returns the inode number - satisfies Node interface func (f *File) Inode() uint64 { return f.inode diff --git a/vfs/vfs.go b/vfs/vfs.go index 9006b798c..aaf94d9bf 100644 --- a/vfs/vfs.go +++ b/vfs/vfs.go @@ -69,6 +69,8 @@ type Node interface { Truncate(size int64) error Path() string SetSys(any) + Aux(owner any) any + SetAux(owner, value any) } // Check interfaces