Re: [PATCH] bcache: fix bch_hprint crash and improve output

[Coly Li <colyli@suse.de>]

On 2017/9/5 上午5:55, Michael Lyle wrote:
> Most importantly, solve a crash where %llu was used to format signed
> numbers.  This would cause a buffer overflow when reading sysfs
> writeback_rate_debug, as only 20 bytes were allocated for this and
> %llu writes 20 characters plus a null.
> 
> Always use the units mechanism rather than having different output
> paths for simplicity.
> 
> Also, correct problems with display output where 1.10 was a larger
> number than 1.09, by multiplying by 10 and then dividing by 1024 instead
> of dividing by 100.  (Remainders of >= 1000 would print as .10).
> 
> Minor changes: Always display the decimal point instead of trying to
> omit it based on number of digits shown.  Decide what units to use
> based on 1000 as a threshold, not 1024 (in other words, always print
> at most 3 digits before the decimal point).
> 
> Signed-off-by: Michael Lyle <mlyle@lyle.org>
> Reported-by: Dmitry Yu Okunev <dyokunev@ut.mephi.ru>
> ---
>  drivers/md/bcache/util.c | 50 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
> index 8c3a938f4bf0..176d3c2ef5f5 100644
> --- a/drivers/md/bcache/util.c
> +++ b/drivers/md/bcache/util.c
> @@ -74,24 +74,44 @@ STRTO_H(strtouint, unsigned int)
>  STRTO_H(strtoll, long long)
>  STRTO_H(strtoull, unsigned long long)
>  
> +/**
> + * bch_hprint() - formats @v to human readable string for sysfs.
> + *
> + * @v - signed 64 bit integer
> + * @buf - the (at least 8 byte) buffer to format the result into.
> + *
> + * Returns the number of bytes used by format.
> + */
>  ssize_t bch_hprint(char *buf, int64_t v)
>  {
>  	static const char units[] = "?kMGTPEZY";
> -	char dec[4] = "";
> -	int u, t = 0;
> -
> -	for (u = 0; v >= 1024 || v <= -1024; u++) {
> -		t = v & ~(~0 << 10);
> -		v >>= 10;
> -	}
> -
> -	if (!u)
> -		return sprintf(buf, "%llu", v);
> -
> -	if (v < 100 && v > -100)
> -		snprintf(dec, sizeof(dec), ".%i", t / 100);
> -
> -	return sprintf(buf, "%lli%s%c", v, dec, units[u]);
> +	int u = 0, t;
> +
> +	uint64_t q;

It would be good to remove a blank line between the variables.

> +
> +	if (v < 0)
> +		q = -v;
> +	else
> +		q = v;
> +
> +	/* For as long as the number is more than 3 digits, but at least
> +	 * once, shift right / divide by 1024.  Keep the remainder for
> +	 * a digit after the decimal point.
> +	 */
> +	do {
> +		u++;
> +
> +		t = q & ~(~0 << 10);
> +		q >>= 10;
> +	} while (q >= 1000);
> +

The original for-loop is correct, and the above do-while loop is
probably wrong. If q < 1024, a number without K/M/G/T/P/E/Z/Y should be
printed out, in this patch it is missing. And while (q>=1000) is not
correct, it should be (q>=1024), because >>10 means write shifting 10
bits, which is 1024 in decimal integer. How about just keep the
following original code,
	for (u = 0; v >= 1024 || v <= -1024; u++) {
		t = v & ~(~0 << 10);
		v >>= 10;
	}

	if (!u)
		return sprintf(buf, "%llu", v);
It is good enough IMHO.



> +	if (v < 0)
> +		/* '-', up to 3 digits, '.', 1 digit, 1 character, null;
> +		 * yields 8 bytes.
> +		 */
> +		return sprintf(buf, "-%llu.%i%c", q, t * 10 / 1024, units[u]);
> +	else
> +		return sprintf(buf, "%llu.%i%c", q, t * 10 / 1024, units[u]);
>  }

If you use char sign[2], that's two bytes temporary space on stack. Here
you use "-%llu.%i%c" and "%llu.%i%c" which occupy permanent space in
kernel readonly data section, which means 9 bytes more. That's not a big
memory consumption, but we can avoid it, why not.

The logic in this patch is much clear, and thanks for your detailed
commit log and patch comments. Could you please compose another version ?

Coly Li