Here's a possible way to address a recent 0-day report.
Compile-tested only.
---
Chuck Lever (2):
NFSD: Rewrite synopsis of nfsd_percpu_counters_init()
NFSD: Fix frame size warning in svc_export_parse()
fs/nfsd/export.c | 30 +++++++++++++++++++++---------
fs/nfsd/export.h | 4 ++--
fs/nfsd/stats.c | 2 +-
fs/nfsd/stats.h | 18 +++++++++---------
4 files changed, 33 insertions(+), 21 deletions(-)
--
Chuck Lever
From: Chuck Lever <[email protected]>
fs/nfsd/export.c: In function 'svc_export_parse':
fs/nfsd/export.c:737:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
737 | }
On my systems, svc_export_parse() has a stack frame of over 800
bytes, not 1040, but nonetheless, it could do with some reduction.
When a struct svc_export is on the stack, it's a temporary structure
used as an argument, and not visible as an actual exported FS. No
need to reserve space for export_stats in such cases.
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Cc: Amir Goldstein <[email protected]>
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/export.c | 30 +++++++++++++++++++++---------
fs/nfsd/export.h | 4 ++--
fs/nfsd/stats.h | 12 ++++++------
3 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 11a0eaa2f914..7cf26bfe199d 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -339,12 +339,16 @@ static int export_stats_init(struct export_stats *stats)
static void export_stats_reset(struct export_stats *stats)
{
- nfsd_percpu_counters_reset(stats->counter, EXP_STATS_COUNTERS_NUM);
+ if (stats)
+ nfsd_percpu_counters_reset(stats->counter,
+ EXP_STATS_COUNTERS_NUM);
}
static void export_stats_destroy(struct export_stats *stats)
{
- nfsd_percpu_counters_destroy(stats->counter, EXP_STATS_COUNTERS_NUM);
+ if (stats)
+ nfsd_percpu_counters_destroy(stats->counter,
+ EXP_STATS_COUNTERS_NUM);
}
static void svc_export_put(struct kref *ref)
@@ -353,7 +357,8 @@ static void svc_export_put(struct kref *ref)
path_put(&exp->ex_path);
auth_domain_put(exp->ex_client);
nfsd4_fslocs_free(&exp->ex_fslocs);
- export_stats_destroy(&exp->ex_stats);
+ export_stats_destroy(exp->ex_stats);
+ kfree(exp->ex_stats);
kfree(exp->ex_uuid);
kfree_rcu(exp, ex_rcu);
}
@@ -767,13 +772,13 @@ static int svc_export_show(struct seq_file *m,
seq_putc(m, '\t');
seq_escape(m, exp->ex_client->name, " \t\n\\");
if (export_stats) {
- seq_printf(m, "\t%lld\n", exp->ex_stats.start_time);
+ seq_printf(m, "\t%lld\n", exp->ex_stats->start_time);
seq_printf(m, "\tfh_stale: %lld\n",
- percpu_counter_sum_positive(&exp->ex_stats.counter[EXP_STATS_FH_STALE]));
+ percpu_counter_sum_positive(&exp->ex_stats->counter[EXP_STATS_FH_STALE]));
seq_printf(m, "\tio_read: %lld\n",
- percpu_counter_sum_positive(&exp->ex_stats.counter[EXP_STATS_IO_READ]));
+ percpu_counter_sum_positive(&exp->ex_stats->counter[EXP_STATS_IO_READ]));
seq_printf(m, "\tio_write: %lld\n",
- percpu_counter_sum_positive(&exp->ex_stats.counter[EXP_STATS_IO_WRITE]));
+ percpu_counter_sum_positive(&exp->ex_stats->counter[EXP_STATS_IO_WRITE]));
seq_putc(m, '\n');
return 0;
}
@@ -819,7 +824,7 @@ static void svc_export_init(struct cache_head *cnew, struct cache_head *citem)
new->ex_layout_types = 0;
new->ex_uuid = NULL;
new->cd = item->cd;
- export_stats_reset(&new->ex_stats);
+ export_stats_reset(new->ex_stats);
}
static void export_update(struct cache_head *cnew, struct cache_head *citem)
@@ -856,7 +861,14 @@ static struct cache_head *svc_export_alloc(void)
if (!i)
return NULL;
- if (export_stats_init(&i->ex_stats)) {
+ i->ex_stats = kmalloc(sizeof(i->ex_stats), GFP_KERNEL);
+ if (!i->ex_stats) {
+ kfree(i);
+ return NULL;
+ }
+
+ if (export_stats_init(i->ex_stats)) {
+ kfree(i->ex_stats);
kfree(i);
return NULL;
}
diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
index 2df8ae25aad3..ca9dc230ae3d 100644
--- a/fs/nfsd/export.h
+++ b/fs/nfsd/export.h
@@ -64,10 +64,10 @@ struct svc_export {
struct cache_head h;
struct auth_domain * ex_client;
int ex_flags;
+ int ex_fsid;
struct path ex_path;
kuid_t ex_anon_uid;
kgid_t ex_anon_gid;
- int ex_fsid;
unsigned char * ex_uuid; /* 16 byte fsid */
struct nfsd4_fs_locations ex_fslocs;
uint32_t ex_nflavors;
@@ -76,8 +76,8 @@ struct svc_export {
struct nfsd4_deviceid_map *ex_devid_map;
struct cache_detail *cd;
struct rcu_head ex_rcu;
- struct export_stats ex_stats;
unsigned long ex_xprtsec_modes;
+ struct export_stats *ex_stats;
};
/* an "export key" (expkey) maps a filehandlefragement to an
diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h
index a3e9e2f47ec4..14f50c660b61 100644
--- a/fs/nfsd/stats.h
+++ b/fs/nfsd/stats.h
@@ -61,22 +61,22 @@ static inline void nfsd_stats_rc_nocache_inc(void)
static inline void nfsd_stats_fh_stale_inc(struct svc_export *exp)
{
percpu_counter_inc(&nfsdstats.counter[NFSD_STATS_FH_STALE]);
- if (exp)
- percpu_counter_inc(&exp->ex_stats.counter[EXP_STATS_FH_STALE]);
+ if (exp && exp->ex_stats)
+ percpu_counter_inc(&exp->ex_stats->counter[EXP_STATS_FH_STALE]);
}
static inline void nfsd_stats_io_read_add(struct svc_export *exp, s64 amount)
{
percpu_counter_add(&nfsdstats.counter[NFSD_STATS_IO_READ], amount);
- if (exp)
- percpu_counter_add(&exp->ex_stats.counter[EXP_STATS_IO_READ], amount);
+ if (exp && exp->ex_stats)
+ percpu_counter_add(&exp->ex_stats->counter[EXP_STATS_IO_READ], amount);
}
static inline void nfsd_stats_io_write_add(struct svc_export *exp, s64 amount)
{
percpu_counter_add(&nfsdstats.counter[NFSD_STATS_IO_WRITE], amount);
- if (exp)
- percpu_counter_add(&exp->ex_stats.counter[EXP_STATS_IO_WRITE], amount);
+ if (exp && exp->ex_stats)
+ percpu_counter_add(&exp->ex_stats->counter[EXP_STATS_IO_WRITE], amount);
}
static inline void nfsd_stats_payload_misses_inc(struct nfsd_net *nn)
On Sun, 2023-10-01 at 13:25 -0400, Chuck Lever wrote:
> From: Chuck Lever <[email protected]>
>
> fs/nfsd/export.c: In function 'svc_export_parse':
> fs/nfsd/export.c:737:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 737 | }
>
> On my systems, svc_export_parse() has a stack frame of over 800
> bytes, not 1040, but nonetheless, it could do with some reduction.
>
> When a struct svc_export is on the stack, it's a temporary structure
> used as an argument, and not visible as an actual exported FS. No
> need to reserve space for export_stats in such cases.
>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Cc: Amir Goldstein <[email protected]>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> fs/nfsd/export.c | 30 +++++++++++++++++++++---------
> fs/nfsd/export.h | 4 ++--
> fs/nfsd/stats.h | 12 ++++++------
> 3 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 11a0eaa2f914..7cf26bfe199d 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -339,12 +339,16 @@ static int export_stats_init(struct export_stats *stats)
>
> static void export_stats_reset(struct export_stats *stats)
> {
> - nfsd_percpu_counters_reset(stats->counter, EXP_STATS_COUNTERS_NUM);
> + if (stats)
> + nfsd_percpu_counters_reset(stats->counter,
> + EXP_STATS_COUNTERS_NUM);
> }
>
> static void export_stats_destroy(struct export_stats *stats)
> {
> - nfsd_percpu_counters_destroy(stats->counter, EXP_STATS_COUNTERS_NUM);
> + if (stats)
> + nfsd_percpu_counters_destroy(stats->counter,
> + EXP_STATS_COUNTERS_NUM);
> }
>
> static void svc_export_put(struct kref *ref)
> @@ -353,7 +357,8 @@ static void svc_export_put(struct kref *ref)
> path_put(&exp->ex_path);
> auth_domain_put(exp->ex_client);
> nfsd4_fslocs_free(&exp->ex_fslocs);
> - export_stats_destroy(&exp->ex_stats);
> + export_stats_destroy(exp->ex_stats);
> + kfree(exp->ex_stats);
> kfree(exp->ex_uuid);
> kfree_rcu(exp, ex_rcu);
> }
> @@ -767,13 +772,13 @@ static int svc_export_show(struct seq_file *m,
> seq_putc(m, '\t');
> seq_escape(m, exp->ex_client->name, " \t\n\\");
> if (export_stats) {
> - seq_printf(m, "\t%lld\n", exp->ex_stats.start_time);
> + seq_printf(m, "\t%lld\n", exp->ex_stats->start_time);
> seq_printf(m, "\tfh_stale: %lld\n",
> - percpu_counter_sum_positive(&exp->ex_stats.counter[EXP_STATS_FH_STALE]));
> + percpu_counter_sum_positive(&exp->ex_stats->counter[EXP_STATS_FH_STALE]));
> seq_printf(m, "\tio_read: %lld\n",
> - percpu_counter_sum_positive(&exp->ex_stats.counter[EXP_STATS_IO_READ]));
> + percpu_counter_sum_positive(&exp->ex_stats->counter[EXP_STATS_IO_READ]));
> seq_printf(m, "\tio_write: %lld\n",
> - percpu_counter_sum_positive(&exp->ex_stats.counter[EXP_STATS_IO_WRITE]));
> + percpu_counter_sum_positive(&exp->ex_stats->counter[EXP_STATS_IO_WRITE]));
> seq_putc(m, '\n');
> return 0;
> }
> @@ -819,7 +824,7 @@ static void svc_export_init(struct cache_head *cnew, struct cache_head *citem)
> new->ex_layout_types = 0;
> new->ex_uuid = NULL;
> new->cd = item->cd;
> - export_stats_reset(&new->ex_stats);
> + export_stats_reset(new->ex_stats);
> }
>
> static void export_update(struct cache_head *cnew, struct cache_head *citem)
> @@ -856,7 +861,14 @@ static struct cache_head *svc_export_alloc(void)
> if (!i)
> return NULL;
>
> - if (export_stats_init(&i->ex_stats)) {
> + i->ex_stats = kmalloc(sizeof(i->ex_stats), GFP_KERNEL);
> + if (!i->ex_stats) {
> + kfree(i);
> + return NULL;
> + }
> +
> + if (export_stats_init(i->ex_stats)) {
> + kfree(i->ex_stats);
> kfree(i);
> return NULL;
> }
> diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
> index 2df8ae25aad3..ca9dc230ae3d 100644
> --- a/fs/nfsd/export.h
> +++ b/fs/nfsd/export.h
> @@ -64,10 +64,10 @@ struct svc_export {
> struct cache_head h;
> struct auth_domain * ex_client;
> int ex_flags;
> + int ex_fsid;
> struct path ex_path;
> kuid_t ex_anon_uid;
> kgid_t ex_anon_gid;
> - int ex_fsid;
> unsigned char * ex_uuid; /* 16 byte fsid */
> struct nfsd4_fs_locations ex_fslocs;
> uint32_t ex_nflavors;
> @@ -76,8 +76,8 @@ struct svc_export {
> struct nfsd4_deviceid_map *ex_devid_map;
> struct cache_detail *cd;
> struct rcu_head ex_rcu;
> - struct export_stats ex_stats;
> unsigned long ex_xprtsec_modes;
> + struct export_stats *ex_stats;
> };
>
> /* an "export key" (expkey) maps a filehandlefragement to an
> diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h
> index a3e9e2f47ec4..14f50c660b61 100644
> --- a/fs/nfsd/stats.h
> +++ b/fs/nfsd/stats.h
> @@ -61,22 +61,22 @@ static inline void nfsd_stats_rc_nocache_inc(void)
> static inline void nfsd_stats_fh_stale_inc(struct svc_export *exp)
> {
> percpu_counter_inc(&nfsdstats.counter[NFSD_STATS_FH_STALE]);
> - if (exp)
> - percpu_counter_inc(&exp->ex_stats.counter[EXP_STATS_FH_STALE]);
> + if (exp && exp->ex_stats)
> + percpu_counter_inc(&exp->ex_stats->counter[EXP_STATS_FH_STALE]);
> }
>
> static inline void nfsd_stats_io_read_add(struct svc_export *exp, s64 amount)
> {
> percpu_counter_add(&nfsdstats.counter[NFSD_STATS_IO_READ], amount);
> - if (exp)
> - percpu_counter_add(&exp->ex_stats.counter[EXP_STATS_IO_READ], amount);
> + if (exp && exp->ex_stats)
> + percpu_counter_add(&exp->ex_stats->counter[EXP_STATS_IO_READ], amount);
> }
>
> static inline void nfsd_stats_io_write_add(struct svc_export *exp, s64 amount)
> {
> percpu_counter_add(&nfsdstats.counter[NFSD_STATS_IO_WRITE], amount);
> - if (exp)
> - percpu_counter_add(&exp->ex_stats.counter[EXP_STATS_IO_WRITE], amount);
> + if (exp && exp->ex_stats)
> + percpu_counter_add(&exp->ex_stats->counter[EXP_STATS_IO_WRITE], amount);
> }
>
> static inline void nfsd_stats_payload_misses_inc(struct nfsd_net *nn)
>
>
Seems reasonable.
Reviewed-by: Jeff Layton <[email protected]>