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@xxxxxxxxxxxxxxx # 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@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.
>
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@xxxxxxxxxx>
