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;@@ -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,