netdev - Re: [PATCH RFC 29/30] NFSD: Convert the filecache to use rhashtable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 23 Jun 2022 17:27:20 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>, "tgraf@...g.ch" <tgraf@...g.ch>,
        Dave Chinner <david@...morbit.com>,
        Jeff Layton <jlayton@...hat.com>
Subject: Re: [PATCH RFC 29/30] NFSD: Convert the filecache to use rhashtable

> On Jun 22, 2022, at 8:58 PM, Chuck Lever III <chuck.lever@...cle.com> wrote:
> 
>> On Jun 22, 2022, at 8:38 PM, Dave Chinner <david@...morbit.com> wrote:
>> 
>> On Wed, Jun 22, 2022 at 10:15:56AM -0400, Chuck Lever wrote:
> 
>>> +
>>> +/*
>>> + * Atomically insert a new nfsd_file item into nfsd_file_rhash_tbl.
>>> + *
>>> + * Return values:
>>> + *   %NULL: @new was inserted successfully
>>> + *   %A valid pointer: @new was not inserted, a matching item is returned
>>> + *   %ERR_PTR: an unexpected error occurred during insertion
>>> + */
>>> +static struct nfsd_file *nfsd_file_insert(struct nfsd_file *new)
>>> +{
>>> +	struct nfsd_file_lookup_key key = {
>>> +		.type	= NFSD_FILE_KEY_FULL,
>>> +		.inode	= new->nf_inode,
>>> +		.need	= new->nf_flags,
>>> +		.net	= new->nf_net,
>>> +		.cred	= current_cred(),
>>> +	};
>>> +	struct nfsd_file *nf;
>>> +
>>> +	nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
>>> +					      &key, &new->nf_rhash,
>>> +					      nfsd_file_rhash_params);
>>> +	if (!nf)
>>> +		return nf;
>> 
>> The insert can return an error (e.g. -ENOMEM) so need to check
>> IS_ERR(nf) here as well.
> 
> That is likely the cause of the BUG that Wang just reported, as
> that will send a ERR_PTR to nfsd_file_get(), which blows up when
> it tries to defererence it.

Yep, that was it. I've fixed it, but some other doubts have surfaced
in the meantime.

Removing the .max_size cap also helps, and in the long run, I now
feel that cap should be left off. But I would like to be certain that
nfsd_file_acquire's logic works when hard errors occur, so I left the cap
in place for now. I found that the "failed to open newly created file!"
warning fires when insertion fails. I need to work on addressing that
case silently.

Also I just found Neil's nice rhashtable explainer:

   https://lwn.net/Articles/751374/

Where he writes that:

> Sometimes you might want a hash table to potentially contain multiple objects for any given key. In that case you can use "rhltables" — rhashtables with lists of objects.

I believe that is the case for the filecache. The hash value is
computed based on the inode pointer, and therefore there can be more
than one nfsd_file object for a particular inode (depending on who
is opening and for what access). So I think filecache needs to use
rhltable, not rhashtable. Any thoughts from rhashtable experts?

--
Chuck Lever