Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "Trond Myklebust" <trondmy@hammerspace.com>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2] NFSD: Fix RCU-related sparse splat
In-reply-to: <2ABC02B1-CE76-422A-B64F-64B108B12C0B@oracle.com>
References: <163821156142.90770.4019362941494014139.stgit@bazille.1015granger.net>,
 <20211130225250.GC641268@paulmck-ThinkPad-P17-Gen-1>,
 <163831537509.26075.12859361728901873347@noble.neil.brown.name>,
 <2ABC02B1-CE76-422A-B64F-64B108B12C0B@oracle.com>
Date:   Wed, 01 Dec 2021 12:38:50 +1100
Message-id: <163832273051.26075.5034720991945492453@noble.neil.brown.name>
Precedence: bulk

On Wed, 01 Dec 2021, Chuck Lever III wrote:
> 
> By way of further explanation:
> 
> The Documentation/ for RCU (ie, "RCU for Dummies") suggests that
> rcu_assign_pointer() is adequate for preventing undesirable
> compiler optimizations or CPU load/store reordering.
> 
> rcu_assign_pointer() uses WRITE_ONCE for constants like NULL and
> smp_store_release() when assigning variable values. I copied the
> use of WRITE_ONCE() from rcu_assign_pointer(). So I expect exactly
> zero change in behavior or compiled code... (but I should have
> checked the generated object to verify that is the case).

True, there would be no change in behaviour - but we should at least ask
if that behaviour is correct, and why.

If any barriers are needed here, they would have to be between the
assignment of NULL here, and the tests of l->net in
   nfsd_file_list_add_disposal()
   nfsd_free_fcache_disposal_net()
(because they are the only places ->net is used)
The only conceivable race is that they will see a value in ->net "after"
NULL has been assigned.
If there were a race there, it would be between different cpus, so
 smp_store_relase() and smp_load_acquire()
would be the correct tools to avoid that race.

If, on the other hand, there is no chance of a race, then there is no
need to assign NULL to ->net at all.  I believe this is actually the
case.
As the 'net' is freed using kfree_rcu, there is no possibility for a
search that started before something was removed from the list to get a
false-positive when testing if l->net == net.

So while your change is safe from a behavioural perspective, I don't
think it is safe from a maintenance perspective because it leaves in
place code that doesn't really make sense, but removes the warning that
helps us to find that nonsense.

> 
> Sure. If it makes sense to use nfsd_net instead of a separate data
> structure, then that can be made to happen. I would like to hear
> from Trond regarding why he felt a separate data structure was
> necessary for fcache_disposal.

That would be best, yes.

Thanks,
NeilBrown