[PATCH] bitmap: remove explicit newline handling using scnprintf format string

[PATCH] bitmap: remove explicit newline handling using scnprintf format string

[Sudeep Holla]

bitmap_print_to_pagebuf uses scnprintf to copy the cpumask/list to page
buffer. It handles the newline and trailing null character explicitly.

It's unnecessary and also partially duplicated as scnprintf already adds
trailing null character. The newline can be passed through format string
to scnprintf. This patch does that simplification.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Suggested-by: Pawel Moll <Pawel.Moll@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 lib/bitmap.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 64c0926f5dd8..9b24b57b4b01 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -466,15 +466,12 @@ EXPORT_SYMBOL(bitmap_parse_user);
 int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
 			    int nmaskbits)
 {
-	ptrdiff_t len = PTR_ALIGN(buf + PAGE_SIZE - 1, PAGE_SIZE) - buf - 2;
+	ptrdiff_t len = PTR_ALIGN(buf + PAGE_SIZE - 1, PAGE_SIZE) - buf;
 	int n = 0;

-	if (len > 1) {
-		n = list ? scnprintf(buf, len, "%*pbl", nmaskbits, maskp) :
-			   scnprintf(buf, len, "%*pb", nmaskbits, maskp);
-		buf[n++] = '\n';
-		buf[n] = '\0';
-	}
+	if (len > 1)
+		n = list ? scnprintf(buf, len, "%*pbl\n", nmaskbits, maskp) :
+			   scnprintf(buf, len, "%*pb\n", nmaskbits, maskp);
 	return n;
 }
 EXPORT_SYMBOL(bitmap_print_to_pagebuf);
--
1.9.1

Re: [PATCH] bitmap: remove explicit newline handling using scnprintf format string

[Tejun Heo]

Hello, Sudeep.

On Mon, Apr 27, 2015 at 10:46:58AM +0100, Sudeep Holla wrote:
>  int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
>  			    int nmaskbits)
>  {
> -	ptrdiff_t len = PTR_ALIGN(buf + PAGE_SIZE - 1, PAGE_SIZE) - buf - 2;
> +	ptrdiff_t len = PTR_ALIGN(buf + PAGE_SIZE - 1, PAGE_SIZE) - buf;
>  	int n = 0;
> 
> -	if (len > 1) {
> -		n = list ? scnprintf(buf, len, "%*pbl", nmaskbits, maskp) :
> -			   scnprintf(buf, len, "%*pb", nmaskbits, maskp);
> -		buf[n++] = '\n';
> -		buf[n] = '\0';
> -	}
> +	if (len > 1)
> +		n = list ? scnprintf(buf, len, "%*pbl\n", nmaskbits, maskp) :
> +			   scnprintf(buf, len, "%*pb\n", nmaskbits, maskp);
>  	return n;
>  }
>  EXPORT_SYMBOL(bitmap_print_to_pagebuf);

So, there's one behavior difference stemming from this.  When the
buffer is too small, the original code would still output '\n' at the
end while the new code would just continue to print the formatted
string.  Given that bitmap outputs can be pretty long, this behavior
difference has a minute but still non-zero chance of causing something
surprising.  There are multiple copies of the above function in arch
codes too.  We prolly want to audit the usages to verify that the
passed in buffer is always big enough at which point the above
function and its copies can simply be replaced with direct scnprintf()
calls.  This function doesn't actually add anything.

Thanks.

-- 
tejun

Re: [PATCH] bitmap: remove explicit newline handling using scnprintf format string

[Sudeep Holla]

On 27/04/15 17:14, Tejun Heo wrote:
> Hello, Sudeep.
>
> On Mon, Apr 27, 2015 at 10:46:58AM +0100, Sudeep Holla wrote:
>>   int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
>>   			    int nmaskbits)
>>   {
>> -	ptrdiff_t len = PTR_ALIGN(buf + PAGE_SIZE - 1, PAGE_SIZE) - buf - 2;
>> +	ptrdiff_t len = PTR_ALIGN(buf + PAGE_SIZE - 1, PAGE_SIZE) - buf;
>>   	int n = 0;
>>
>> -	if (len > 1) {
>> -		n = list ? scnprintf(buf, len, "%*pbl", nmaskbits, maskp) :
>> -			   scnprintf(buf, len, "%*pb", nmaskbits, maskp);
>> -		buf[n++] = '\n';
>> -		buf[n] = '\0';
>> -	}
>> +	if (len > 1)
>> +		n = list ? scnprintf(buf, len, "%*pbl\n", nmaskbits, maskp) :
>> +			   scnprintf(buf, len, "%*pb\n", nmaskbits, maskp);
>>   	return n;
>>   }
>>   EXPORT_SYMBOL(bitmap_print_to_pagebuf);
>
> So, there's one behavior difference stemming from this.  When the
> buffer is too small, the original code would still output '\n' at the
> end while the new code would just continue to print the formatted
> string.

Completely agree and in-fact we did discuss that internally too.
But since this function deals only with page size buffers, we thought
it's highly unlikely to hit that corner case.

> Given that bitmap outputs can be pretty long, this behavior
> difference has a minute but still non-zero chance of causing something
> surprising.  There are multiple copies of the above function in arch
> codes too.

I assumed that I had consolidated most of them in commit 5aaba36318e5
("cpumask: factor out show_cpumap into separate helper function").
I might have missed, will have a look at it again.

> We prolly want to audit the usages to verify that the
> passed in buffer is always big enough at which point the above
> function and its copies can simply be replaced with direct scnprintf()
> calls.  This function doesn't actually add anything.
>

Ah, right that would be much simpler.

Regards,
Sudeep

Re: [PATCH] bitmap: remove explicit newline handling using scnprintf format string

[Tejun Heo]

Hello, Sudeep.

On Mon, Apr 27, 2015 at 05:26:16PM +0100, Sudeep Holla wrote:
> Completely agree and in-fact we did discuss that internally too.
> But since this function deals only with page size buffers, we thought
> it's highly unlikely to hit that corner case.

Ah, yeah, right.  It'd probably be worthwhile to document the above in
the description tho.

> >Given that bitmap outputs can be pretty long, this behavior
> >difference has a minute but still non-zero chance of causing something
> >surprising.  There are multiple copies of the above function in arch
> >codes too.
> 
> I assumed that I had consolidated most of them in commit 5aaba36318e5
> ("cpumask: factor out show_cpumap into separate helper function").
> I might have missed, will have a look at it again.

I noticed them while %pb[l] conversion but was too lazy to actually do
anything.  Thanks a lot for actually taking care of them.

> >We prolly want to audit the usages to verify that the
> >passed in buffer is always big enough at which point the above
> >function and its copies can simply be replaced with direct scnprintf()
> >calls.  This function doesn't actually add anything.
>
> Ah, right that would be much simpler.

Yeah, let's get rid of it.

Thanks.

-- 
tejun

Re: [PATCH] bitmap: remove explicit newline handling using scnprintf format string

[Sudeep Holla]

Hi Tejun,

On 27/04/15 17:30, Tejun Heo wrote:
> Hello, Sudeep.
>
> On Mon, Apr 27, 2015 at 05:26:16PM +0100, Sudeep Holla wrote:
>> Completely agree and in-fact we did discuss that internally too.
>> But since this function deals only with page size buffers, we thought
>> it's highly unlikely to hit that corner case.
>
> Ah, yeah, right.  It'd probably be worthwhile to document the above in
> the description tho.
>

OK will update the commit log and the doxygen description.

>>> Given that bitmap outputs can be pretty long, this behavior
>>> difference has a minute but still non-zero chance of causing something
>>> surprising.  There are multiple copies of the above function in arch
>>> codes too.
>>
>> I assumed that I had consolidated most of them in commit 5aaba36318e5
>> ("cpumask: factor out show_cpumap into separate helper function").
>> I might have missed, will have a look at it again.
>
> I noticed them while %pb[l] conversion but was too lazy to actually do
> anything.  Thanks a lot for actually taking care of them.
>

No worries.

>>> We prolly want to audit the usages to verify that the
>>> passed in buffer is always big enough at which point the above
>>> function and its copies can simply be replaced with direct scnprintf()
>>> calls.  This function doesn't actually add anything.
>>
>> Ah, right that would be much simpler.
>
> Yeah, let's get rid of it.
>

/me confused, is it fine to push this patch first, and follow up later
after auditing thoroughly to replace with direct scnprintf()

Regards,
Sudeep

Re: [PATCH] bitmap: remove explicit newline handling using scnprintf format string

[Tejun Heo]

On Mon, Apr 27, 2015 at 05:39:13PM +0100, Sudeep Holla wrote:
> >Yeah, let's get rid of it.
> 
> /me confused, is it fine to push this patch first, and follow up later
> after auditing thoroughly to replace with direct scnprintf()

I don't really care how it happens as long as the end result is the
same.

Thanks.

-- 
tejun

[PATCH v2] bitmap: remove explicit newline handling using scnprintf format string

[Sudeep Holla]

bitmap_print_to_pagebuf uses scnprintf to copy the cpumask/list to page
buffer. It handles the newline and trailing null character explicitly.

It's unnecessary and also partially duplicated as scnprintf already adds
trailing null character. The newline can be passed through format string
to scnprintf. This patch does that simplification.

However theoritically there's one behavior difference: when the buffer
is too small, the original code would still output '\n' at the end while
the new code(with this patch) would just continue to print the formatted
string. Since this function is dealing with only page buffers, it's
highly unlikely to hit that corner case.

This patch will help in auditing the users of bitmap_print_to_pagebuf
to verify that the buffer passed is large enough and get rid of it
completely by replacing them with direct scnprintf()

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Suggested-by: Pawel Moll <Pawel.Moll@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 lib/bitmap.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

v1->v2:
 - Updated the doxygen comment and the commit log as suggested by
   Tejun.

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 64c0926f5dd8..10088473e3ba 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -462,19 +462,18 @@ EXPORT_SYMBOL(bitmap_parse_user);
  * Output format is a comma-separated list of decimal numbers and
  * ranges if list is specified or hex digits grouped into comma-separated
  * sets of 8 digits/set. Returns the number of characters written to buf.
+ * It is assumed that the buffer passed is atleast PAGE_SIZE and easily
+ * accommodate nmaskbits.
  */
 int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
 			    int nmaskbits)
 {
-	ptrdiff_t len = PTR_ALIGN(buf + PAGE_SIZE - 1, PAGE_SIZE) - buf - 2;
+	ptrdiff_t len = PTR_ALIGN(buf + PAGE_SIZE - 1, PAGE_SIZE) - buf;
 	int n = 0;

-	if (len > 1) {
-		n = list ? scnprintf(buf, len, "%*pbl", nmaskbits, maskp) :
-			   scnprintf(buf, len, "%*pb", nmaskbits, maskp);
-		buf[n++] = '\n';
-		buf[n] = '\0';
-	}
+	if (len > 1)
+		n = list ? scnprintf(buf, len, "%*pbl\n", nmaskbits, maskp) :
+			   scnprintf(buf, len, "%*pb\n", nmaskbits, maskp);
 	return n;
 }
 EXPORT_SYMBOL(bitmap_print_to_pagebuf);
--
1.9.1

Re: [PATCH v2] bitmap: remove explicit newline handling using scnprintf format string

[Andrew Morton]

On Tue, 28 Apr 2015 16:36:41 +0100 Sudeep Holla <sudeep.holla@arm.com> wrote:

> bitmap_print_to_pagebuf uses scnprintf to copy the cpumask/list to page
> buffer. It handles the newline and trailing null character explicitly.

bitmap_print_to_pagebuf() is horrid :(  That automagic "assume caller
passed us an offset into a PAGE_SIZE area".

> It's unnecessary and also partially duplicated as scnprintf already adds
> trailing null character. The newline can be passed through format string
> to scnprintf. This patch does that simplification.
> 
> However theoritically there's one behavior difference: when the buffer
> is too small, the original code would still output '\n' at the end while
> the new code(with this patch) would just continue to print the formatted
> string. Since this function is dealing with only page buffers, it's
> highly unlikely to hit that corner case.
> 
> This patch will help in auditing the users of bitmap_print_to_pagebuf
> to verify that the buffer passed is large enough and get rid of it
> completely by replacing them with direct scnprintf()

Yes, bitmap_print_to_pagebuf() should be eliminated.  Make the callers
keep track of how much room they have in their buffer.

> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -462,19 +462,18 @@ EXPORT_SYMBOL(bitmap_parse_user);
>   * Output format is a comma-separated list of decimal numbers and
>   * ranges if list is specified or hex digits grouped into comma-separated
>   * sets of 8 digits/set. Returns the number of characters written to buf.
> + * It is assumed that the buffer passed is atleast PAGE_SIZE and easily
> + * accommodate nmaskbits.

Well kind-of.

How does this look?

--- a/lib/bitmap.c~bitmap-remove-explicit-newline-handling-using-scnprintf-format-string-fix
+++ a/lib/bitmap.c
@@ -462,8 +462,10 @@ EXPORT_SYMBOL(bitmap_parse_user);
  * Output format is a comma-separated list of decimal numbers and
  * ranges if list is specified or hex digits grouped into comma-separated
  * sets of 8 digits/set. Returns the number of characters written to buf.
- * It is assumed that the buffer passed is atleast PAGE_SIZE and easily
- * accommodate nmaskbits.
+ *
+ * It is assumed that @buf is a pointer into a PAGE_SIZE area and that
+ * sufficient storage remains at @buf to accommodate the
+ * bitmap_print_to_pagebuf() output.
  */
 int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
 			    int nmaskbits)

Re: [PATCH v2] bitmap: remove explicit newline handling using scnprintf format string

[Sudeep Holla]

On 29/04/15 23:48, Andrew Morton wrote:
> On Tue, 28 Apr 2015 16:36:41 +0100 Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>> bitmap_print_to_pagebuf uses scnprintf to copy the cpumask/list to page
>> buffer. It handles the newline and trailing null character explicitly.
>
> bitmap_print_to_pagebuf() is horrid :(  That automagic "assume caller
> passed us an offset into a PAGE_SIZE area".
>

Agreed.

>> It's unnecessary and also partially duplicated as scnprintf already adds
>> trailing null character. The newline can be passed through format string
>> to scnprintf. This patch does that simplification.
>>
>> However theoritically there's one behavior difference: when the buffer
>> is too small, the original code would still output '\n' at the end while
>> the new code(with this patch) would just continue to print the formatted
>> string. Since this function is dealing with only page buffers, it's
>> highly unlikely to hit that corner case.
>>
>> This patch will help in auditing the users of bitmap_print_to_pagebuf
>> to verify that the buffer passed is large enough and get rid of it
>> completely by replacing them with direct scnprintf()
>
> Yes, bitmap_print_to_pagebuf() should be eliminated.  Make the callers
> keep track of how much room they have in their buffer.
>

Yes that's what Tejun also mentioned in earlier mail. This patch is just
to audit all the callers and check if something breaks before we
replace this with scnprintf() at the call sites.

>> --- a/lib/bitmap.c
>> +++ b/lib/bitmap.c
>> @@ -462,19 +462,18 @@ EXPORT_SYMBOL(bitmap_parse_user);
>>    * Output format is a comma-separated list of decimal numbers and
>>    * ranges if list is specified or hex digits grouped into comma-separated
>>    * sets of 8 digits/set. Returns the number of characters written to buf.
>> + * It is assumed that the buffer passed is atleast PAGE_SIZE and easily
>> + * accommodate nmaskbits.
>
> Well kind-of.
>
> How does this look?
>

Looks good to me.

Regards,
Sudeep

> --- a/lib/bitmap.c~bitmap-remove-explicit-newline-handling-using-scnprintf-format-string-fix
> +++ a/lib/bitmap.c
> @@ -462,8 +462,10 @@ EXPORT_SYMBOL(bitmap_parse_user);
>    * Output format is a comma-separated list of decimal numbers and
>    * ranges if list is specified or hex digits grouped into comma-separated
>    * sets of 8 digits/set. Returns the number of characters written to buf.
> - * It is assumed that the buffer passed is atleast PAGE_SIZE and easily
> - * accommodate nmaskbits.
> + *
> + * It is assumed that @buf is a pointer into a PAGE_SIZE area and that
> + * sufficient storage remains at @buf to accommodate the
> + * bitmap_print_to_pagebuf() output.
>    */
>   int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
>   			    int nmaskbits)
>