> On Oct 10, 2022, at 8:16 PM, NeilBrown <neilb@xxxxxxx> wrote: > > On Fri, 07 Oct 2022, Chuck Lever wrote: >> >> -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/ 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. > >> +/** >> + * 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
