Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     paulmck@kernel.org
Cc:     "Chuck Lever" <chuck.lever@oracle.com>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH v2] NFSD: Fix RCU-related sparse splat
In-reply-to: <20211130225250.GC641268@paulmck-ThinkPad-P17-Gen-1>
References: <163821156142.90770.4019362941494014139.stgit@bazille.1015granger.net>,
 <20211130225250.GC641268@paulmck-ThinkPad-P17-Gen-1>
Date:   Wed, 01 Dec 2021 10:36:15 +1100
Message-id: <163831537509.26075.12859361728901873347@noble.neil.brown.name>
Precedence: bulk

On Wed, 01 Dec 2021, paulmck@kernel.org wrote:
> On Mon, Nov 29, 2021 at 01:46:07PM -0500, Chuck Lever wrote:
> > To address this error:
> >=20
> >   CC [M]  fs/nfsd/filecache.o
> >   CHECK   /home/cel/src/linux/linux/fs/nfsd/filecache.c
> > /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: error: incompatible =
types in comparison expression (different address spaces):
> > /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9:    struct net [noder=
ef] __rcu *
> > /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9:    struct net *
> >=20
> > The "net" field in struct nfsd_fcache_disposal is not annotated as
> > requiring an RCU assignment, so replace the macro that includes an
> > invocation of rcu_check_sparse() with an equivalent that does not.
> >=20
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>=20
> From an RCU perspective:
>=20
> Acked-by: Paul E. McKenney <paulmck@kernel.org>
>=20
> But it would be good to get someone more familiar with the code to
> look at this.

There is a global list of 'struct nfsd_fcache_disposal', potentially one
for each network namespace.  Each contains a '*net' which is effectively
an opaque lookup key.  It is never dereferenced - only used to find the
nfsd_fcache_disposal which matches a given 'struct net *'.

The lookup happens under rcu_read_lock().  A 'struct net' is freed after
a grace period (see cleanup_net() in net_namespace.c) so to ensure the
lookup doesn't find a false positive - caused by a 'struct net' being
deallocated and reallocated, it is sufficient to clear the ->net pointer
while the the net is known to still be active.  At most, a write-barrier
might be justified.  i.e. smp_store_release(l->net, NULL);

However ... the list of nfsd_fcache_disposal seems unnecessary.
nfsd has a 'struct nfsd_net' which parallels any interesting 'struct
net' using the net_generic() framework.
If the nfs_fcache_disposal were linked from the nfsd_net, there would be
no need for the list, no need to record the ->net, and not need for the
code in question here.
The nfsd_fcache_disposal is destroyed (nfsd_file_cache_shutdown_new())
before the nfsd_net is destroyed, so there are no lifetime issues with
keeping the separate list.

So I think this patch is not the best fix for the problem highlighted by
the warning...  it's not wrong though.

NeilBrown