Re: [PATCH] nfsd: move init of percpu reply_cache_stats counters back to nfsd_init_net

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2023-06-16 at 16:27 -0400, Chuck Lever wrote:
> Thanks Eirik and Jeff.
> 
> At this point in the release cycle, I plan to apply this for the
> next merge window (6.5).
> 

I think we should take this in sooner. This is a regression and a
user-triggerable oops in the right situation. If:

- non-x86_64 arch
- /proc/fs/nfsd is mounted in the namespace
- nfsd is not started in the namespace
- unprivileged user calls "cat /proc/fs/nfsd/reply_cache_stats"

> A few cosmetic remarks below. I can take this as-is and make
> adjustments, or you can redrive. Just let me know.
> 
> 

I'll plan to redrive it.

> On Fri, Jun 16, 2023 at 03:17:43PM -0400, Jeff Layton wrote:
> > f5f9d4a314da moved the initialization of the reply cache into the nfsd
> > startup, but it didn't account for the stats counters which can be
> > accessed before nfsd is ever started, causing a NULL pointer
> > dereference.
> > 
> > This is easy to trigger on some arches (like aarch64), but on x86_64,
> > calling this_cpu_ptr(NULL) evidently returns a pointer to the
> > fixed_percpu_data, which I guess this looks just enough like a newly
> > initialized percpu var to allow nfsd_reply_cache_stats_show to access it
> > without Oopsing.
> > 
> > Move the initialization of the per-net+per-cpu reply-cache counters back
> > into nfsd_init_net, while leaving the rest of the reply cache
> > allocations to be done at nfsd startup time.
> > 
> > Kudos to Eirik who did most of the legwork to track this down.
> > 
> > Cc: stable@vger.kernel.org # v6.3+
> > Fixes: f5f9d4a314da ("nfsd: move reply cache initialization into nfsd startup")
> 
> Why both Fixes: and Cc: stable?
> 
> 

*shrug* : they mean different things. I can drop the Cc stable.

> > Reported-and-Tested-by: Eirik Fuller <efuller@redhat.com>
> 
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2215429  ?
> 
> 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/cache.h    |  2 ++
> >  fs/nfsd/nfscache.c | 13 +++----------
> >  fs/nfsd/nfsctl.c   |  8 ++++++++
> >  3 files changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/nfsd/cache.h b/fs/nfsd/cache.h
> > index f21259ead64b..a4b12d6c41d3 100644
> > --- a/fs/nfsd/cache.h
> > +++ b/fs/nfsd/cache.h
> > @@ -80,6 +80,8 @@ enum {
> >  
> >  int	nfsd_drc_slab_create(void);
> >  void	nfsd_drc_slab_free(void);
> > +int	nfsd_reply_cache_stats_init(struct nfsd_net *nn);
> > +void	nfsd_reply_cache_stats_destroy(struct nfsd_net *nn);
> >  int	nfsd_reply_cache_init(struct nfsd_net *);
> >  void	nfsd_reply_cache_shutdown(struct nfsd_net *);
> >  int	nfsd_cache_lookup(struct svc_rqst *);
> > diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
> > index 041faa13b852..b696dc629c0b 100644
> > --- a/fs/nfsd/nfscache.c
> > +++ b/fs/nfsd/nfscache.c
> > @@ -148,12 +148,12 @@ void nfsd_drc_slab_free(void)
> >  	kmem_cache_destroy(drc_slab);
> >  }
> >  
> > -static int nfsd_reply_cache_stats_init(struct nfsd_net *nn)
> > +int nfsd_reply_cache_stats_init(struct nfsd_net *nn)
> 
> As part of making these two functions into a more public API, I
> would prefer to rename this nfsd_net_reply_cache_init(), and it
> should include a KDOC comment out front.
> 
> I'm having some trouble easily distinguishing between the purpose of
> 
>    static __net_init int nfsd_init_net(struct net *net)
> 
> and
> 
>    static int nfsd_startup_net(struct net *net, const struct cred *cred)
> 
> The former is invoked when a net namespace is created. The latter is
> called by write_threads, therefore /proc/fs/nfsd/ must already be
> mounted.
> 
> The function names are confusingly similar and there's no KDOC to be
> found. I might add a clean-up patch to this one.
> 

I can add some kdoc patches in v2.
> >  {
> >  	return nfsd_percpu_counters_init(nn->counter, NFSD_NET_COUNTERS_NUM);
> >  }
> >  
> > -static void nfsd_reply_cache_stats_destroy(struct nfsd_net *nn)
> > +void nfsd_reply_cache_stats_destroy(struct nfsd_net *nn)
> 
> Ditto, rename this nfsd_net_reply_cache_destroy(), plus KDOC.
> 
> 

Ok.

> >  {
> >  	nfsd_percpu_counters_destroy(nn->counter, NFSD_NET_COUNTERS_NUM);
> >  }
> > @@ -169,17 +169,13 @@ int nfsd_reply_cache_init(struct nfsd_net *nn)
> >  	hashsize = nfsd_hashsize(nn->max_drc_entries);
> >  	nn->maskbits = ilog2(hashsize);
> >  
> > -	status = nfsd_reply_cache_stats_init(nn);
> > -	if (status)
> > -		goto out_nomem;
> > -
> >  	nn->nfsd_reply_cache_shrinker.scan_objects = nfsd_reply_cache_scan;
> >  	nn->nfsd_reply_cache_shrinker.count_objects = nfsd_reply_cache_count;
> >  	nn->nfsd_reply_cache_shrinker.seeks = 1;
> >  	status = register_shrinker(&nn->nfsd_reply_cache_shrinker,
> >  				   "nfsd-reply:%s", nn->nfsd_name);
> >  	if (status)
> > -		goto out_stats_destroy;
> > +		return status;
> >  
> >  	nn->drc_hashtbl = kvzalloc(array_size(hashsize,
> >  				sizeof(*nn->drc_hashtbl)), GFP_KERNEL);
> > @@ -195,9 +191,6 @@ int nfsd_reply_cache_init(struct nfsd_net *nn)
> >  	return 0;
> >  out_shrinker:
> >  	unregister_shrinker(&nn->nfsd_reply_cache_shrinker);
> > -out_stats_destroy:
> > -	nfsd_reply_cache_stats_destroy(nn);
> > -out_nomem:
> >  	printk(KERN_ERR "nfsd: failed to allocate reply cache\n");
> >  	return -ENOMEM;
> >  }
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 1489e0b703b4..7c837afcf615 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1505,6 +1505,9 @@ static __net_init int nfsd_init_net(struct net *net)
> >  	retval = nfsd_idmap_init(net);
> >  	if (retval)
> >  		goto out_idmap_error;
> > +	retval = nfsd_reply_cache_stats_init(nn);
> > +	if (retval)
> > +		goto out_repcache_error;
> >  	nn->nfsd_versions = NULL;
> >  	nn->nfsd4_minorversions = NULL;
> >  	nfsd4_init_leases_net(nn);
> > @@ -1513,6 +1516,8 @@ static __net_init int nfsd_init_net(struct net *net)
> >  
> >  	return 0;
> >  
> > +out_repcache_error:
> > +	nfsd_idmap_shutdown(net);
> >  out_idmap_error:
> >  	nfsd_export_shutdown(net);
> >  out_export_error:
> > @@ -1521,6 +1526,9 @@ static __net_init int nfsd_init_net(struct net *net)
> >  
> >  static __net_exit void nfsd_exit_net(struct net *net)
> >  {
> > +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > +
> > +	nfsd_reply_cache_stats_destroy(nn);
> >  	nfsd_idmap_shutdown(net);
> >  	nfsd_export_shutdown(net);
> >  	nfsd_netns_free_versions(net_generic(net, nfsd_net_id));
> 
> We should update this nfsd_netns_free_versions() call site to take
> the new @nn variable.
> 

Ahh yes. Will fix.

I'll plan to send a v2 with the changes you suggest.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>