fix onRename: reject a cross-file-system rename on fsid by AliRamberg · Pull Request #12 · treeverse/go-nfs

AliRamberg · 2026-07-26T14:24:56Z

RFC 1813 3.3.14 defines "same file system" as the two directories' fsid fields being equal and requires NFS3ERR_XDEV when they differ. onRename compared the filesystems FromHandle returned instead — a different question, and one that rejected every rename through helpers.RecoverPanics, which hands out a fresh wrapper per call.

ToFileAttribute already reads the platform stat data and dropped st_dev alone, so this carries it through as file.FileInfo.FSID. Zero reports nothing rather than a file system, so it falls back to the handler's root handle, which compares a filesystem equal to itself through any wrapper. The destination is now stat'd through the filesystem to.Handle named, not the source's.

Unrelated fix, own commit: recoveryFilesystem delegates explicitly, so it never satisfied billy.Capable.
CapabilityCheck fell back to DefaultCapabilities, reporting a read-only filesystem as writable.

arielshaqed

Not sure: why would these ever be different filesystems?

arielshaqed · 2026-08-02T14:50:04Z

+// non-nil func fields are never DeepEqual.
+func unwrapFS(fs billy.Filesystem) billy.Filesystem {
+	for {
+		type unwrapper interface{ Unwrap() billy.Filesystem }


Raise to a higher level - this is now an external interface, you expect Filesystems to implement it, and go-nfs clients need to implement it.

arielshaqed

This seems weird: we suddenly need to add a RO wrapper, some things need to be held by pointer, etc. Can you instead explore getting rid of reflect.DeepEqual?

It is not useful. Suppose I write a memory billy.Filesystem that consists of a map from paths to contents. I mount two different such filesystems using go-nfs in the same process on /mnt/1/ and on /mnt/2. After
```
echo a > /mnt/1/x
echo b > /mnt/2/x
mv -f /mnt/1/x /mnt/2/x
```
what happens? The filesystems are reflect.DeepEqual (it does deep equality on maps). So go-nfs thinks it's not cross-filesystem. How would you even write such a Rename method on the first filesystem? How does it even know it needs to copy to the other (currently equal) filesystem?
It is not correct. Suppose I mount the same NFS filesystem on /mnt/1/ and on /mnt/2/. I would expect the kernel to report cross-filesystem renames.

I think we should understand why go-nfs even has this check.

arielshaqed · 2026-08-03T06:07:17Z

+// same filesystem with reflect.DeepEqual, and non-nil func values are never
+// deeply equal, so an inline func field would make every wrapped filesystem


I had to look up the godoc to understand "non-nil func values are never deeply equal". Learn something new every day, I guess.

The deep issue is probably in the spec here:

Slice, map, and function types are not comparable. However, as a special case, a slice, map, or function value may be compared to the predeclared identifier nil

arielshaqed · 2026-08-03T06:11:24Z

 // If h also implements nfs.DirIteratorHandler, the returned handler will too.
 func RecoverPanics(h nfs.Handler, onPanic OnPanic) nfs.Handler {
-	base := &recoveryHandler{handler: h, onPanic: onPanic}
+	base := &recoveryHandler{handler: h, onPanic: &panicRef{fn: onPanic}}


Not sure what this does: different calls to RecoverPanics will create different pointers. So this only works if RecoverPanics is called once. This all makes sense and I am happy with it; the only issue is that the documentation of panicRef is really confusing: it stresses using a pointer to the same panicRef each time when in fact the point is to make it comparable.

arielshaqed · 2026-08-03T06:11:52Z

 	}
 }
+
+// readOnlyFS reports only ReadCapability, unlike billy.DefaultCapabilities.


Why do we now need this?

arielshaqed · 2026-08-03T06:13:14Z

+		// A write to a read-only target is not an I/O failure, and reporting it
+		// as one leaves the client with a misleading EIO.
+		if errors.Is(err, syscall.EROFS) {
+			return &NFSStatusError{NFSStatusROFS, err}
+		}


Is this only within onRename, and not in other funcs?

recoveryFilesystem delegates explicitly rather than embedding, so it did not satisfy billy.Capable and billy.CapabilityCheck fell back to billy.DefaultCapabilities. That reports a read-only filesystem as writable, and the write operations gate on it.

arielshaqed

Thanks! This is a good way to go, I just wish we could stay backwards-compatible with go-nfs.

arielshaqed · 2026-08-04T13:52:12Z

-	// check the two fs are the same
-	if !reflect.DeepEqual(fs, fs2) {
-		return &NFSStatusError{NFSStatusNotSupp, os.ErrPermission}
-	}
-


This breaks backwards compatibility. I think we should not break, at least not yet.

AliRamberg

Pushed the fsid-with-fallback version. Replies inline on the three open threads.

AliRamberg · 2026-08-04T14:43:57Z

+	if fromDirAttr.FSID != toDirAttr.FSID {
+		return &NFSStatusError{NFSStatusXDev, os.ErrInvalid}
+	}
+	if !bytes.Equal(userHandle.ToHandle(fs, nil), userHandle.ToHandle(fs2, nil)) {


Fallback as you asked: fsid decides only when it differs. Zero on both sides decides nothing, so a Filesystem that reports no fsid keeps returning NFSStatusNotSupp exactly as it does today.

It asks the handler for the root handle rather than comparing the filesystems directly, because recoveryHandler.ToHandle already unwraps *recoveryFilesystem -- so this needs no new type and no change to the wrappers. Verified: swapping it for a plain fs != fs2 makes a same-directory rename through RecoverPanics fail with NOTSUPP.

Not sure why "RecoverPanics" is relevant here, but I don't think it affects this reply either.

This still changes behaviour of existing go-nfs - e.g. consider a filesystem type that returns the same handle for root. I don't know, maybe always "Find your dream.". We can probably retain the previous behaviour with something like

Suggested change

if !bytes.Equal(userHandle.ToHandle(fs, nil), userHandle.ToHandle(fs2, nil)) {

if fromDirAttr.FSID == 0 && !reflect.DeepEqual(fs, fs2) {

The fact that it passed for a specially-crafted test does not mean that it will always pass.

AliRamberg · 2026-08-04T14:43:57Z

+	// operating-system backing reports one by returning a FileInfo from
+	// os.FileInfo.Sys(); leaving it zero makes every file appear to share a
+	// single file system.
+	FSID uint64


Reworded rather than switched to *uint64. With the fallback, zero no longer means "same file system, allow" -- it means fsid decided nothing and the handle comparison runs instead, so the backwards-compatibility problem you flagged goes away without a pointer. Everest returns either nil or a FileInfo with no FSID from Sys(), so it is unaffected either way.

Still happy to switch to *uint64, or move it onto the Filesystem, if you would rather have an explicit absent.

Thanks, this makes a lot of sense. Indeed the Linux exports manpage gives fsid 0 a very special meaning. Requiring the mounted filesystem to report something different makes a lot of sense.

arielshaqed

Thanks, this is great! I would still like to keep the exact earlier behaviour when fsid == 0. See inline - I think it's easy to do. To be clear, if it is hard then let's not.

arielshaqed · 2026-08-05T06:46:13Z

+	// operating-system backing reports one by returning a FileInfo from
+	// os.FileInfo.Sys(); leaving it zero makes every file appear to share a
+	// single file system.
+	FSID uint64


Thanks, this makes a lot of sense. Indeed the Linux exports manpage gives fsid 0 a very special meaning. Requiring the mounted filesystem to report something different makes a lot of sense.

arielshaqed · 2026-08-05T06:52:46Z

+	if fromDirAttr.FSID != toDirAttr.FSID {
+		return &NFSStatusError{NFSStatusXDev, os.ErrInvalid}
+	}
+	if !bytes.Equal(userHandle.ToHandle(fs, nil), userHandle.ToHandle(fs2, nil)) {


Not sure why "RecoverPanics" is relevant here, but I don't think it affects this reply either.

This still changes behaviour of existing go-nfs - e.g. consider a filesystem type that returns the same handle for root. I don't know, maybe always "Find your dream.". We can probably retain the previous behaviour with something like

Suggested change

if !bytes.Equal(userHandle.ToHandle(fs, nil), userHandle.ToHandle(fs2, nil)) {

if fromDirAttr.FSID == 0 && !reflect.DeepEqual(fs, fs2) {

The fact that it passed for a specially-crafted test does not mean that it will always pass.

RFC 1813 3.3.14 defines "to.dir and from.dir must reside on the same file system on the server" as the fsid fields in the two directories' attributes being equal, and requires NFS3ERR_XDEV when they differ. onRename only compared the filesystems that FromHandle returned, which answers a different question and rejected every rename made through helpers.RecoverPanics: it returns a fresh wrapper per call, and reflect.DeepEqual reports two non-nil func values as unequal. Compare fsid first. ToFileAttribute already reads the rest of the platform stat data and dropped st_dev alone, so carry it through as file.FileInfo.FSID; a Filesystem with no operating-system backing reports one by returning a file.FileInfo from os.FileInfo.Sys(). Zero reports nothing rather than a file system, so fall back to asking the handler that minted the handles, which compares the same filesystem equal to itself through any wrapper. Stat the destination through the filesystem that to.Handle named rather than through the source's. Reading both attributes through one filesystem compares it against itself.

AliRamberg · 2026-08-05T10:39:33Z

+	if fromDirAttr.FSID != toDirAttr.FSID {
+		return &NFSStatusError{NFSStatusXDev, os.ErrInvalid}
+	}
+	if fromDirAttr.FSID == 0 && !reflect.DeepEqual(fs, fs2) {


Applied -- and you're right the root handle isn't an identity: Everest's ToHandle ignores its fs and always returns that constant.

RecoverPanics matters only as the consequence: a filesystem reporting no fsid lands in DeepEqual, which is false for its per-call wrapper, so every rename through it stays refused as on master -- the fix reaches only filesystems that report an fsid, which is why the test now does.

AliRamberg requested review from ItamarYuran and arielshaqed August 2, 2026 13:38

AliRamberg force-pushed the fix/rename-deepequal branch 3 times, most recently from 09368e2 to 151cce0 Compare August 2, 2026 13:44

arielshaqed requested changes Aug 2, 2026

View reviewed changes

AliRamberg force-pushed the fix/rename-deepequal branch from 151cce0 to 29e1829 Compare August 2, 2026 16:11

arielshaqed reviewed Aug 3, 2026

View reviewed changes

AliRamberg force-pushed the fix/rename-deepequal branch from 29e1829 to e70f51e Compare August 3, 2026 07:52

AliRamberg mentioned this pull request Aug 3, 2026

fix: report the status describing a filesystem error, not always EIO #13

Merged

AliRamberg force-pushed the fix/rename-deepequal branch 2 times, most recently from 5be31d8 to 49f0d65 Compare August 3, 2026 13:33

AliRamberg force-pushed the fix/rename-deepequal branch from 49f0d65 to 2932c4f Compare August 4, 2026 09:39

AliRamberg changed the title ~~fix onRename: unwrap recoveryFilesystem before reflect.DeepEqual~~ fix onRename: reject a cross-file-system rename on fsid Aug 4, 2026

AliRamberg force-pushed the fix/rename-deepequal branch 2 times, most recently from 835ca53 to 7181caf Compare August 4, 2026 12:02

AliRamberg requested a review from arielshaqed August 4, 2026 12:06

arielshaqed requested changes Aug 4, 2026

View reviewed changes

AliRamberg force-pushed the fix/rename-deepequal branch from 7181caf to 466afcf Compare August 4, 2026 14:19

AliRamberg commented Aug 4, 2026

View reviewed changes

AliRamberg force-pushed the fix/rename-deepequal branch from 466afcf to b1ebc5d Compare August 4, 2026 14:28

AliRamberg commented Aug 4, 2026

View reviewed changes

AliRamberg requested a review from arielshaqed August 4, 2026 14:52

arielshaqed approved these changes Aug 5, 2026

View reviewed changes

AliRamberg force-pushed the fix/rename-deepequal branch from b1ebc5d to cc8e349 Compare August 5, 2026 09:02

AliRamberg commented Aug 5, 2026

View reviewed changes

AliRamberg merged commit 049b01b into master Aug 5, 2026

		// same filesystem with reflect.DeepEqual, and non-nil func values are never
		// deeply equal, so an inline func field would make every wrapped filesystem

	if !bytes.Equal(userHandle.ToHandle(fs, nil), userHandle.ToHandle(fs2, nil)) {
	if fromDirAttr.FSID == 0 && !reflect.DeepEqual(fs, fs2) {

Uh oh!

Conversation

AliRamberg commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arielshaqed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielshaqed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielshaqed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliRamberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielshaqed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AliRamberg commented Jul 26, 2026 •

edited

Loading