On Oct 10, 2022, at 8:16 PM, NeilBrown [off-list ref] wrote:

On Fri, 07 Oct 2022, Chuck Lever wrote:

quoted

-static unsigned int file_hashval(struct svc_fh *fh)
+/*
+ * The returned hash value is based solely on the address of an in-code
+ * inode, a pointer to a slab-allocated object. The entropy in such a
+ * pointer is concentrated in its middle bits.

I think you need more justification than that for throwing away some of
the entropy, even if you don't think it is much.

We might have that justification:

https://lore.kernel.org/linux-nfs/YrUFbLJ5uVbWtZbf@ZenIV/ (local)

Actually I believe we are not discarding /any/ entropy in
this function. The bits we discard are invariant.

And, note that this is exactly the same situation we just merged
in the filecache overhaul, and is a common trope amongst other
hash tables that base their function on the inode's address.

Presumably you think hashing 32 bits is faster than hashing 64 bits.
Maybe it is, but is it worth it?

rhashtable depends heavily on having a strong hash function.  In
particular if any bucket ends up with more than 16 elements it will
choose a new seed and rehash.  If you deliberately remove some bits that
it might have been used to spread those 16 out, then you are asking for
trouble.

We know that two different file handles can refer to the same inode
("rarely"), and you deliberately place them in the same hash bucket.
So if an attacker arranges to access 17 files with the same inode but
different file handles, then the hashtable will be continuously
rehashed.

The preferred approach when you require things to share a hash chain is
to use an rhl table.

Again, this is the same situation for the filecache. Do you
believe it is worth reworking that? I'm guessing "yes".

This allows multiple instances with the same key.
You would then key the rhl-table with the inode, and search a
linked-list to find the entry with the desired file handle.  This would
be no worse in search time than the current code for aliased inodes, but
less susceptible to attack.

quoted

+/**
+ * nfs4_file_obj_cmpfn - Match a cache item against search criteria
+ * @arg: search criteria
+ * @ptr: cache item to check
+ *
+ * Return values:
+ *   %0 - Item matches search criteria
+ *   %1 - Item does not match search criteria

I *much* prefer %-ESRCH for "does not match search criteria".  It is
self-documenting.  Any non-zero value will do.

Noted, but returning 1 appears to be the typical arrangement for
existing obj_cmpfn methods in most other areas.


--
Chuck Lever