FileHandle vs Inode (real and pseudo) vs Inode's FileId (byte[]) vs. Stat ino; PseudoFS, Opaque & co.

[kohlschuetter]

Going through the NFS4j code, I'm a bit consued by the many different layers of referencing an inode in the underlying file system. We seem to be converting, serializing, deserializing, masquerading, extracting and ultimately conflating different objects. This not only makes the code quite complex and hard to follow, it also adds to the memory and CPU footprint.

Ultimately, from a protocol perspective, NFSv4 expects nfs_fh4 to be 128 bytes of rather arbitrary content, but also, in some places (e.g., getattr), expects a 64-bit "file id" (similar to a UNIX "inode").

In our code, however I see the following:

1. FileHandle: is practically only referenced by Inode, which encapsulates it, but Inode provides hashcode/equality based upon FileHandle's byte representation, which seems to be the only functional addition over FileHandle itself. However that code is very inefficient as it builds a ByteBuffer/byte[] for each check.
2. In FileTracker, VfsCache, LockManager, etc., we don't track the Inode, we track inode.getFileId() (which at some point could be a byte[] of rather arbitrary length), and we may even wrap it into an Opaque object before each use
3. LocalFileSystem uses incrementally generated long-based "inode" values (which are not based upon the actual file system inodes — this may cause problems when restarting the server, especially after altering the local file system)
4. Stat returns a 64-bit long for getIno() ("Ino" refers to "inode"), which is reported to NFS as the "fileid".
5. PseudoFS encodes additional bits of information (such as "exportIndex") into the FileHandle. By making access decisions based on bits encoded in the file handle, a malicious NFS client may trigger privilege escalation by manipulating the "exportIndex" part. The current logic is probably also the reason why we can't use the inode's full 128 bytes by the downstream VirtualFileSystem.

I wonder if there is interest in simplifying this structure as follows. As I'm new to the codebase, please correct my assumptions if I'm missing something important:

1. Combine FileHandle and Inode into one type, and simplify the hashCode/equals checks, remove the various byte[] serialiation steps, etc. We can keep calling this "Inode" for the sake of API compatibility. (I have working code that extends Inode from FileHandle, but I think we could also completely absorb that class into one).
2. Make FileTracker etc. reference the Inode objects directly instead of byte[]/Opaque.
3. LocalFileSystem could use random UUIDs (or the underlying FS's actual inode number when available) instead of incrementally added numbers. This seems to fix an inode-confusion issue I'm observing on macOS after restarting the NFS server (while modifying the contents of the underlying file system between restarts).
4. Make reporting of a 64-bit "inode" fileid optional (= make Stat.ino refer to a Long object instead of long, or maybe interpret 0 as "no value" in NFSv4).
5. Rework PseudoFS to track state internally instead of exposing it via bytes in the NFS file handle object (this is the item with the least priority for me, as I might be able to just skip that level of indirection for my use case, but I think this is critical for hardening NFS4j's security).

I already have working code for most of this, but I first want to understand if there's 1. any interest to pursue this and 2. anything else I'm missing here.

[kofemann]

@kohlschuetter, thanks for starting this discussion.

Indeed, the code is confusing and (probably) over-engineered/complicated. The main reason for separating FileHandle and Inode is their responsibilities. The FileHandle class is tied to NFS requests on the wire. It includes a pointer to the export entry for quick lookup, an inode type that allows us in dCache to access non-filesystem objects, and so on. A malicious client can construct a manipulated file handle, but is still required to have a valid entry in the export file.

The Inode, on the other hand, represents filesystem object identity. For regular filesystem objects, this is just long that matches ino. Like you have noticed, in many places, like caching, lock management, and file tracking, we use Inode. This is to point to the very same filesystem object, independent of which export entry the client has used, FileHandles created via different exports are different.

The LocalFileSystem is intended as a quick test and was not intended to survive restarts or be used in production. The code is used to run pynfs test-suite in CI. For Linux-based (osx?), you can have a look at https://github.com/kofemann/vfs4j/blob/master/src/main/java/org/dcache/vfs4j/LocalVFS.java. It is still for testing by uses filesystems generated file handles.

So, yeah, I agree, many things can be improved. Low-hanging fruit is fixing Inode vs Opaque. I don't remember why it was done like that. Probably to have a more generic code.

With your change suggestion list above I thinks:

⏸️ 1. Combine FileHandle and Inode into one type

• Needs careful inspection on our side, as the existing functionality might break badly

✅ 2. Make FileTracker etc. reference the Inode
✅ 3. LocalFileSystem could use random UUID
❓ 4. Make reporting of a 64-bit "inode" fileid optional

• fileid is a required attribute in NFS4 spec.

🔧 5. Rework PseudoFS to track state internally instead of exposing it via bytes.

• I am for simplicity and security. But we need to pay extra attention not to break existing installations. In most cases, we have a single export entry per client, so they can't really escape.

[kofemann]

I had yet another look at FileHandle vs. Inode. Indeed, there is not much logic there. I think they can be merged. ⏯️

[kohlschuetter]

Cool. I understand compatibility with existing installations is very important, so will try to make all changes backwards compatible.

Regarding fileid: That is a "recommended" field (not "required") according to the NFSv4 specs (RFC 8881, 5661, 7530). It's required for NFSv3 though.

I think I can make this a backwards-compatible change since it will only apply to new file systems and new NFSv4-only setups. OperationGETATTR already returns Optional for all attributes, so it's worth testing if it works for actual NFSv4 clients.

Concerning PseudoFS, I will try to make as few changes as possible, especially as I'm already quite sure I don't need that layer in my setup. Right now, we always add it as a shim; I'll look into making that optional.

[kohlschuetter]

First batch of changes: #153