Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Neil Brown <neilb@suse.de>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 7/9] NFSD: Use rhashtable for managing nfs4_file
 objects
Thread-Topic: [PATCH v2 7/9] NFSD: Use rhashtable for managing nfs4_file
 objects
Thread-Index: AQHY2Z+f4KLIvxFyZE+pJJ7lJKAPUq4IWjWAgADUNAA=
Date:   Tue, 11 Oct 2022 12:56:09 +0000
Message-ID: <EB08B095-BF02-4B5E-8CD2-12B0201328D2@oracle.com>
References: <166507275951.1802.13184584115155050247.stgit@manet.1015granger.net>
 <166507324882.1802.884870684212914640.stgit@manet.1015granger.net>
 <166544739751.14457.9018300177489236723@noble.neil.brown.name>
In-Reply-To: <166544739751.14457.9018300177489236723@noble.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ewr8kGHcNmaR+ZALr7Dtx3wMIun8N4Nq4yKM5tlm1AvfLlV405/PCNi++Hpv?=
 =?us-ascii?Q?cUoE2dwoXDrMq8Z6+wWREeGLKFe61Kn+bz+OvmFa58NmUEIbCHe23MfEGmGX?=
 =?us-ascii?Q?bPmlxMvcp7uku3z4dQW1d1NEcPAYiqr4NhtjNo0gOcTTF+Sj0pz1U7+UXXHJ?=
 =?us-ascii?Q?x6lwfcicywfDXmOZu+aAZ2L1ga8tOIZYu1WUiho38ioGm+kMsQlDicItxf4x?=
 =?us-ascii?Q?z94MiWSlrQph8jcg+3zqQ6v7pdd8RPlPO9lXj74r9+TFEVtFE69azaFV5m6n?=
 =?us-ascii?Q?VZkOEE78xFoBlPtsRKwIrC2UhlAfwqKH6ap4C7SzCLSQ7cgKqcdSCFNqound?=
 =?us-ascii?Q?fTYQLOtccPZSTsTR36d9lHh6VfeMAAArictCC+3xdByXz5LqRWC1lAAscmXm?=
 =?us-ascii?Q?JNWqXv6FHZQVq1aygeor6fazC0h1SO2l1XRuu3cKLYHNFQKK/G33yjxZeHL9?=
 =?us-ascii?Q?G+foYoC1QQBommwTQXXUKDYvFmaK/TNxfv6QEBEncXDLaO4ybhDf5WEKX+pZ?=
 =?us-ascii?Q?Oc+Tb/X+hkPue8cW4hzP2uzAwl765TrxVYy4fvgU1TEsVzAMGZUeqtFD2RQ9?=
 =?us-ascii?Q?ZQPxURYzAWaHumnrJb8uLCf81EuDEADYmqJacSgnobUUChM/C6Um8uWDbVSA?=
 =?us-ascii?Q?6cYD0VTiudqsEztV8W3qTbmq9KheR1QVz68BWJSPWyR8PEIDG5XoTq1hay3Z?=
 =?us-ascii?Q?pDjApcR7fq0czZ8UPR7DEmWAr1lzIF/Zmj2EGDLOcBe2d6fEAh+VA+oZ+ext?=
 =?us-ascii?Q?R8VK9vnVGSt6DchtJlCLtwkLAqUpqHN6b1Y/vjGFB1+uDnJa6Ncr4f3XZmyd?=
 =?us-ascii?Q?4jXXPQ6iBt1boLX3jToXNQaMut/u9SfZrWC9mSp6aLbKEoZsa4HtkLC3QhJ/?=
 =?us-ascii?Q?LPpurnfaO0f09wLGrjSFlC7Ng0RE4EGcJll4XgUI6Wus4YdxzZemvZffXexu?=
 =?us-ascii?Q?iZ+yL21HbubqOiVGDfyGluCozgP9EHIKPnSVd3L19PRriMt2VqK5KALWax/b?=
 =?us-ascii?Q?Vawn7eWwYPKeJlYrq6DVpGb4ZKmMwICl/07aremzidUEWUnn44+WniJoYVZC?=
 =?us-ascii?Q?cUDz0Hh1l7+oSrXgE32MusdjVnCgw6SExHFSkekBML0wrAJJ7p0rUY0mP68Q?=
 =?us-ascii?Q?OWMeSAMqvEXm/pmMaPPyyXnxynEfeN318HCpXxPuVxLQsSk0rPBVo/TnNH5f?=
 =?us-ascii?Q?8+uatVx52GG0rFD6YYuZ3DRN8/Jccl4yvcvygeWXxIKfbQFLy/bmyOpZV1ry?=
 =?us-ascii?Q?qY1/VYqJdMUSmAjGWtCFKbqUkBP/9IDaR3lY7oziX7LOsXLAuCg3i12VFXG6?=
 =?us-ascii?Q?8OH8OF96ne3AHdYn38qwH6btrOluGJ3sAVIJAU/p6zgKm2ECUUlZ/ll/B0ga?=
 =?us-ascii?Q?G/kb3ofzTQ3wBm3Um6/oosd9KH7ZVy6zsqAS3gAfIOipcE3Ze3bIItgx9WhF?=
 =?us-ascii?Q?yD3KBKvmh7CdqWU6Ys3cSTlVfIehIOxe6aJj2+1cLuQtllFXYB2Uul44bx9D?=
 =?us-ascii?Q?y7OcZl4BhXseTex3amjv8zv4gGUhqznQCM6TlojnurD4Oy9pCLnmHV7F6u/F?=
 =?us-ascii?Q?kv/IrgFsLlO0k7fhGYPtJ5NpkJe/t+O3WRVGnAJkHLO0T8/QrSWu4G40CDH6?=
 =?us-ascii?Q?NA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <512856B508CF584CBE21252CA0352480@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 51624672-62ab-47a6-7188-08daab87f7e3
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Oct 2022 12:56:09.3847
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: y3qi0zxsuw6jydvupE+pYZvNiNnwFbM8dDCebybjSzhX2RLkMlqXHGdPcgDHTzPNngyV7oz52vDsEM9NzBudJw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB6324
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-11_07,2022-10-11_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 bulkscore=0
 adultscore=0 malwarescore=0 phishscore=0 suspectscore=0 mlxscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2210110073
X-Proofpoint-ORIG-GUID: Y6Odrs3Y9N8IES63eXa6njZA5M4aqsj1
X-Proofpoint-GUID: Y6Odrs3Y9N8IES63eXa6njZA5M4aqsj1
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


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