Re: [PATCH] kernel/rcutorture.c: use scnprintf() instead of sprintf()

[Paul E. McKenney]

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@xxxxxxxxxxx>

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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/