Thanks Eirik and Jeff.
At this point in the release cycle, I plan to apply this for the
next merge window (6.5).
A few cosmetic remarks below. I can take this as-is and make
adjustments, or you can redrive. Just let me know.
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@xxxxxxxxxxxxxxx # v6.3+
> Fixes: f5f9d4a314da ("nfsd: move reply cache initialization into nfsd startup")
Why both Fixes: and Cc: stable?
> Reported-and-Tested-by: Eirik Fuller <efuller@xxxxxxxxxx>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2215429 ?
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> 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.
> {
> 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.
> {
> 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.
> --
> 2.40.1
>
