Message ID | 20250206-nfsd-fixes-v1-1-c6647b92ca6f@kernel.org (mailing list archive) |
---|---|
State | In Next |
Delegated to: | Chuck Lever |
Headers | show |
Series | nfsd: don't ignore the return code of svc_proc_register() | expand |
On 2/6/25 1:12 PM, Jeff Layton wrote: > Currently, nfsd_proc_stat_init() ignores the return value of > svc_proc_register(). If the procfile creation fails, then the kernel > will WARN when it tries to remove the entry later. > > Fix nfsd_proc_stat_init() to return the same type of pointer as > svc_proc_register(), and fix up nfsd_net_init() to check that and fail > the nfsd_net construction if it occurs. > > svc_proc_register() can fail if the dentry can't be allocated, or if an > identical dentry already exists. The second case is pretty unlikely in > the nfsd_net construction codepath, so if this happens, return -ENOMEM. > > Fixes: 93483ac5fec6 ("nfsd: expose /proc/net/sunrpc/nfsd in net namespaces") > Reported-by: syzbot+e34ad04f27991521104c@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/linux-nfs/67a47501.050a0220.19061f.05f9.GAE@google.com/ > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > I looked at the console log from the report, and syzkaller is doing > fault injection on allocations. You can see the stack where the "nfsd" > directory under /proc failed to be created due to one. This is a pretty > unlikely bug under normal circumstances, but it's simple to fix. The > problem predates the patch in Fixes:, but it's not worth the effort to > backport this to anything earlier. I'd prefer to document this by labeling the actual commit that introduced the problem in the Fixes: tag, then using "Cc: stable # vN.M" to block automatic backporting to LTS kernels where this patch won't apply cleanly. I can derive the values of N and M from the commit you mention above, but do you happen to know the actual culprit commit? > --- > fs/nfsd/nfsctl.c | 9 ++++++++- > fs/nfsd/stats.c | 4 ++-- > fs/nfsd/stats.h | 2 +- > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > index 95ea4393305bd38493b640fbaba2e8f57f5a501d..583eda0df54dca394de4bbe8d148be2892df39cb 100644 > --- a/fs/nfsd/nfsctl.c > +++ b/fs/nfsd/nfsctl.c > @@ -2204,8 +2204,14 @@ static __net_init int nfsd_net_init(struct net *net) > NFSD_STATS_COUNTERS_NUM); > if (retval) > goto out_repcache_error; > + > memset(&nn->nfsd_svcstats, 0, sizeof(nn->nfsd_svcstats)); > nn->nfsd_svcstats.program = &nfsd_programs[0]; > + if (!nfsd_proc_stat_init(net)) { > + retval = -ENOMEM; > + goto out_proc_error; > + } > + > for (i = 0; i < sizeof(nn->nfsd_versions); i++) > nn->nfsd_versions[i] = nfsd_support_version(i); > for (i = 0; i < sizeof(nn->nfsd4_minorversions); i++) > @@ -2215,12 +2221,13 @@ static __net_init int nfsd_net_init(struct net *net) > nfsd4_init_leases_net(nn); > get_random_bytes(&nn->siphash_key, sizeof(nn->siphash_key)); > seqlock_init(&nn->writeverf_lock); > - nfsd_proc_stat_init(net); > #if IS_ENABLED(CONFIG_NFS_LOCALIO) > INIT_LIST_HEAD(&nn->local_clients); > #endif > return 0; > > +out_proc_error: > + percpu_counter_destroy_many(nn->counter, NFSD_STATS_COUNTERS_NUM); > out_repcache_error: > nfsd_idmap_shutdown(net); > out_idmap_error: > diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c > index bb22893f1157e4c159e123b6d8e25b8eab52e187..f7eaf95e20fc8758566f469c6c2de79119fea070 100644 > --- a/fs/nfsd/stats.c > +++ b/fs/nfsd/stats.c > @@ -73,11 +73,11 @@ static int nfsd_show(struct seq_file *seq, void *v) > > DEFINE_PROC_SHOW_ATTRIBUTE(nfsd); > > -void nfsd_proc_stat_init(struct net *net) > +struct proc_dir_entry *nfsd_proc_stat_init(struct net *net) > { > struct nfsd_net *nn = net_generic(net, nfsd_net_id); > > - svc_proc_register(net, &nn->nfsd_svcstats, &nfsd_proc_ops); > + return svc_proc_register(net, &nn->nfsd_svcstats, &nfsd_proc_ops); > } > > void nfsd_proc_stat_shutdown(struct net *net) > diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h > index 04aacb6c36e2576ba231ee481e3a3e9e9f255a61..e4efb0e4e56d467c13eaa5a1dd312c85dadeb4b8 100644 > --- a/fs/nfsd/stats.h > +++ b/fs/nfsd/stats.h > @@ -10,7 +10,7 @@ > #include <uapi/linux/nfsd/stats.h> > #include <linux/percpu_counter.h> > > -void nfsd_proc_stat_init(struct net *net); > +struct proc_dir_entry *nfsd_proc_stat_init(struct net *net); > void nfsd_proc_stat_shutdown(struct net *net); > > static inline void nfsd_stats_rc_hits_inc(struct nfsd_net *nn) > > --- > base-commit: ebbdc9429c39336a406b191cfe84bca2c12c2f73 > change-id: 20250206-nfsd-fixes-8e61bdf66347 > > Best regards,
On Thu, 2025-02-06 at 13:17 -0500, Chuck Lever wrote: > On 2/6/25 1:12 PM, Jeff Layton wrote: > > Currently, nfsd_proc_stat_init() ignores the return value of > > svc_proc_register(). If the procfile creation fails, then the kernel > > will WARN when it tries to remove the entry later. > > > > Fix nfsd_proc_stat_init() to return the same type of pointer as > > svc_proc_register(), and fix up nfsd_net_init() to check that and fail > > the nfsd_net construction if it occurs. > > > > svc_proc_register() can fail if the dentry can't be allocated, or if an > > identical dentry already exists. The second case is pretty unlikely in > > the nfsd_net construction codepath, so if this happens, return -ENOMEM. > > > > Fixes: 93483ac5fec6 ("nfsd: expose /proc/net/sunrpc/nfsd in net namespaces") > > Reported-by: syzbot+e34ad04f27991521104c@syzkaller.appspotmail.com > > Closes: https://lore.kernel.org/linux-nfs/67a47501.050a0220.19061f.05f9.GAE@google.com/ > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > I looked at the console log from the report, and syzkaller is doing > > fault injection on allocations. You can see the stack where the "nfsd" > > directory under /proc failed to be created due to one. This is a pretty > > unlikely bug under normal circumstances, but it's simple to fix. The > > problem predates the patch in Fixes:, but it's not worth the effort to > > backport this to anything earlier. > > I'd prefer to document this by labeling the actual commit that > introduced the problem in the Fixes: tag, then using > > "Cc: stable # vN.M" > > to block automatic backporting to LTS kernels where this patch won't > apply cleanly. I can derive the values of N and M from the commit you > mention above, but do you happen to know the actual culprit commit? > > Unfortunately this bug goes back to the initial 2.6.12 import into git. I didn't look earlier. Note that nfsd is not alone here. Ignoring the result of proc_create_data() is very common. If you want to drop the Fixes: tag, and add the Cc: stable instead, then that's fine with me. Whatever works best. > > --- > > fs/nfsd/nfsctl.c | 9 ++++++++- > > fs/nfsd/stats.c | 4 ++-- > > fs/nfsd/stats.h | 2 +- > > 3 files changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > > index 95ea4393305bd38493b640fbaba2e8f57f5a501d..583eda0df54dca394de4bbe8d148be2892df39cb 100644 > > --- a/fs/nfsd/nfsctl.c > > +++ b/fs/nfsd/nfsctl.c > > @@ -2204,8 +2204,14 @@ static __net_init int nfsd_net_init(struct net *net) > > NFSD_STATS_COUNTERS_NUM); > > if (retval) > > goto out_repcache_error; > > + > > memset(&nn->nfsd_svcstats, 0, sizeof(nn->nfsd_svcstats)); > > nn->nfsd_svcstats.program = &nfsd_programs[0]; > > + if (!nfsd_proc_stat_init(net)) { > > + retval = -ENOMEM; > > + goto out_proc_error; > > + } > > + > > for (i = 0; i < sizeof(nn->nfsd_versions); i++) > > nn->nfsd_versions[i] = nfsd_support_version(i); > > for (i = 0; i < sizeof(nn->nfsd4_minorversions); i++) > > @@ -2215,12 +2221,13 @@ static __net_init int nfsd_net_init(struct net *net) > > nfsd4_init_leases_net(nn); > > get_random_bytes(&nn->siphash_key, sizeof(nn->siphash_key)); > > seqlock_init(&nn->writeverf_lock); > > - nfsd_proc_stat_init(net); > > #if IS_ENABLED(CONFIG_NFS_LOCALIO) > > INIT_LIST_HEAD(&nn->local_clients); > > #endif > > return 0; > > > > +out_proc_error: > > + percpu_counter_destroy_many(nn->counter, NFSD_STATS_COUNTERS_NUM); > > out_repcache_error: > > nfsd_idmap_shutdown(net); > > out_idmap_error: > > diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c > > index bb22893f1157e4c159e123b6d8e25b8eab52e187..f7eaf95e20fc8758566f469c6c2de79119fea070 100644 > > --- a/fs/nfsd/stats.c > > +++ b/fs/nfsd/stats.c > > @@ -73,11 +73,11 @@ static int nfsd_show(struct seq_file *seq, void *v) > > > > DEFINE_PROC_SHOW_ATTRIBUTE(nfsd); > > > > -void nfsd_proc_stat_init(struct net *net) > > +struct proc_dir_entry *nfsd_proc_stat_init(struct net *net) > > { > > struct nfsd_net *nn = net_generic(net, nfsd_net_id); > > > > - svc_proc_register(net, &nn->nfsd_svcstats, &nfsd_proc_ops); > > + return svc_proc_register(net, &nn->nfsd_svcstats, &nfsd_proc_ops); > > } > > > > void nfsd_proc_stat_shutdown(struct net *net) > > diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h > > index 04aacb6c36e2576ba231ee481e3a3e9e9f255a61..e4efb0e4e56d467c13eaa5a1dd312c85dadeb4b8 100644 > > --- a/fs/nfsd/stats.h > > +++ b/fs/nfsd/stats.h > > @@ -10,7 +10,7 @@ > > #include <uapi/linux/nfsd/stats.h> > > #include <linux/percpu_counter.h> > > > > -void nfsd_proc_stat_init(struct net *net); > > +struct proc_dir_entry *nfsd_proc_stat_init(struct net *net); > > void nfsd_proc_stat_shutdown(struct net *net); > > > > static inline void nfsd_stats_rc_hits_inc(struct nfsd_net *nn) > > > > --- > > base-commit: ebbdc9429c39336a406b191cfe84bca2c12c2f73 > > change-id: 20250206-nfsd-fixes-8e61bdf66347 > > > > Best regards, > >
On 2/6/25 1:29 PM, Jeff Layton wrote: > On Thu, 2025-02-06 at 13:17 -0500, Chuck Lever wrote: >> On 2/6/25 1:12 PM, Jeff Layton wrote: >>> Currently, nfsd_proc_stat_init() ignores the return value of >>> svc_proc_register(). If the procfile creation fails, then the kernel >>> will WARN when it tries to remove the entry later. >>> >>> Fix nfsd_proc_stat_init() to return the same type of pointer as >>> svc_proc_register(), and fix up nfsd_net_init() to check that and fail >>> the nfsd_net construction if it occurs. >>> >>> svc_proc_register() can fail if the dentry can't be allocated, or if an >>> identical dentry already exists. The second case is pretty unlikely in >>> the nfsd_net construction codepath, so if this happens, return -ENOMEM. >>> >>> Fixes: 93483ac5fec6 ("nfsd: expose /proc/net/sunrpc/nfsd in net namespaces") >>> Reported-by: syzbot+e34ad04f27991521104c@syzkaller.appspotmail.com >>> Closes: https://lore.kernel.org/linux-nfs/67a47501.050a0220.19061f.05f9.GAE@google.com/ >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>> --- >>> I looked at the console log from the report, and syzkaller is doing >>> fault injection on allocations. You can see the stack where the "nfsd" >>> directory under /proc failed to be created due to one. This is a pretty >>> unlikely bug under normal circumstances, but it's simple to fix. The >>> problem predates the patch in Fixes:, but it's not worth the effort to >>> backport this to anything earlier. >> >> I'd prefer to document this by labeling the actual commit that >> introduced the problem in the Fixes: tag, then using >> >> "Cc: stable # vN.M" >> >> to block automatic backporting to LTS kernels where this patch won't >> apply cleanly. I can derive the values of N and M from the commit you >> mention above, but do you happen to know the actual culprit commit? >> >> > > Unfortunately this bug goes back to the initial 2.6.12 import into git. > I didn't look earlier. Note that nfsd is not alone here. Ignoring the > result of proc_create_data() is very common. > > If you want to drop the Fixes: tag, and add the Cc: stable instead, > then that's fine with me. Whatever works best. OK. If we don't know the culprit, then a lone "Cc: stable" should be sufficient. >>> --- >>> fs/nfsd/nfsctl.c | 9 ++++++++- >>> fs/nfsd/stats.c | 4 ++-- >>> fs/nfsd/stats.h | 2 +- >>> 3 files changed, 11 insertions(+), 4 deletions(-) >>> >>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c >>> index 95ea4393305bd38493b640fbaba2e8f57f5a501d..583eda0df54dca394de4bbe8d148be2892df39cb 100644 >>> --- a/fs/nfsd/nfsctl.c >>> +++ b/fs/nfsd/nfsctl.c >>> @@ -2204,8 +2204,14 @@ static __net_init int nfsd_net_init(struct net *net) >>> NFSD_STATS_COUNTERS_NUM); >>> if (retval) >>> goto out_repcache_error; >>> + >>> memset(&nn->nfsd_svcstats, 0, sizeof(nn->nfsd_svcstats)); >>> nn->nfsd_svcstats.program = &nfsd_programs[0]; >>> + if (!nfsd_proc_stat_init(net)) { >>> + retval = -ENOMEM; >>> + goto out_proc_error; >>> + } >>> + >>> for (i = 0; i < sizeof(nn->nfsd_versions); i++) >>> nn->nfsd_versions[i] = nfsd_support_version(i); >>> for (i = 0; i < sizeof(nn->nfsd4_minorversions); i++) >>> @@ -2215,12 +2221,13 @@ static __net_init int nfsd_net_init(struct net *net) >>> nfsd4_init_leases_net(nn); >>> get_random_bytes(&nn->siphash_key, sizeof(nn->siphash_key)); >>> seqlock_init(&nn->writeverf_lock); >>> - nfsd_proc_stat_init(net); >>> #if IS_ENABLED(CONFIG_NFS_LOCALIO) >>> INIT_LIST_HEAD(&nn->local_clients); >>> #endif >>> return 0; >>> >>> +out_proc_error: >>> + percpu_counter_destroy_many(nn->counter, NFSD_STATS_COUNTERS_NUM); >>> out_repcache_error: >>> nfsd_idmap_shutdown(net); >>> out_idmap_error: >>> diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c >>> index bb22893f1157e4c159e123b6d8e25b8eab52e187..f7eaf95e20fc8758566f469c6c2de79119fea070 100644 >>> --- a/fs/nfsd/stats.c >>> +++ b/fs/nfsd/stats.c >>> @@ -73,11 +73,11 @@ static int nfsd_show(struct seq_file *seq, void *v) >>> >>> DEFINE_PROC_SHOW_ATTRIBUTE(nfsd); >>> >>> -void nfsd_proc_stat_init(struct net *net) >>> +struct proc_dir_entry *nfsd_proc_stat_init(struct net *net) >>> { >>> struct nfsd_net *nn = net_generic(net, nfsd_net_id); >>> >>> - svc_proc_register(net, &nn->nfsd_svcstats, &nfsd_proc_ops); >>> + return svc_proc_register(net, &nn->nfsd_svcstats, &nfsd_proc_ops); >>> } >>> >>> void nfsd_proc_stat_shutdown(struct net *net) >>> diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h >>> index 04aacb6c36e2576ba231ee481e3a3e9e9f255a61..e4efb0e4e56d467c13eaa5a1dd312c85dadeb4b8 100644 >>> --- a/fs/nfsd/stats.h >>> +++ b/fs/nfsd/stats.h >>> @@ -10,7 +10,7 @@ >>> #include <uapi/linux/nfsd/stats.h> >>> #include <linux/percpu_counter.h> >>> >>> -void nfsd_proc_stat_init(struct net *net); >>> +struct proc_dir_entry *nfsd_proc_stat_init(struct net *net); >>> void nfsd_proc_stat_shutdown(struct net *net); >>> >>> static inline void nfsd_stats_rc_hits_inc(struct nfsd_net *nn) >>> >>> --- >>> base-commit: ebbdc9429c39336a406b191cfe84bca2c12c2f73 >>> change-id: 20250206-nfsd-fixes-8e61bdf66347 >>> >>> Best regards, >> >> >
From: Chuck Lever <chuck.lever@oracle.com> On Thu, 06 Feb 2025 13:12:13 -0500, Jeff Layton wrote: > Currently, nfsd_proc_stat_init() ignores the return value of > svc_proc_register(). If the procfile creation fails, then the kernel > will WARN when it tries to remove the entry later. > > Fix nfsd_proc_stat_init() to return the same type of pointer as > svc_proc_register(), and fix up nfsd_net_init() to check that and fail > the nfsd_net construction if it occurs. > > [...] Applied to nfsd-testing, thanks! Added Cc: stable # v6.9 [1/1] nfsd: don't ignore the return code of svc_proc_register() commit: 312aef1c1520c10cf035e27c0b0229bea71f0c68 -- Chuck Lever
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c index 95ea4393305bd38493b640fbaba2e8f57f5a501d..583eda0df54dca394de4bbe8d148be2892df39cb 100644 --- a/fs/nfsd/nfsctl.c +++ b/fs/nfsd/nfsctl.c @@ -2204,8 +2204,14 @@ static __net_init int nfsd_net_init(struct net *net) NFSD_STATS_COUNTERS_NUM); if (retval) goto out_repcache_error; + memset(&nn->nfsd_svcstats, 0, sizeof(nn->nfsd_svcstats)); nn->nfsd_svcstats.program = &nfsd_programs[0]; + if (!nfsd_proc_stat_init(net)) { + retval = -ENOMEM; + goto out_proc_error; + } + for (i = 0; i < sizeof(nn->nfsd_versions); i++) nn->nfsd_versions[i] = nfsd_support_version(i); for (i = 0; i < sizeof(nn->nfsd4_minorversions); i++) @@ -2215,12 +2221,13 @@ static __net_init int nfsd_net_init(struct net *net) nfsd4_init_leases_net(nn); get_random_bytes(&nn->siphash_key, sizeof(nn->siphash_key)); seqlock_init(&nn->writeverf_lock); - nfsd_proc_stat_init(net); #if IS_ENABLED(CONFIG_NFS_LOCALIO) INIT_LIST_HEAD(&nn->local_clients); #endif return 0; +out_proc_error: + percpu_counter_destroy_many(nn->counter, NFSD_STATS_COUNTERS_NUM); out_repcache_error: nfsd_idmap_shutdown(net); out_idmap_error: diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c index bb22893f1157e4c159e123b6d8e25b8eab52e187..f7eaf95e20fc8758566f469c6c2de79119fea070 100644 --- a/fs/nfsd/stats.c +++ b/fs/nfsd/stats.c @@ -73,11 +73,11 @@ static int nfsd_show(struct seq_file *seq, void *v) DEFINE_PROC_SHOW_ATTRIBUTE(nfsd); -void nfsd_proc_stat_init(struct net *net) +struct proc_dir_entry *nfsd_proc_stat_init(struct net *net) { struct nfsd_net *nn = net_generic(net, nfsd_net_id); - svc_proc_register(net, &nn->nfsd_svcstats, &nfsd_proc_ops); + return svc_proc_register(net, &nn->nfsd_svcstats, &nfsd_proc_ops); } void nfsd_proc_stat_shutdown(struct net *net) diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h index 04aacb6c36e2576ba231ee481e3a3e9e9f255a61..e4efb0e4e56d467c13eaa5a1dd312c85dadeb4b8 100644 --- a/fs/nfsd/stats.h +++ b/fs/nfsd/stats.h @@ -10,7 +10,7 @@ #include <uapi/linux/nfsd/stats.h> #include <linux/percpu_counter.h> -void nfsd_proc_stat_init(struct net *net); +struct proc_dir_entry *nfsd_proc_stat_init(struct net *net); void nfsd_proc_stat_shutdown(struct net *net); static inline void nfsd_stats_rc_hits_inc(struct nfsd_net *nn)
Currently, nfsd_proc_stat_init() ignores the return value of svc_proc_register(). If the procfile creation fails, then the kernel will WARN when it tries to remove the entry later. Fix nfsd_proc_stat_init() to return the same type of pointer as svc_proc_register(), and fix up nfsd_net_init() to check that and fail the nfsd_net construction if it occurs. svc_proc_register() can fail if the dentry can't be allocated, or if an identical dentry already exists. The second case is pretty unlikely in the nfsd_net construction codepath, so if this happens, return -ENOMEM. Fixes: 93483ac5fec6 ("nfsd: expose /proc/net/sunrpc/nfsd in net namespaces") Reported-by: syzbot+e34ad04f27991521104c@syzkaller.appspotmail.com Closes: https://lore.kernel.org/linux-nfs/67a47501.050a0220.19061f.05f9.GAE@google.com/ Signed-off-by: Jeff Layton <jlayton@kernel.org> --- I looked at the console log from the report, and syzkaller is doing fault injection on allocations. You can see the stack where the "nfsd" directory under /proc failed to be created due to one. This is a pretty unlikely bug under normal circumstances, but it's simple to fix. The problem predates the patch in Fixes:, but it's not worth the effort to backport this to anything earlier. --- fs/nfsd/nfsctl.c | 9 ++++++++- fs/nfsd/stats.c | 4 ++-- fs/nfsd/stats.h | 2 +- 3 files changed, 11 insertions(+), 4 deletions(-) --- base-commit: ebbdc9429c39336a406b191cfe84bca2c12c2f73 change-id: 20250206-nfsd-fixes-8e61bdf66347 Best regards,