From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Chen Gang <gang.chen@asianux.com>
Cc: josh@freedesktop.org,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] kernel/rcutorture.c: use scnprintf() instead of sprintf()
Date: Mon, 14 Oct 2013 04:28:39 -0700 [thread overview]
Message-ID: <20131014112839.GO5790@linux.vnet.ibm.com> (raw)
In-Reply-To: <525BAD9F.6060406@asianux.com>
On Mon, Oct 14, 2013 at 04:38:55PM +0800, Chen Gang wrote:
> If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
> sprintf() will cause memory overflow.
>
> They are all test information which can be truncated, so use scnprintf()
> instead of sprintf(), also add 'max' parameter for related functions,
> also notice 80 columns boundary and parameters alignments.
>
> Test case:
>
> Fedora16 x86_64, 2 CPUs, 2GB RAM, [in/rm]mod with "torture_type=srcu".
>
> let maximize buffer to 256 to truncate in rcu_torture_printk().
> let maximize buffer to 410 to may truncate in srcu_torture_stats().
> let maximize buffer to 4096 (original size) to print full.
>
> it is a rcu test module, so not need additional test or consideration.
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
At first glance, not a bad start.
Suppose that your goal was to make it avoid truncation. What would you
do differently?
Thanx, Paul
> ---
> kernel/rcutorture.c | 110 +++++++++++++++++++++++++++-----------------------
> 1 files changed, 59 insertions(+), 51 deletions(-)
>
> diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
> index be63101..107fd76 100644
> --- a/kernel/rcutorture.c
> +++ b/kernel/rcutorture.c
> @@ -370,7 +370,7 @@ struct rcu_torture_ops {
> void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
> void (*cb_barrier)(void);
> void (*fqs)(void);
> - int (*stats)(char *page);
> + int (*stats)(char *page, int max);
> int irq_capable;
> int can_boost;
> const char *name;
> @@ -572,20 +572,20 @@ static void srcu_torture_barrier(void)
> srcu_barrier(&srcu_ctl);
> }
>
> -static int srcu_torture_stats(char *page)
> +static int srcu_torture_stats(char *page, int max)
> {
> int cnt = 0;
> int cpu;
> int idx = srcu_ctl.completed & 0x1;
>
> - cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
> - torture_type, TORTURE_FLAG, idx);
> + cnt += scnprintf(&page[cnt], max - cnt, "%s%s per-CPU(idx=%d):",
> + torture_type, TORTURE_FLAG, idx);
> for_each_possible_cpu(cpu) {
> - cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
> - per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
> - per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
> + cnt += scnprintf(&page[cnt], max - cnt, " %d(%lu,%lu)", cpu,
> + per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
> + per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
> }
> - cnt += sprintf(&page[cnt], "\n");
> + cnt += scnprintf(&page[cnt], max - cnt, "\n");
> return cnt;
> }
>
> @@ -1047,7 +1047,7 @@ rcu_torture_reader(void *arg)
> * Create an RCU-torture statistics message in the specified buffer.
> */
> static int
> -rcu_torture_printk(char *page)
> +rcu_torture_printk(char *page, int max)
> {
> int cnt = 0;
> int cpu;
> @@ -1065,61 +1065,69 @@ rcu_torture_printk(char *page)
> if (pipesummary[i] != 0)
> break;
> }
> - cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
> - cnt += sprintf(&page[cnt],
> - "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
> - rcu_torture_current,
> - rcu_torture_current_version,
> - list_empty(&rcu_torture_freelist),
> - atomic_read(&n_rcu_torture_alloc),
> - atomic_read(&n_rcu_torture_alloc_fail),
> - atomic_read(&n_rcu_torture_free));
> - cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
> - atomic_read(&n_rcu_torture_mberror),
> - n_rcu_torture_boost_ktrerror,
> - n_rcu_torture_boost_rterror);
> - cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
> - n_rcu_torture_boost_failure,
> - n_rcu_torture_boosts,
> - n_rcu_torture_timers);
> - cnt += sprintf(&page[cnt],
> - "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
> - n_online_successes, n_online_attempts,
> - n_offline_successes, n_offline_attempts,
> - min_online, max_online,
> - min_offline, max_offline,
> - sum_online, sum_offline, HZ);
> - cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
> - n_barrier_successes,
> - n_barrier_attempts,
> - n_rcu_torture_barrier_error);
> - cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> + cnt += scnprintf(&page[cnt], max - cnt, "%s%s ",
> + torture_type, TORTURE_FLAG);
> + cnt += scnprintf(&page[cnt], max - cnt,
> + "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
> + rcu_torture_current,
> + rcu_torture_current_version,
> + list_empty(&rcu_torture_freelist),
> + atomic_read(&n_rcu_torture_alloc),
> + atomic_read(&n_rcu_torture_alloc_fail),
> + atomic_read(&n_rcu_torture_free));
> + cnt += scnprintf(&page[cnt], max - cnt,
> + "rtmbe: %d rtbke: %ld rtbre: %ld ",
> + atomic_read(&n_rcu_torture_mberror),
> + n_rcu_torture_boost_ktrerror,
> + n_rcu_torture_boost_rterror);
> + cnt += scnprintf(&page[cnt], max - cnt,
> + "rtbf: %ld rtb: %ld nt: %ld ",
> + n_rcu_torture_boost_failure,
> + n_rcu_torture_boosts,
> + n_rcu_torture_timers);
> + cnt += scnprintf(&page[cnt], max - cnt,
> + "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
> + n_online_successes, n_online_attempts,
> + n_offline_successes, n_offline_attempts,
> + min_online, max_online,
> + min_offline, max_offline,
> + sum_online, sum_offline, HZ);
> + cnt += scnprintf(&page[cnt], max - cnt,
> + "barrier: %ld/%ld:%ld",
> + n_barrier_successes,
> + n_barrier_attempts,
> + n_rcu_torture_barrier_error);
> + cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
> + torture_type, TORTURE_FLAG);
> if (atomic_read(&n_rcu_torture_mberror) != 0 ||
> n_rcu_torture_barrier_error != 0 ||
> n_rcu_torture_boost_ktrerror != 0 ||
> n_rcu_torture_boost_rterror != 0 ||
> n_rcu_torture_boost_failure != 0 ||
> i > 1) {
> - cnt += sprintf(&page[cnt], "!!! ");
> + cnt += scnprintf(&page[cnt], max - cnt, "!!! ");
> atomic_inc(&n_rcu_torture_error);
> WARN_ON_ONCE(1);
> }
> - cnt += sprintf(&page[cnt], "Reader Pipe: ");
> + cnt += scnprintf(&page[cnt], max - cnt, "Reader Pipe: ");
> for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> - cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
> - cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> - cnt += sprintf(&page[cnt], "Reader Batch: ");
> + cnt += scnprintf(&page[cnt], max - cnt, " %ld", pipesummary[i]);
> + cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
> + torture_type, TORTURE_FLAG);
> + cnt += scnprintf(&page[cnt], max - cnt, "Reader Batch: ");
> for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> - cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
> - cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> - cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
> + cnt += scnprintf(&page[cnt], max - cnt, " %ld",
> + batchsummary[i]);
> + cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
> + torture_type, TORTURE_FLAG);
> + cnt += scnprintf(&page[cnt], max - cnt, "Free-Block Circulation: ");
> for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
> - cnt += sprintf(&page[cnt], " %d",
> - atomic_read(&rcu_torture_wcount[i]));
> + cnt += scnprintf(&page[cnt], max - cnt, " %d",
> + atomic_read(&rcu_torture_wcount[i]));
> }
> - cnt += sprintf(&page[cnt], "\n");
> + cnt += scnprintf(&page[cnt], max - cnt, "\n");
> if (cur_ops->stats)
> - cnt += cur_ops->stats(&page[cnt]);
> + cnt += cur_ops->stats(&page[cnt], max - cnt);
> return cnt;
> }
>
> @@ -1136,7 +1144,7 @@ rcu_torture_stats_print(void)
> {
> int cnt;
>
> - cnt = rcu_torture_printk(printk_buf);
> + cnt = rcu_torture_printk(printk_buf, sizeof(printk_buf));
> pr_alert("%s", printk_buf);
> }
>
> --
> 1.7.7.6
>
next prev parent reply other threads:[~2013-10-14 11:28 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-08 8:32 [Suggestion] kernel/rcutorture.c: about using scnprintf() instead of sprintf() Chen Gang
2013-10-13 11:05 ` Paul E. McKenney
2013-10-14 1:41 ` Chen Gang
2013-10-14 2:22 ` Chen Gang
2013-10-14 11:24 ` Paul E. McKenney
2013-10-15 1:40 ` Chen Gang
2013-10-15 8:31 ` Paul E. McKenney
2013-10-15 9:03 ` Chen Gang
2013-10-14 8:38 ` [PATCH] kernel/rcutorture.c: use " Chen Gang
2013-10-14 11:28 ` Paul E. McKenney [this message]
2013-10-15 0:54 ` Chen Gang
2013-10-15 1:51 ` Chen Gang
2013-10-15 8:26 ` Paul E. McKenney
2013-10-15 12:32 ` Chen Gang
2013-10-15 14:47 ` Paul E. McKenney
2013-10-16 2:07 ` Chen Gang
2013-10-17 1:06 ` Chen Gang
2013-10-21 5:51 ` [PATCH] kernel/rcutorture.c: be sure of enough memory for result printing Chen Gang
2013-10-21 6:18 ` Chen Gang
2013-10-21 9:35 ` Chen Gang
2013-10-27 14:43 ` Chen Gang
2013-11-04 9:42 ` Chen Gang
2013-11-06 20:38 ` Paul E. McKenney
2013-11-07 2:30 ` [PATCH v2] " Chen Gang
2013-11-07 20:59 ` Paul E. McKenney
2013-11-08 0:58 ` Chen Gang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131014112839.GO5790@linux.vnet.ibm.com \
--to=paulmck@linux.vnet.ibm.com \
--cc=gang.chen@asianux.com \
--cc=josh@freedesktop.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).