[PATCH] bcache: fix bch_hprint crash and improve output

[PATCH] bcache: fix bch_hprint crash and improve output

[Michael Lyle]

Solve a crash where the stack is overrun when reading sysfs and
small negative values are present.  Also correct output, as before
it would output "1.10" for numbers bigger than "1.9", and negative
fractional output was incorrect.

Signed-off-by: Michael Lyle <mlyle@lyle.org>
Reported-by: Dmitry Yu Okunev <dyokunev@ut.mephi.ru>
---
 drivers/md/bcache/util.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
index 8c3a938f4bf0..11957038c630 100644
--- a/drivers/md/bcache/util.c
+++ b/drivers/md/bcache/util.c
@@ -86,10 +86,14 @@ ssize_t bch_hprint(char *buf, int64_t v)
 	}
 
 	if (!u)
-		return sprintf(buf, "%llu", v);
+		return sprintf(buf, "%lli", v);
 
-	if (v < 100 && v > -100)
-		snprintf(dec, sizeof(dec), ".%i", t / 100);
+	if (t > 103) {
+		if (v > 0)
+			snprintf(dec, sizeof(dec), ".%i", t * 10 / 1024);
+		else
+			snprintf(dec, sizeof(dec), ".%i", 10 - (t * 10 / 1024));
+	}
 
 	return sprintf(buf, "%lli%s%c", v, dec, units[u]);
 }
-- 
2.11.0

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

[Coly Li]

On 2017/9/2 上午4:37, Michael Lyle wrote:
> Solve a crash where the stack is overrun when reading sysfs and
> small negative values are present.  Also correct output, as before
> it would output "1.10" for numbers bigger than "1.9", and negative
> fractional output was incorrect.
> 

I suggest to provide more detailed comments here, to explain,
- the purpose and expected output of bch_hprint()
- when a small negative value comes, how bhc_hprint() goes wrong
- how your patch fixes the root cause.

The reason for my suggestion is, the original code is ambiguous on
positive and negative integer values. When you see the following
for-loop in bch_hprint(),
 83         for (u = 0; v >= 1024 || v <= -1024; u++) {
 84                 t = v & ~(~0 << 10);
 85                 v >>= 10;
 86         }
You may notice no matter v is positive or negative, variables 't' and
'v' are handled with the bit operations which taking for granted that
'v' is always positive integer values.

I don't have very deep insight of coding theory, my intuition of a first
glance on the code gives me a warning: the code might be buggy.
Example an integer N, most of time, -N and +N has different ordering in
non most-significant bits. The original code neglects when v is negative
value, v is in form of complement code, so value t here is not correct,
then the following calculation in wrong,
 91         if (v < 100 && v > -100)
 92                 snprintf(dec, sizeof(dec), ".%i", t / 100);

I see your patch fixes the above two lines, but your patch is not simply
understandable for me, e.g. why you use 't > 103' (I guess it is because
103*10 > 1024).

A fix much simpler in logic maybe look like this, which is enlighten by
your patch,

diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
index 8c3a938f4bf0..bf1dbb9c3ea8 100644
--- a/drivers/md/bcache/util.c
+++ b/drivers/md/bcache/util.c
@@ -78,20 +78,26 @@ ssize_t bch_hprint(char *buf, int64_t v)
 {
        static const char units[] = "?kMGTPEZY";
        char dec[4] = "";
+       char sign[2] = "";
        int u, t = 0;

-       for (u = 0; v >= 1024 || v <= -1024; u++) {
+       if (v < 0) {
+               sign[0] = '-';
+               v = -v;
+       }
+
+       for (u = 0; v >= 1024; u++) {
                t = v & ~(~0 << 10);
                v >>= 10;
        }

        if (!u)
-               return sprintf(buf, "%llu", v);
+               return sprintf(buf, "%s%lli", sign, v);

-       if (v < 100 && v > -100)
-               snprintf(dec, sizeof(dec), ".%i", t / 100);
+       if (t > 0)
+               snprintf(dec, sizeof(dec), ".%i", (t*10)/1024);

-       return sprintf(buf, "%lli%s%c", v, dec, units[u]);
+       return sprintf(buf, "%s%lli%s%c", sign, v, dec, units[u]);
 }

This is just an idea in my brain which is enlighten by your patch, not
correct maybe, just for your reference. I do like the idea "t * 10 /
1024", I have to say I feel happiness when I understand it, very smart :-)

Finally please offer a more detailed comments as I suggested, thank you
in advance.

Coly Li


> Signed-off-by: Michael Lyle <mlyle@lyle.org>
> Reported-by: Dmitry Yu Okunev <dyokunev@ut.mephi.ru>
> ---
>  drivers/md/bcache/util.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
> index 8c3a938f4bf0..11957038c630 100644
> --- a/drivers/md/bcache/util.c
> +++ b/drivers/md/bcache/util.c
> @@ -86,10 +86,14 @@ ssize_t bch_hprint(char *buf, int64_t v)
>  	}
>  
>  	if (!u)
> -		return sprintf(buf, "%llu", v);
> +		return sprintf(buf, "%lli", v);
>  
> -	if (v < 100 && v > -100)
> -		snprintf(dec, sizeof(dec), ".%i", t / 100);
> +	if (t > 103) {
> +		if (v > 0)
> +			snprintf(dec, sizeof(dec), ".%i", t * 10 / 1024);
> +		else
> +			snprintf(dec, sizeof(dec), ".%i", 10 - (t * 10 / 1024));
> +	}
>  
>  	return sprintf(buf, "%lli%s%c", v, dec, units[u]);
>  }
> 

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

[Michael Lyle]

[I'm sorry for the resend-- seems I can't send plain text mode from
inbox so sent HTML mail on accident]

OK, I don't like the formulation of the code either-- I will
restructure a bit to be clearer in a bit bigger of a patch today or
tomorrow.

The *critical* issue is this-- if v is between -1023 and -1, the line
return sprintf(buf, "%llu", v); is run. In turn, this prints out a 20
digit number plus terminating null. In turn, bch_hprint is called
places where it is provided with a 20 character buffer, which overruns
and causes a kernel panic. This doesn't require any privilege to
trigger, so it's a user-space denial of service vulnerability.

I discovered this when running watch cat writeback_rate_debug to
troubleshoot the control system (which I am tempted to rewrite,
because it has various issues) and I encountered multiple panics.

Mike

On Sun, Sep 3, 2017 at 11:07 PM Coly Li <colyli@suse.de> wrote:

On 2017/9/2 上午4:37, Michael Lyle wrote:
> Solve a crash where the stack is overrun when reading sysfs and
> small negative values are present. Also correct output, as before
> it would output "1.10" for numbers bigger than "1.9", and negative
> fractional output was incorrect.
>

I suggest to provide more detailed comments here, to explain,
- the purpose and expected output of bch_hprint()
- when a small negative value comes, how bhc_hprint() goes wrong
- how your patch fixes the root cause.

The reason for my suggestion is, the original code is ambiguous on
positive and negative integer values. When you see the following
for-loop in bch_hprint(),
83 for (u = 0; v >= 1024 || v <= -1024; u++) {
84 t = v & ~(~0 << 10);
85 v >>= 10;
86 }
You may notice no matter v is positive or negative, variables 't' and
'v' are handled with the bit operations which taking for granted that
'v' is always positive integer values.

I don't have very deep insight of coding theory, my intuition of a first
glance on the code gives me a warning: the code might be buggy.
Example an integer N, most of time, -N and +N has different ordering in
non most-significant bits. The original code neglects when v is negative
value, v is in form of complement code, so value t here is not correct,
then the following calculation in wrong,
91 if (v < 100 && v > -100)
92 snprintf(dec, sizeof(dec), ".%i", t / 100);

I see your patch fixes the above two lines, but your patch is not simply
understandable for me, e.g. why you use 't > 103' (I guess it is because
103*10 > 1024).

A fix much simpler in logic maybe look like this, which is enlighten by
your patch,

diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
index 8c3a938f4bf0..bf1dbb9c3ea8 100644
--- a/drivers/md/bcache/util.c
+++ b/drivers/md/bcache/util.c
@@ -78,20 +78,26 @@ ssize_t bch_hprint(char *buf, int64_t v)
{
static const char units[] = "?kMGTPEZY";
char dec[4] = "";
+ char sign[2] = "";
int u, t = 0;

- for (u = 0; v >= 1024 || v <= -1024; u++) {
+ if (v < 0) {
+ sign[0] = '-';
+ v = -v;
+ }
+
+ for (u = 0; v >= 1024; u++) {
t = v & ~(~0 << 10);
v >>= 10;
}

if (!u)
- return sprintf(buf, "%llu", v);
+ return sprintf(buf, "%s%lli", sign, v);

- if (v < 100 && v > -100)
- snprintf(dec, sizeof(dec), ".%i", t / 100);
+ if (t > 0)
+ snprintf(dec, sizeof(dec), ".%i", (t*10)/1024);

- return sprintf(buf, "%lli%s%c", v, dec, units[u]);
+ return sprintf(buf, "%s%lli%s%c", sign, v, dec, units[u]);
}

This is just an idea in my brain which is enlighten by your patch, not
correct maybe, just for your reference. I do like the idea "t * 10 /
1024", I have to say I feel happiness when I understand it, very smart :-)

Finally please offer a more detailed comments as I suggested, thank you
in advance.

Coly Li


> Signed-off-by: Michael Lyle <mlyle@lyle.org>
> Reported-by: Dmitry Yu Okunev <dyokunev@ut.mephi.ru>
> ---
> drivers/md/bcache/util.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
> index 8c3a938f4bf0..11957038c630 100644
> --- a/drivers/md/bcache/util.c
> +++ b/drivers/md/bcache/util.c
> @@ -86,10 +86,14 @@ ssize_t bch_hprint(char *buf, int64_t v)
> }
>
> if (!u)
> - return sprintf(buf, "%llu", v);
> + return sprintf(buf, "%lli", v);
>
> - if (v < 100 && v > -100)
> - snprintf(dec, sizeof(dec), ".%i", t / 100);
> + if (t > 103) {
> + if (v > 0)
> + snprintf(dec, sizeof(dec), ".%i", t * 10 / 1024);
> + else
> + snprintf(dec, sizeof(dec), ".%i", 10 - (t * 10 / 1024));
> + }
>
> return sprintf(buf, "%lli%s%c", v, dec, units[u]);
> }
>

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

[Coly Li]

On 2017/9/5 上午12:31, Michael Lyle wrote:
> [I'm sorry for the resend-- seems I can't send plain text mode from
> inbox so sent HTML mail on accident]
> 
> OK, I don't like the formulation of the code either-- I will
> restructure a bit to be clearer in a bit bigger of a patch today or
> tomorrow.
> 

Yes, using "v = -v" for negative numbers, then you don't need to worry
how bits are ordered in memory. Handling complemented code with bit
operations is quite easy to introduce bug(s).

> The *critical* issue is this-- if v is between -1023 and -1, the line
> return sprintf(buf, "%llu", v); is run. In turn, this prints out a 20
> digit number plus terminating null. In turn, bch_hprint is called
> places where it is provided with a 20 character buffer, which overruns
> and causes a kernel panic. This doesn't require any privilege to
> trigger, so it's a user-space denial of service vulnerability.
> 

Oh, I see why you call it a stack overflow. Yes, this fix is critical,
and sounds important. Thanks for catching and fixing it.

> I discovered this when running watch cat writeback_rate_debug to
> troubleshoot the control system (which I am tempted to rewrite,
> because it has various issues) and I encountered multiple panics.
> 

Wow, I will review your next version patch once you send out. Do not
forget a more detailed patch comments, please :-)

Coly Li

> Mike
> 
> On Sun, Sep 3, 2017 at 11:07 PM Coly Li <colyli@suse.de> wrote:
> 
> On 2017/9/2 上午4:37, Michael Lyle wrote:
>> Solve a crash where the stack is overrun when reading sysfs and
>> small negative values are present. Also correct output, as before
>> it would output "1.10" for numbers bigger than "1.9", and negative
>> fractional output was incorrect.
>>
> 
> I suggest to provide more detailed comments here, to explain,
> - the purpose and expected output of bch_hprint()
> - when a small negative value comes, how bhc_hprint() goes wrong
> - how your patch fixes the root cause.
> 
> The reason for my suggestion is, the original code is ambiguous on
> positive and negative integer values. When you see the following
> for-loop in bch_hprint(),
> 83 for (u = 0; v >= 1024 || v <= -1024; u++) {
> 84 t = v & ~(~0 << 10);
> 85 v >>= 10;
> 86 }
> You may notice no matter v is positive or negative, variables 't' and
> 'v' are handled with the bit operations which taking for granted that
> 'v' is always positive integer values.
> 
> I don't have very deep insight of coding theory, my intuition of a first
> glance on the code gives me a warning: the code might be buggy.
> Example an integer N, most of time, -N and +N has different ordering in
> non most-significant bits. The original code neglects when v is negative
> value, v is in form of complement code, so value t here is not correct,
> then the following calculation in wrong,
> 91 if (v < 100 && v > -100)
> 92 snprintf(dec, sizeof(dec), ".%i", t / 100);
> 
> I see your patch fixes the above two lines, but your patch is not simply
> understandable for me, e.g. why you use 't > 103' (I guess it is because
> 103*10 > 1024).
> 
> A fix much simpler in logic maybe look like this, which is enlighten by
> your patch,
> 
> diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
> index 8c3a938f4bf0..bf1dbb9c3ea8 100644
> --- a/drivers/md/bcache/util.c
> +++ b/drivers/md/bcache/util.c
> @@ -78,20 +78,26 @@ ssize_t bch_hprint(char *buf, int64_t v)
> {
> static const char units[] = "?kMGTPEZY";
> char dec[4] = "";
> + char sign[2] = "";
> int u, t = 0;
> 
> - for (u = 0; v >= 1024 || v <= -1024; u++) {
> + if (v < 0) {
> + sign[0] = '-';
> + v = -v;
> + }
> +
> + for (u = 0; v >= 1024; u++) {
> t = v & ~(~0 << 10);
> v >>= 10;
> }
> 
> if (!u)
> - return sprintf(buf, "%llu", v);
> + return sprintf(buf, "%s%lli", sign, v);
> 
> - if (v < 100 && v > -100)
> - snprintf(dec, sizeof(dec), ".%i", t / 100);
> + if (t > 0)
> + snprintf(dec, sizeof(dec), ".%i", (t*10)/1024);
> 
> - return sprintf(buf, "%lli%s%c", v, dec, units[u]);
> + return sprintf(buf, "%s%lli%s%c", sign, v, dec, units[u]);
> }
> 
> This is just an idea in my brain which is enlighten by your patch, not
> correct maybe, just for your reference. I do like the idea "t * 10 /
> 1024", I have to say I feel happiness when I understand it, very smart :-)
> 
> Finally please offer a more detailed comments as I suggested, thank you
> in advance.
> 
> Coly Li
> 
> 
>> Signed-off-by: Michael Lyle <mlyle@lyle.org>
>> Reported-by: Dmitry Yu Okunev <dyokunev@ut.mephi.ru>
>> ---
>> drivers/md/bcache/util.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
>> index 8c3a938f4bf0..11957038c630 100644
>> --- a/drivers/md/bcache/util.c
>> +++ b/drivers/md/bcache/util.c
>> @@ -86,10 +86,14 @@ ssize_t bch_hprint(char *buf, int64_t v)
>> }
>>
>> if (!u)
>> - return sprintf(buf, "%llu", v);
>> + return sprintf(buf, "%lli", v);
>>
>> - if (v < 100 && v > -100)
>> - snprintf(dec, sizeof(dec), ".%i", t / 100);
>> + if (t > 103) {
>> + if (v > 0)
>> + snprintf(dec, sizeof(dec), ".%i", t * 10 / 1024);
>> + else
>> + snprintf(dec, sizeof(dec), ".%i", 10 - (t * 10 / 1024));
>> + }
>>
>> return sprintf(buf, "%lli%s%c", v, dec, units[u]);
>> }
>>

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

[Kent Overstreet]

On Tue, Sep 05, 2017 at 12:52:44PM +0800, Coly Li wrote:
> OK, I give up the paranoid. This patch will be in my patch list and sent
> out in this merge window. Thanks for your response.

Thanks!

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

[Coly Li]

On 2017/9/5 下午12:19, Kent Overstreet wrote:
> On Mon, Sep 04, 2017 at 02:55:24PM -0700, 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>
> 
> Acked-by: Kent Overstreet <kent.overstreet@gmail.com>
> 

Reviewed-by: Coly Li <colyli@suse.de>

No paranoid comment any more, will be sent to block layer maintainer in
4.14 merge window.

Thanks to Michael Lyle, Michael Lyle, and Kent :-)

Coly Li

>> ---
>>  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;
>> +
>> +	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);
>> +
>> +	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]);
>>  }
>>  
>>  ssize_t bch_snprint_string_list(char *buf, size_t size, const char * const list[],
>> -- 
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Coly Li]

On 2017/9/5 下午12:19, Kent Overstreet wrote:
> On Tue, Sep 05, 2017 at 10:50:51AM +0800, Coly Li wrote:
>> 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.
> 
> I'd prefer without the blank line, but this is pretty nitpicky.
> 
>>> +
>>> +	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,
> 
> It's just a style thing - printing out 100 vs 0.9k. I personally don't care one
> way or the other.
> 

Hi Kent,

Sure, I agree. You are the original author, once you are OK with current
patch, I don't have comment ^_^

> 
>> 	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 ?
> 
> The patch is to fix a rather serious bug where with small negative numbers it
> blows through the output buffer. Let's get this in, please.
> 

OK, I give up the paranoid. This patch will be in my patch list and sent
out in this merge window. Thanks for your response.

Coly Li

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

[Kent Overstreet]

On Mon, Sep 04, 2017 at 02:55:24PM -0700, 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>

Acked-by: Kent Overstreet <kent.overstreet@gmail.com>

> ---
>  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;
> +
> +	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);
> +
> +	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]);
>  }
>  
>  ssize_t bch_snprint_string_list(char *buf, size_t size, const char * const list[],
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kent Overstreet]

On Tue, Sep 05, 2017 at 10:50:51AM +0800, Coly Li wrote:
> 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.

I'd prefer without the blank line, but this is pretty nitpicky.

> > +
> > +	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,

It's just a style thing - printing out 100 vs 0.9k. I personally don't care one
way or the other.


> 	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 ?

The patch is to fix a rather serious bug where with small negative numbers it
blows through the output buffer. Let's get this in, please.

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

[Coly Li]

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

[PATCH] bcache: fix bch_hprint crash and improve output

[Michael Lyle]

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;
+
+	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);
+
+	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]);
 }
 
 ssize_t bch_snprint_string_list(char *buf, size_t size, const char * const list[],
-- 
2.11.0