[PATCH 00/14] printf stuff for 4.5

[PATCH 00/14] printf stuff for 4.5

[Rasmus Villemoes]

I was too late for 4.4, so here's hoping to get this into 4.5.

1-3 fix a theoretical race in printing strings via %s - since we're
reusing existing code from the dentry printer, we actually also win on
code size. Ingo, can I perhaps get you to turn your "looks good to"
into an ack?

4 fixes a problem introduced by converting all bitmap formatting to go
via %pb[l] - it turns out that bitmaps with >= 1<<15 bits are actually
printed sometimes. This isn't necessarily the best fix, but the
discussion died out, so I'm proposing this for now.

5 is just a minor optimization (maybe not so much on
register-challenged arches).

6 I'm not too sure about, but maybe the bitmap problem had been
discovered sooner (it took a little over half a year to be reported)
if these had been in place.

7 is another microoptimization.

Paranoid-me wanted me to include patch 8, but I don't have strong
feelings for it.

9-14 are minor additions to the test module (some with acks from
Kees).

Rasmus Villemoes (14):
  lib/vsprintf.c: pull out padding code from dentry_name()
  lib/vsprintf.c: move string() below widen_string()
  lib/vsprintf.c: eliminate potential race in string()
  lib/vsprintf.c: expand field_width to 24 bits
  lib/vsprintf.c: help gcc make number() smaller
  lib/vsprintf.c: warn about too large precisions and field widths
  lib/vsprintf.c: slightly refactor vscnprintf()
  lib/kasprintf.c: add sanity check to kvasprintf
  lib/test_printf.c: don't BUG
  lib/test_printf.c: check for out-of-bound writes
  lib/test_printf.c: test precision quirks
  lib/test_printf.c: account for kvasprintf tests
  lib/test_printf.c: add test for large bitmaps
  lib/test_printf.c: test dentry printing

 lib/kasprintf.c   |  10 +--
 lib/test_printf.c |  96 ++++++++++++++++++++++----
 lib/vsprintf.c    | 196 +++++++++++++++++++++++++++++++-----------------------
 3 files changed, 203 insertions(+), 99 deletions(-)

-- 
2.6.1

[PATCH 01/14] lib/vsprintf.c: pull out padding code from dentry_name()

[Rasmus Villemoes]

Pull out the logic in dentry_name() which handles field width space
padding, in preparation for reusing it from string(). Rename the
widen() helper to move_right(), since it is used for handling the
!(flags & LEFT) case.

Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f9cee8e1233c..d7452563a6a6 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -538,7 +538,7 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 	return buf;
 }
 
-static void widen(char *buf, char *end, unsigned len, unsigned spaces)
+static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
 {
 	size_t size;
 	if (buf >= end)	/* nowhere to put anything */
@@ -556,6 +556,35 @@ static void widen(char *buf, char *end, unsigned len, unsigned spaces)
 	memset(buf, ' ', spaces);
 }
 
+/*
+ * Handle field width padding for a string.
+ * @buf: current buffer position
+ * @n: length of string
+ * @end: end of output buffer
+ * @spec: for field width and flags
+ * Returns: new buffer position after padding.
+ */
+static noinline_for_stack
+char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
+{
+	unsigned spaces;
+
+	if (likely(n >= spec.field_width))
+		return buf;
+	/* we want to pad the sucker */
+	spaces = spec.field_width - n;
+	if (!(spec.flags & LEFT)) {
+		move_right(buf - n, end, n, spaces);
+		return buf + spaces;
+	}
+	while (spaces--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+
 static noinline_for_stack
 char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_spec spec,
 		  const char *fmt)
@@ -597,20 +626,7 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 			*buf = c;
 	}
 	rcu_read_unlock();
-	if (n < spec.field_width) {
-		/* we want to pad the sucker */
-		unsigned spaces = spec.field_width - n;
-		if (!(spec.flags & LEFT)) {
-			widen(buf - n, end, n, spaces);
-			return buf + spaces;
-		}
-		while (spaces--) {
-			if (buf < end)
-				*buf = ' ';
-			++buf;
-		}
-	}
-	return buf;
+	return widen_string(buf, n, end, spec);
 }
 
 static noinline_for_stack
-- 
2.6.1

Re: [PATCH 01/14] lib/vsprintf.c: pull out padding code from dentry_name()

[Andy Shevchenko]

On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Pull out the logic in dentry_name() which handles field width space
> padding, in preparation for reusing it from string(). Rename the
> widen() helper to move_right(), since it is used for handling the
> !(flags & LEFT) case.
>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 46 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index f9cee8e1233c..d7452563a6a6 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -538,7 +538,7 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
>         return buf;
>  }
>
> -static void widen(char *buf, char *end, unsigned len, unsigned spaces)
> +static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
>  {
>         size_t size;
>         if (buf >= end) /* nowhere to put anything */
> @@ -556,6 +556,35 @@ static void widen(char *buf, char *end, unsigned len, unsigned spaces)
>         memset(buf, ' ', spaces);
>  }
>
> +/*

Perhaps /**

> + * Handle field width padding for a string.
> + * @buf: current buffer position
> + * @n: length of string
> + * @end: end of output buffer
> + * @spec: for field width and flags
> + * Returns: new buffer position after padding.
> + */
> +static noinline_for_stack
> +char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
> +{
> +       unsigned spaces;
> +
> +       if (likely(n >= spec.field_width))

> +               return buf;
> +       /* we want to pad the sucker */
> +       spaces = spec.field_width - n;
> +       if (!(spec.flags & LEFT)) {
> +               move_right(buf - n, end, n, spaces);
> +               return buf + spaces;
> +       }
> +       while (spaces--) {
> +               if (buf < end)
> +                       *buf = ' ';
> +               ++buf;
> +       }
> +       return buf;
> +}
> +
>  static noinline_for_stack
>  char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_spec spec,
>                   const char *fmt)
> @@ -597,20 +626,7 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
>                         *buf = c;
>         }
>         rcu_read_unlock();
> -       if (n < spec.field_width) {
> -               /* we want to pad the sucker */
> -               unsigned spaces = spec.field_width - n;
> -               if (!(spec.flags & LEFT)) {
> -                       widen(buf - n, end, n, spaces);
> -                       return buf + spaces;
> -               }
> -               while (spaces--) {
> -                       if (buf < end)
> -                               *buf = ' ';
> -                       ++buf;
> -               }
> -       }
> -       return buf;
> +       return widen_string(buf, n, end, spec);
>  }
>
>  static noinline_for_stack
> --
> 2.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/14] lib/vsprintf.c: pull out padding code from dentry_name()

[Rasmus Villemoes]

On Mon, Nov 23 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> Pull out the logic in dentry_name() which handles field width space
>> padding, in preparation for reusing it from string(). Rename the
>> widen() helper to move_right(), since it is used for handling the
>> !(flags & LEFT) case.
>>
>> Cc: Al Viro <viro@ZenIV.linux.org.uk>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>>
>> -static void widen(char *buf, char *end, unsigned len, unsigned spaces)
>> +static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
>>  {
>>         size_t size;
>>         if (buf >= end) /* nowhere to put anything */
>> @@ -556,6 +556,35 @@ static void widen(char *buf, char *end, unsigned len, unsigned spaces)
>>         memset(buf, ' ', spaces);
>>  }
>>
>> +/*
>
> Perhaps /**

Nah, it's not an exported function, so I didn't want to do a formal
kernel-doc thing. I can make it proper kernel-doc if you insist, but I
don't think the whole "current buffer position", "end of output
buffer", "new buffer position" makes sense outside the context of
vsprintf.c with its own slightly peculiar way of doing things.

>> + * Handle field width padding for a string.
>> + * @buf: current buffer position
>> + * @n: length of string
>> + * @end: end of output buffer
>> + * @spec: for field width and flags
>> + * Returns: new buffer position after padding.
>> + */
>> +static noinline_for_stack
>> +char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
>> +{

Rasmus

[PATCH 02/14] lib/vsprintf.c: move string() below widen_string()

[Rasmus Villemoes]

This is pure code movement, making sure the widen_string() helper is
defined before the string() function.

Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 62 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7452563a6a6..a021e6380404 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -507,37 +507,6 @@ char *number(char *buf, char *end, unsigned long long num,
 	return buf;
 }
 
-static noinline_for_stack
-char *string(char *buf, char *end, const char *s, struct printf_spec spec)
-{
-	int len, i;
-
-	if ((unsigned long)s < PAGE_SIZE)
-		s = "(null)";
-
-	len = strnlen(s, spec.precision);
-
-	if (!(spec.flags & LEFT)) {
-		while (len < spec.field_width--) {
-			if (buf < end)
-				*buf = ' ';
-			++buf;
-		}
-	}
-	for (i = 0; i < len; ++i) {
-		if (buf < end)
-			*buf = *s;
-		++buf; ++s;
-	}
-	while (len < spec.field_width--) {
-		if (buf < end)
-			*buf = ' ';
-		++buf;
-	}
-
-	return buf;
-}
-
 static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
 {
 	size_t size;
@@ -586,6 +555,37 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
 }
 
 static noinline_for_stack
+char *string(char *buf, char *end, const char *s, struct printf_spec spec)
+{
+	int len, i;
+
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "(null)";
+
+	len = strnlen(s, spec.precision);
+
+	if (!(spec.flags & LEFT)) {
+		while (len < spec.field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < spec.field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+
+	return buf;
+}
+
+static noinline_for_stack
 char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_spec spec,
 		  const char *fmt)
 {
-- 
2.6.1

[PATCH 03/14] lib/vsprintf.c: eliminate potential race in string()

[Rasmus Villemoes]

If the string corresponding to a %s specifier can change under us, we
might end up copying a \0 byte to the output buffer. There might be
callers who expect the output buffer to contain a genuine C string
whose length is exactly the snprintf return value (assuming truncation
hasn't happened or has been checked for).

We can avoid this by only passing over the source string once,
stopping the first time we meet a nul byte (or when we reach the given
precision), and then letting widen_string() handle left/right space
padding. As a small bonus, this code reuse also makes the generated
code slightly smaller.

Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a021e6380404..63ca52366049 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -557,32 +557,22 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
 static noinline_for_stack
 char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 {
-	int len, i;
+	int len = 0;
+	size_t lim = spec.precision;
 
 	if ((unsigned long)s < PAGE_SIZE)
 		s = "(null)";
 
-	len = strnlen(s, spec.precision);
-
-	if (!(spec.flags & LEFT)) {
-		while (len < spec.field_width--) {
-			if (buf < end)
-				*buf = ' ';
-			++buf;
-		}
-	}
-	for (i = 0; i < len; ++i) {
-		if (buf < end)
-			*buf = *s;
-		++buf; ++s;
-	}
-	while (len < spec.field_width--) {
+	while (lim--) {
+		char c = *s++;
+		if (!c)
+			break;
 		if (buf < end)
-			*buf = ' ';
+			*buf = c;
 		++buf;
+		++len;
 	}
-
-	return buf;
+	return widen_string(buf, len, end, spec);
 }
 
 static noinline_for_stack
-- 
2.6.1

Re: [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string()

[Andy Shevchenko]

On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> If the string corresponding to a %s specifier can change under us, we
> might end up copying a \0 byte to the output buffer. There might be
> callers who expect the output buffer to contain a genuine C string
> whose length is exactly the snprintf return value (assuming truncation
> hasn't happened or has been checked for).
>
> We can avoid this by only passing over the source string once,
> stopping the first time we meet a nul byte (or when we reach the given
> precision), and then letting widen_string() handle left/right space
> padding. As a small bonus, this code reuse also makes the generated
> code slightly smaller.
>

Could it be pair of patches: a) re-use, b) optimize for fuzzy strings?

> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index a021e6380404..63ca52366049 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -557,32 +557,22 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
>  static noinline_for_stack
>  char *string(char *buf, char *end, const char *s, struct printf_spec spec)
>  {
> -       int len, i;
> +       int len = 0;
> +       size_t lim = spec.precision;

Just a nitpick: maybe longer first?

>
>         if ((unsigned long)s < PAGE_SIZE)
>                 s = "(null)";
>
> -       len = strnlen(s, spec.precision);
> -
> -       if (!(spec.flags & LEFT)) {
> -               while (len < spec.field_width--) {
> -                       if (buf < end)
> -                               *buf = ' ';
> -                       ++buf;
> -               }
> -       }
> -       for (i = 0; i < len; ++i) {
> -               if (buf < end)
> -                       *buf = *s;
> -               ++buf; ++s;
> -       }
> -       while (len < spec.field_width--) {
> +       while (lim--) {
> +               char c = *s++;
> +               if (!c)
> +                       break;
>                 if (buf < end)
> -                       *buf = ' ';
> +                       *buf = c;
>                 ++buf;
> +               ++len;
>         }
> -
> -       return buf;
> +       return widen_string(buf, len, end, spec);
>  }
>
>  static noinline_for_stack
> --
> 2.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string()

[Rasmus Villemoes]

On Mon, Nov 23 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> If the string corresponding to a %s specifier can change under us, we
>> might end up copying a \0 byte to the output buffer. There might be
>> callers who expect the output buffer to contain a genuine C string
>> whose length is exactly the snprintf return value (assuming truncation
>> hasn't happened or has been checked for).
>>
>> We can avoid this by only passing over the source string once,
>> stopping the first time we meet a nul byte (or when we reach the given
>> precision), and then letting widen_string() handle left/right space
>> padding. As a small bonus, this code reuse also makes the generated
>> code slightly smaller.
>>
>
> Could it be pair of patches: a) re-use, b) optimize for fuzzy strings?

I'm afraid I have no idea what you mean. The patch is already broken
into three (pull out from dentry(), move helper, do the actual thing to
string()). What's a 'fuzzy string', and what optimization do you think of?

This patch already gives us the bonus of only passing over the source
once instead of twice. (Well, at the expense of a little complicated
logic in case we have a larger field width and not the LEFT flag set,
but these are so extremely rare compared to plain %s that it's not worth
caring about. And in any case, the logic already existed.)

>> Cc: Ingo Molnar <mingo@kernel.org>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  lib/vsprintf.c | 28 +++++++++-------------------
>>  1 file changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index a021e6380404..63ca52366049 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -557,32 +557,22 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
>>  static noinline_for_stack
>>  char *string(char *buf, char *end, const char *s, struct printf_spec spec)
>>  {
>> -       int len, i;
>> +       int len = 0;
>> +       size_t lim = spec.precision;
>
> Just a nitpick: maybe longer first?

Why?

Rasmus

[PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits

[Rasmus Villemoes]

Maurizio Lombardi reported a problem [1] with the %pb extension: It
doesn't work for sufficiently large bitmaps, since the size is stashed
in the field_width field of the struct printf_spec, which is currently
an s16. Concretely, this manifested itself in
/sys/bus/pseudo/drivers/scsi_debug/map being empty, since the bitmap
printer got a size of 0, which is the 16 bit truncation of the actual
bitmap size.

We do want to keep struct printf_spec at 8 bytes so that it can
cheaply be passed by value. The qualifier field is only used for
internal bookkeeping in format_decode, so we might as well use a local
variable for that. This gives us an additional 8 bits, which we can
then use for the field width.

To stay in 8 bytes, we need to do a little rearranging and make the
type member a bitfield as well. For consistency, change all the
members to bit fields. gcc doesn't generate much worse code with these
changes (in fact, bloat-o-meter says we save 300 bytes - which I think
is a little surprising).

I didn't find a BUILD_BUG/compiletime_assertion/... which would work
outside function context, so for now I just open-coded it.

[1] http://thread.gmane.org/gmane.linux.kernel/2034835

Reported-by: Maurizio Lombardi <mlombard@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 63ca52366049..01c3aa638582 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -380,13 +380,13 @@ enum format_type {
 };
 
 struct printf_spec {
-	u8	type;		/* format_type enum */
-	u8	flags;		/* flags to number() */
-	u8	base;		/* number base, 8, 10 or 16 only */
-	u8	qualifier;	/* number qualifier, one of 'hHlLtzZ' */
-	s16	field_width;	/* width of output field */
-	s16	precision;	/* # of digits/chars */
-};
+	unsigned int	type:8;		/* format_type enum */
+	signed int	field_width:24;	/* width of output field */
+	unsigned int	flags:8;	/* flags to number() */
+	unsigned int	base:8;		/* number base, 8, 10 or 16 only */
+	signed int	precision:16;	/* # of digits/chars */
+} __packed;
+extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)];
 
 static noinline_for_stack
 char *number(char *buf, char *end, unsigned long long num,
@@ -1641,6 +1641,7 @@ static noinline_for_stack
 int format_decode(const char *fmt, struct printf_spec *spec)
 {
 	const char *start = fmt;
+	char qualifier;
 
 	/* we finished early by reading the field width */
 	if (spec->type == FORMAT_TYPE_WIDTH) {
@@ -1723,16 +1724,16 @@ precision:
 
 qualifier:
 	/* get the conversion qualifier */
-	spec->qualifier = -1;
+	qualifier = 0;
 	if (*fmt == 'h' || _tolower(*fmt) == 'l' ||
 	    _tolower(*fmt) == 'z' || *fmt == 't') {
-		spec->qualifier = *fmt++;
-		if (unlikely(spec->qualifier == *fmt)) {
-			if (spec->qualifier == 'l') {
-				spec->qualifier = 'L';
+		qualifier = *fmt++;
+		if (unlikely(qualifier == *fmt)) {
+			if (qualifier == 'l') {
+				qualifier = 'L';
 				++fmt;
-			} else if (spec->qualifier == 'h') {
-				spec->qualifier = 'H';
+			} else if (qualifier == 'h') {
+				qualifier = 'H';
 				++fmt;
 			}
 		}
@@ -1789,19 +1790,19 @@ qualifier:
 		return fmt - start;
 	}
 
-	if (spec->qualifier == 'L')
+	if (qualifier == 'L')
 		spec->type = FORMAT_TYPE_LONG_LONG;
-	else if (spec->qualifier == 'l') {
+	else if (qualifier == 'l') {
 		BUILD_BUG_ON(FORMAT_TYPE_ULONG + SIGN != FORMAT_TYPE_LONG);
 		spec->type = FORMAT_TYPE_ULONG + (spec->flags & SIGN);
-	} else if (_tolower(spec->qualifier) == 'z') {
+	} else if (_tolower(qualifier) == 'z') {
 		spec->type = FORMAT_TYPE_SIZE_T;
-	} else if (spec->qualifier == 't') {
+	} else if (qualifier == 't') {
 		spec->type = FORMAT_TYPE_PTRDIFF;
-	} else if (spec->qualifier == 'H') {
+	} else if (qualifier == 'H') {
 		BUILD_BUG_ON(FORMAT_TYPE_UBYTE + SIGN != FORMAT_TYPE_BYTE);
 		spec->type = FORMAT_TYPE_UBYTE + (spec->flags & SIGN);
-	} else if (spec->qualifier == 'h') {
+	} else if (qualifier == 'h') {
 		BUILD_BUG_ON(FORMAT_TYPE_USHORT + SIGN != FORMAT_TYPE_SHORT);
 		spec->type = FORMAT_TYPE_USHORT + (spec->flags & SIGN);
 	} else {
-- 
2.6.1

Re: [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits

[Andy Shevchenko]

On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Maurizio Lombardi reported a problem [1] with the %pb extension: It
> doesn't work for sufficiently large bitmaps, since the size is stashed
> in the field_width field of the struct printf_spec, which is currently
> an s16. Concretely, this manifested itself in
> /sys/bus/pseudo/drivers/scsi_debug/map being empty, since the bitmap
> printer got a size of 0, which is the 16 bit truncation of the actual
> bitmap size.
>
> We do want to keep struct printf_spec at 8 bytes so that it can
> cheaply be passed by value. The qualifier field is only used for
> internal bookkeeping in format_decode, so we might as well use a local
> variable for that. This gives us an additional 8 bits, which we can
> then use for the field width.
>
> To stay in 8 bytes, we need to do a little rearranging and make the
> type member a bitfield as well. For consistency, change all the
> members to bit fields. gcc doesn't generate much worse code with these
> changes (in fact, bloat-o-meter says we save 300 bytes - which I think
> is a little surprising).
>
> I didn't find a BUILD_BUG/compiletime_assertion/... which would work
> outside function context, so for now I just open-coded it.

And any objections to put it into vsnprintf() ?

>
> [1] http://thread.gmane.org/gmane.linux.kernel/2034835
>
> Reported-by: Maurizio Lombardi <mlombard@redhat.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 63ca52366049..01c3aa638582 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -380,13 +380,13 @@ enum format_type {
>  };
>
>  struct printf_spec {
> -       u8      type;           /* format_type enum */
> -       u8      flags;          /* flags to number() */
> -       u8      base;           /* number base, 8, 10 or 16 only */
> -       u8      qualifier;      /* number qualifier, one of 'hHlLtzZ' */
> -       s16     field_width;    /* width of output field */
> -       s16     precision;      /* # of digits/chars */
> -};
> +       unsigned int    type:8;         /* format_type enum */
> +       signed int      field_width:24; /* width of output field */
> +       unsigned int    flags:8;        /* flags to number() */
> +       unsigned int    base:8;         /* number base, 8, 10 or 16 only */
> +       signed int      precision:16;   /* # of digits/chars */
> +} __packed;
> +extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)];
>
>  static noinline_for_stack
>  char *number(char *buf, char *end, unsigned long long num,
> @@ -1641,6 +1641,7 @@ static noinline_for_stack
>  int format_decode(const char *fmt, struct printf_spec *spec)
>  {
>         const char *start = fmt;
> +       char qualifier;

u8 ?

>
>         /* we finished early by reading the field width */
>         if (spec->type == FORMAT_TYPE_WIDTH) {
> @@ -1723,16 +1724,16 @@ precision:
>
>  qualifier:
>         /* get the conversion qualifier */
> -       spec->qualifier = -1;
> +       qualifier = 0;
>         if (*fmt == 'h' || _tolower(*fmt) == 'l' ||
>             _tolower(*fmt) == 'z' || *fmt == 't') {
> -               spec->qualifier = *fmt++;
> -               if (unlikely(spec->qualifier == *fmt)) {
> -                       if (spec->qualifier == 'l') {
> -                               spec->qualifier = 'L';
> +               qualifier = *fmt++;
> +               if (unlikely(qualifier == *fmt)) {
> +                       if (qualifier == 'l') {
> +                               qualifier = 'L';
>                                 ++fmt;
> -                       } else if (spec->qualifier == 'h') {
> -                               spec->qualifier = 'H';
> +                       } else if (qualifier == 'h') {
> +                               qualifier = 'H';
>                                 ++fmt;
>                         }
>                 }
> @@ -1789,19 +1790,19 @@ qualifier:
>                 return fmt - start;
>         }
>
> -       if (spec->qualifier == 'L')
> +       if (qualifier == 'L')
>                 spec->type = FORMAT_TYPE_LONG_LONG;
> -       else if (spec->qualifier == 'l') {
> +       else if (qualifier == 'l') {
>                 BUILD_BUG_ON(FORMAT_TYPE_ULONG + SIGN != FORMAT_TYPE_LONG);
>                 spec->type = FORMAT_TYPE_ULONG + (spec->flags & SIGN);
> -       } else if (_tolower(spec->qualifier) == 'z') {
> +       } else if (_tolower(qualifier) == 'z') {
>                 spec->type = FORMAT_TYPE_SIZE_T;
> -       } else if (spec->qualifier == 't') {
> +       } else if (qualifier == 't') {
>                 spec->type = FORMAT_TYPE_PTRDIFF;
> -       } else if (spec->qualifier == 'H') {
> +       } else if (qualifier == 'H') {
>                 BUILD_BUG_ON(FORMAT_TYPE_UBYTE + SIGN != FORMAT_TYPE_BYTE);
>                 spec->type = FORMAT_TYPE_UBYTE + (spec->flags & SIGN);
> -       } else if (spec->qualifier == 'h') {
> +       } else if (qualifier == 'h') {
>                 BUILD_BUG_ON(FORMAT_TYPE_USHORT + SIGN != FORMAT_TYPE_SHORT);
>                 spec->type = FORMAT_TYPE_USHORT + (spec->flags & SIGN);
>         } else {
> --
> 2.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits

[Rasmus Villemoes]

On Tue, Nov 24 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> Maurizio Lombardi reported a problem [1] with the %pb extension: It
>> doesn't work for sufficiently large bitmaps, since the size is stashed
>> in the field_width field of the struct printf_spec, which is currently
>> an s16. Concretely, this manifested itself in
>> /sys/bus/pseudo/drivers/scsi_debug/map being empty, since the bitmap
>> printer got a size of 0, which is the 16 bit truncation of the actual
>> bitmap size.
>>
>> We do want to keep struct printf_spec at 8 bytes so that it can
>> cheaply be passed by value. The qualifier field is only used for
>> internal bookkeeping in format_decode, so we might as well use a local
>> variable for that. This gives us an additional 8 bits, which we can
>> then use for the field width.
>>
>> To stay in 8 bytes, we need to do a little rearranging and make the
>> type member a bitfield as well. For consistency, change all the
>> members to bit fields. gcc doesn't generate much worse code with these
>> changes (in fact, bloat-o-meter says we save 300 bytes - which I think
>> is a little surprising).
>>
>> I didn't find a BUILD_BUG/compiletime_assertion/... which would work
>> outside function context, so for now I just open-coded it.
>
> And any objections to put it into vsnprintf() ?

I'd like to keep it close to the type definition. And I was hoping
someone would come forward and say "yeah, that's been bugging me too,
here's a patch I've been sitting on to fix that". Almost every compiler
released this decade has _Static_assert, it's about time we start using
that instead of the current mess of homegrown workarounds...

Rasmus

Re: [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits

[Tejun Heo]

On Mon, Nov 23, 2015 at 10:29:21PM +0100, Rasmus Villemoes wrote:
> Maurizio Lombardi reported a problem [1] with the %pb extension: It
> doesn't work for sufficiently large bitmaps, since the size is stashed
> in the field_width field of the struct printf_spec, which is currently
> an s16. Concretely, this manifested itself in
> /sys/bus/pseudo/drivers/scsi_debug/map being empty, since the bitmap
> printer got a size of 0, which is the 16 bit truncation of the actual
> bitmap size.
> 
> We do want to keep struct printf_spec at 8 bytes so that it can
> cheaply be passed by value. The qualifier field is only used for
> internal bookkeeping in format_decode, so we might as well use a local
> variable for that. This gives us an additional 8 bits, which we can
> then use for the field width.
> 
> To stay in 8 bytes, we need to do a little rearranging and make the
> type member a bitfield as well. For consistency, change all the
> members to bit fields. gcc doesn't generate much worse code with these
> changes (in fact, bloat-o-meter says we save 300 bytes - which I think
> is a little surprising).
> 
> I didn't find a BUILD_BUG/compiletime_assertion/... which would work
> outside function context, so for now I just open-coded it.
> 
> [1] http://thread.gmane.org/gmane.linux.kernel/2034835
> 
> Reported-by: Maurizio Lombardi <mlombard@redhat.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

FWIW,

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH 05/14] lib/vsprintf.c: help gcc make number() smaller

[Rasmus Villemoes]

One consequence of the reorganization of struct printf_spec to make
field_width 24 bits was that number() gained about 180 bytes. Since
spec is never passed to other functions, we can help gcc make number()
lose most of that extra weight by using local variables for the field
width and precision.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 01c3aa638582..d7e27c54fa00 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -399,6 +399,8 @@ char *number(char *buf, char *end, unsigned long long num,
 	int need_pfx = ((spec.flags & SPECIAL) && spec.base != 10);
 	int i;
 	bool is_zero = num == 0LL;
+	int field_width = spec.field_width;
+	int precision = spec.precision;
 
 	/* locase = 0 or 0x20. ORing digits or letters with 'locase'
 	 * produces same digits or (maybe lowercased) letters */
@@ -410,20 +412,20 @@ char *number(char *buf, char *end, unsigned long long num,
 		if ((signed long long)num < 0) {
 			sign = '-';
 			num = -(signed long long)num;
-			spec.field_width--;
+			field_width--;
 		} else if (spec.flags & PLUS) {
 			sign = '+';
-			spec.field_width--;
+			field_width--;
 		} else if (spec.flags & SPACE) {
 			sign = ' ';
-			spec.field_width--;
+			field_width--;
 		}
 	}
 	if (need_pfx) {
 		if (spec.base == 16)
-			spec.field_width -= 2;
+			field_width -= 2;
 		else if (!is_zero)
-			spec.field_width--;
+			field_width--;
 	}
 
 	/* generate full string in tmp[], in reverse order */
@@ -445,12 +447,12 @@ char *number(char *buf, char *end, unsigned long long num,
 	}
 
 	/* printing 100 using %2d gives "100", not "00" */
-	if (i > spec.precision)
-		spec.precision = i;
+	if (i > precision)
+		precision = i;
 	/* leading space padding */
-	spec.field_width -= spec.precision;
+	field_width -= precision;
 	if (!(spec.flags & (ZEROPAD | LEFT))) {
-		while (--spec.field_width >= 0) {
+		while (--field_width >= 0) {
 			if (buf < end)
 				*buf = ' ';
 			++buf;
@@ -479,14 +481,14 @@ char *number(char *buf, char *end, unsigned long long num,
 	if (!(spec.flags & LEFT)) {
 		char c = ' ' + (spec.flags & ZEROPAD);
 		BUILD_BUG_ON(' ' + ZEROPAD != '0');
-		while (--spec.field_width >= 0) {
+		while (--field_width >= 0) {
 			if (buf < end)
 				*buf = c;
 			++buf;
 		}
 	}
 	/* hmm even more zero padding? */
-	while (i <= --spec.precision) {
+	while (i <= --precision) {
 		if (buf < end)
 			*buf = '0';
 		++buf;
@@ -498,7 +500,7 @@ char *number(char *buf, char *end, unsigned long long num,
 		++buf;
 	}
 	/* trailing space padding */
-	while (--spec.field_width >= 0) {
+	while (--field_width >= 0) {
 		if (buf < end)
 			*buf = ' ';
 		++buf;
-- 
2.6.1

Re: [PATCH 05/14] lib/vsprintf.c: help gcc make number() smaller

[Andy Shevchenko]

On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> One consequence of the reorganization of struct printf_spec to make
> field_width 24 bits was that number() gained about 180 bytes. Since
> spec is never passed to other functions, we can help gcc make number()
> lose most of that extra weight by using local variables for the field
> width and precision.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

One minor nitpick below

> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 01c3aa638582..d7e27c54fa00 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -399,6 +399,8 @@ char *number(char *buf, char *end, unsigned long long num,
>         int need_pfx = ((spec.flags & SPECIAL) && spec.base != 10);
>         int i;
>         bool is_zero = num == 0LL;
> +       int field_width = spec.field_width;
> +       int precision = spec.precision;

I would put these before int i; But your choice.

>
>         /* locase = 0 or 0x20. ORing digits or letters with 'locase'
>          * produces same digits or (maybe lowercased) letters */
> @@ -410,20 +412,20 @@ char *number(char *buf, char *end, unsigned long long num,
>                 if ((signed long long)num < 0) {
>                         sign = '-';
>                         num = -(signed long long)num;
> -                       spec.field_width--;
> +                       field_width--;
>                 } else if (spec.flags & PLUS) {
>                         sign = '+';
> -                       spec.field_width--;
> +                       field_width--;
>                 } else if (spec.flags & SPACE) {
>                         sign = ' ';
> -                       spec.field_width--;
> +                       field_width--;
>                 }
>         }
>         if (need_pfx) {
>                 if (spec.base == 16)
> -                       spec.field_width -= 2;
> +                       field_width -= 2;
>                 else if (!is_zero)
> -                       spec.field_width--;
> +                       field_width--;
>         }
>
>         /* generate full string in tmp[], in reverse order */
> @@ -445,12 +447,12 @@ char *number(char *buf, char *end, unsigned long long num,
>         }
>
>         /* printing 100 using %2d gives "100", not "00" */
> -       if (i > spec.precision)
> -               spec.precision = i;
> +       if (i > precision)
> +               precision = i;
>         /* leading space padding */
> -       spec.field_width -= spec.precision;
> +       field_width -= precision;
>         if (!(spec.flags & (ZEROPAD | LEFT))) {
> -               while (--spec.field_width >= 0) {
> +               while (--field_width >= 0) {
>                         if (buf < end)
>                                 *buf = ' ';
>                         ++buf;
> @@ -479,14 +481,14 @@ char *number(char *buf, char *end, unsigned long long num,
>         if (!(spec.flags & LEFT)) {
>                 char c = ' ' + (spec.flags & ZEROPAD);
>                 BUILD_BUG_ON(' ' + ZEROPAD != '0');
> -               while (--spec.field_width >= 0) {
> +               while (--field_width >= 0) {
>                         if (buf < end)
>                                 *buf = c;
>                         ++buf;
>                 }
>         }
>         /* hmm even more zero padding? */
> -       while (i <= --spec.precision) {
> +       while (i <= --precision) {
>                 if (buf < end)
>                         *buf = '0';
>                 ++buf;
> @@ -498,7 +500,7 @@ char *number(char *buf, char *end, unsigned long long num,
>                 ++buf;
>         }
>         /* trailing space padding */
> -       while (--spec.field_width >= 0) {
> +       while (--field_width >= 0) {
>                 if (buf < end)
>                         *buf = ' ';
>                 ++buf;
> --
> 2.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

[PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths

[Rasmus Villemoes]

The field width is overloaded to pass some extra information for
some %p extensions (e.g. #bits for %pb). But we might silently
truncate the passed value when we stash it in struct printf_spec (see
e.g. "lib/vsprintf.c: expand field_width to 24 bits").  Hopefully 23
value bits should now be enough for everybody, but if not, let's make
some noise.

Do the same for the precision. In both cases, clamping seems more
sensible than truncating. While, according to POSIX, "A negative
precision is taken as if the precision were omitted.", the kernel's
printf has always treated that case as if the precision was 0, so we
use that as lower bound. For the field width, the smallest
representable value is actually -(1<<23), but a negative field width
means 'set the LEFT flag and use the absolute value', so we want the
absolute value to fit.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7e27c54fa00..8af5535fd738 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -386,6 +386,8 @@ struct printf_spec {
 	unsigned int	base:8;		/* number base, 8, 10 or 16 only */
 	signed int	precision:16;	/* # of digits/chars */
 } __packed;
+#define FIELD_WIDTH_MAX ((1 << 23) - 1)
+#define PRECISION_MAX ((1 << 15) - 1)
 extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)];
 
 static noinline_for_stack
@@ -1815,6 +1817,24 @@ qualifier:
 	return ++fmt - start;
 }
 
+static inline void
+set_field_width(struct printf_spec *spec, int width)
+{
+	spec->field_width = width;
+	if (WARN_ONCE(spec->field_width != width, "field width %d too large", width)) {
+		spec->field_width = clamp(width, -FIELD_WIDTH_MAX, FIELD_WIDTH_MAX);
+	}
+}
+
+static inline void
+set_precision(struct printf_spec *spec, int prec)
+{
+	spec->precision = prec;
+	if (WARN_ONCE(spec->precision != prec, "precision %d too large", prec)) {
+		spec->precision = clamp(prec, 0, PRECISION_MAX);
+	}
+}
+
 /**
  * vsnprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into
@@ -1882,11 +1902,11 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 		}
 
 		case FORMAT_TYPE_WIDTH:
-			spec.field_width = va_arg(args, int);
+			set_field_width(&spec, va_arg(args, int));
 			break;
 
 		case FORMAT_TYPE_PRECISION:
-			spec.precision = va_arg(args, int);
+			set_precision(&spec, va_arg(args, int));
 			break;
 
 		case FORMAT_TYPE_CHAR: {
-- 
2.6.1

Re: [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths

[Andy Shevchenko]

On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> The field width is overloaded to pass some extra information for
> some %p extensions (e.g. #bits for %pb). But we might silently
> truncate the passed value when we stash it in struct printf_spec (see
> e.g. "lib/vsprintf.c: expand field_width to 24 bits").  Hopefully 23
> value bits should now be enough for everybody, but if not, let's make
> some noise.
>
> Do the same for the precision. In both cases, clamping seems more
> sensible than truncating. While, according to POSIX, "A negative
> precision is taken as if the precision were omitted.", the kernel's
> printf has always treated that case as if the precision was 0, so we
> use that as lower bound. For the field width, the smallest
> representable value is actually -(1<<23), but a negative field width
> means 'set the LEFT flag and use the absolute value', so we want the
> absolute value to fit.
>

Do we need to do the same for bstr_printf() ?

> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index d7e27c54fa00..8af5535fd738 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -386,6 +386,8 @@ struct printf_spec {
>         unsigned int    base:8;         /* number base, 8, 10 or 16 only */
>         signed int      precision:16;   /* # of digits/chars */
>  } __packed;
> +#define FIELD_WIDTH_MAX ((1 << 23) - 1)
> +#define PRECISION_MAX ((1 << 15) - 1)
>  extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)];
>
>  static noinline_for_stack
> @@ -1815,6 +1817,24 @@ qualifier:
>         return ++fmt - start;
>  }
>
> +static inline void
> +set_field_width(struct printf_spec *spec, int width)
> +{
> +       spec->field_width = width;
> +       if (WARN_ONCE(spec->field_width != width, "field width %d too large", width)) {
> +               spec->field_width = clamp(width, -FIELD_WIDTH_MAX, FIELD_WIDTH_MAX);
> +       }
> +}
> +
> +static inline void
> +set_precision(struct printf_spec *spec, int prec)
> +{
> +       spec->precision = prec;
> +       if (WARN_ONCE(spec->precision != prec, "precision %d too large", prec)) {
> +               spec->precision = clamp(prec, 0, PRECISION_MAX);
> +       }
> +}
> +
>  /**
>   * vsnprintf - Format a string and place it in a buffer
>   * @buf: The buffer to place the result into
> @@ -1882,11 +1902,11 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>                 }
>
>                 case FORMAT_TYPE_WIDTH:
> -                       spec.field_width = va_arg(args, int);
> +                       set_field_width(&spec, va_arg(args, int));
>                         break;
>
>                 case FORMAT_TYPE_PRECISION:
> -                       spec.precision = va_arg(args, int);
> +                       set_precision(&spec, va_arg(args, int));
>                         break;
>
>                 case FORMAT_TYPE_CHAR: {
> --
> 2.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths

[Rasmus Villemoes]

On Mon, Nov 23 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> The field width is overloaded to pass some extra information for
>> some %p extensions (e.g. #bits for %pb). But we might silently
>> truncate the passed value when we stash it in struct printf_spec (see
>> e.g. "lib/vsprintf.c: expand field_width to 24 bits").  Hopefully 23
>> value bits should now be enough for everybody, but if not, let's make
>> some noise.
>>
>> Do the same for the precision. In both cases, clamping seems more
>> sensible than truncating. While, according to POSIX, "A negative
>> precision is taken as if the precision were omitted.", the kernel's
>> printf has always treated that case as if the precision was 0, so we
>> use that as lower bound. For the field width, the smallest
>> representable value is actually -(1<<23), but a negative field width
>> means 'set the LEFT flag and use the absolute value', so we want the
>> absolute value to fit.
>>
>
> Do we need to do the same for bstr_printf() ?
>

Heh, apparently I didn't learn anything from 762abb51. Thanks, will fix
in next spin.

Rasmus

[PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf()

[Rasmus Villemoes]

If we're given a size of 0, the vsnprintf() won't have any side
effects, and neither "i < size" or "size != 0" will trigger. So we
might as well return 0 immediately.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 8af5535fd738..e22a6189548f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2036,13 +2036,14 @@ int vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
 {
 	int i;
 
+	if (unlikely(!size))
+		return 0;
+
 	i = vsnprintf(buf, size, fmt, args);
 
 	if (likely(i < size))
 		return i;
-	if (size != 0)
-		return size - 1;
-	return 0;
+	return size - 1;
 }
 EXPORT_SYMBOL(vscnprintf);
 
-- 
2.6.1

Re: [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf()

[Andy Shevchenko]

On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> If we're given a size of 0, the vsnprintf() won't have any side
> effects, and neither "i < size" or "size != 0" will trigger. So we
> might as well return 0 immediately.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 8af5535fd738..e22a6189548f 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2036,13 +2036,14 @@ int vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  {
>         int i;
>
> +       if (unlikely(!size))
> +               return 0;
> +

Might it potentially shadow any issue when run vsnprintf(buf, 0, fmt,
args); with certain arguments?
I can imagine something like %pV with unstable pointer.

>         i = vsnprintf(buf, size, fmt, args);
>
>         if (likely(i < size))
>                 return i;
> -       if (size != 0)
> -               return size - 1;
> -       return 0;
> +       return size - 1;
>  }
>  EXPORT_SYMBOL(vscnprintf);
>
> --
> 2.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf()

[Rasmus Villemoes]

On Mon, Nov 23 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> If we're given a size of 0, the vsnprintf() won't have any side
>> effects, and neither "i < size" or "size != 0" will trigger. So we
>> might as well return 0 immediately.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  lib/vsprintf.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 8af5535fd738..e22a6189548f 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -2036,13 +2036,14 @@ int vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
>>  {
>>         int i;
>>
>> +       if (unlikely(!size))
>> +               return 0;
>> +
>
> Might it potentially shadow any issue when run vsnprintf(buf, 0, fmt,
> args); with certain arguments?

Only if we ever come up with a %p extension with side effects, but then
people couldn't rely on them happening exactly once anyway (kasprintf
would make them happen twice). printf-like calls are also often compiled
out or disabled (dyndebug, ratelimit, ...) without it being obvious at
the call site whether they'll run or not, so I think such a hypothetical
%p extension would meet some resistance.

> I can imagine something like %pV with unstable pointer.

I don't see how %pV is different than any other current %p
extensions. 

Rasmus

[PATCH 08/14] lib/kasprintf.c: add sanity check to kvasprintf

[Rasmus Villemoes]

kasprintf relies on being able to replay the formatting and getting
the same result (in particular, the same length). This will almost
always work, but it is possible that the object pointed to by a %s or
%p argument changed under us (so we might get truncated output). Add a
somewhat paranoid sanity check and let's see if it ever triggers.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/kasprintf.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/kasprintf.c b/lib/kasprintf.c
index f194e6e593e1..7f6c506a4942 100644
--- a/lib/kasprintf.c
+++ b/lib/kasprintf.c
@@ -13,19 +13,21 @@
 /* Simplified asprintf. */
 char *kvasprintf(gfp_t gfp, const char *fmt, va_list ap)
 {
-	unsigned int len;
+	unsigned int first, second;
 	char *p;
 	va_list aq;
 
 	va_copy(aq, ap);
-	len = vsnprintf(NULL, 0, fmt, aq);
+	first = vsnprintf(NULL, 0, fmt, aq);
 	va_end(aq);
 
-	p = kmalloc_track_caller(len+1, gfp);
+	p = kmalloc_track_caller(first+1, gfp);
 	if (!p)
 		return NULL;
 
-	vsnprintf(p, len+1, fmt, ap);
+	second = vsnprintf(p, first+1, fmt, ap);
+	WARN(first != second, "different return values (%u and %u) from vsnprintf(\"%s\", ...)",
+	     first, second, fmt);
 
 	return p;
 }
-- 
2.6.1

Re: [PATCH 08/14] lib/kasprintf.c: add sanity check to kvasprintf

[Andy Shevchenko]

On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> kasprintf relies on being able to replay the formatting and getting
> the same result (in particular, the same length). This will almost
> always work, but it is possible that the object pointed to by a %s or
> %p argument changed under us (so we might get truncated output). Add a
> somewhat paranoid sanity check and let's see if it ever triggers.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/kasprintf.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kasprintf.c b/lib/kasprintf.c
> index f194e6e593e1..7f6c506a4942 100644
> --- a/lib/kasprintf.c
> +++ b/lib/kasprintf.c
> @@ -13,19 +13,21 @@
>  /* Simplified asprintf. */
>  char *kvasprintf(gfp_t gfp, const char *fmt, va_list ap)
>  {
> -       unsigned int len;
> +       unsigned int first, second;

vsnprintf() returns plain int.
Perhaps change the type to follow the function.

>         char *p;
>         va_list aq;
>
>         va_copy(aq, ap);
> -       len = vsnprintf(NULL, 0, fmt, aq);
> +       first = vsnprintf(NULL, 0, fmt, aq);
>         va_end(aq);
>
> -       p = kmalloc_track_caller(len+1, gfp);
> +       p = kmalloc_track_caller(first+1, gfp);
>         if (!p)
>                 return NULL;
>
> -       vsnprintf(p, len+1, fmt, ap);
> +       second = vsnprintf(p, first+1, fmt, ap);
> +       WARN(first != second, "different return values (%u and %u) from vsnprintf(\"%s\", ...)",
> +            first, second, fmt);
>
>         return p;
>  }
> --
> 2.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

[PATCH 09/14] lib/test_printf.c: don't BUG

[Rasmus Villemoes]

BUG is a completely unnecessarily big hammer, and we're more likely to
get the internal bug reported if we just pr_err() and ensure the test
suite fails.

Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index c5a666af9ba5..9232a2add28c 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -91,7 +91,12 @@ __test(const char *expect, int elen, const char *fmt, ...)
 	int rand;
 	char *p;
 
-	BUG_ON(elen >= BUF_SIZE);
+	if (elen >= BUF_SIZE) {
+		pr_err("error in test suite: expected output length %d too long. Format was '%s'.\n",
+		       elen, fmt);
+		failed_tests++;
+		return;
+	}
 
 	va_start(ap, fmt);
 
-- 
2.6.1

[PATCH 10/14] lib/test_printf.c: check for out-of-bound writes

[Rasmus Villemoes]

Add a few padding bytes on either side of the test buffer, and check
that these (and the part of the buffer not used) are untouched by
vsnprintf.

Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 9232a2add28c..1ce1a1dd8faf 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -16,6 +16,7 @@
 #include <linux/in.h>
 
 #define BUF_SIZE 256
+#define PAD_SIZE 16
 #define FILL_CHAR '$'
 
 #define PTR1 ((void*)0x01234567)
@@ -39,6 +40,7 @@
 static unsigned total_tests __initdata;
 static unsigned failed_tests __initdata;
 static char *test_buffer __initdata;
+static char *alloced_buffer __initdata;
 
 static int __printf(4, 0) __init
 do_test(int bufsize, const char *expect, int elen,
@@ -49,7 +51,7 @@ do_test(int bufsize, const char *expect, int elen,
 
 	total_tests++;
 
-	memset(test_buffer, FILL_CHAR, BUF_SIZE);
+	memset(alloced_buffer, FILL_CHAR, BUF_SIZE + 2*PAD_SIZE);
 	va_copy(aq, ap);
 	ret = vsnprintf(test_buffer, bufsize, fmt, aq);
 	va_end(aq);
@@ -60,8 +62,13 @@ do_test(int bufsize, const char *expect, int elen,
 		return 1;
 	}
 
+	if (memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE)) {
+		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote before buffer\n", bufsize, fmt);
+		return 1;
+	}
+
 	if (!bufsize) {
-		if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE)) {
+		if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE + PAD_SIZE)) {
 			pr_warn("vsnprintf(buf, 0, \"%s\", ...) wrote to buffer\n",
 				fmt);
 			return 1;
@@ -76,6 +83,12 @@ do_test(int bufsize, const char *expect, int elen,
 		return 1;
 	}
 
+	if (memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1))) {
+		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
+			bufsize, fmt);
+		return 1;
+	}
+
 	if (memcmp(test_buffer, expect, written)) {
 		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'\n",
 			bufsize, fmt, test_buffer, written, expect);
@@ -342,16 +355,17 @@ test_pointer(void)
 static int __init
 test_printf_init(void)
 {
-	test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
-	if (!test_buffer)
+	alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL);
+	if (!alloced_buffer)
 		return -ENOMEM;
+	test_buffer = alloced_buffer + PAD_SIZE;
 
 	test_basic();
 	test_number();
 	test_string();
 	test_pointer();
 
-	kfree(test_buffer);
+	kfree(alloced_buffer);
 
 	if (failed_tests == 0)
 		pr_info("all %u tests passed\n", total_tests);
-- 
2.6.1

[PATCH 11/14] lib/test_printf.c: test precision quirks

[Rasmus Villemoes]

The kernel's printf doesn't follow the standards in a few corner cases
(which are probably mostly irrelevant). Add tests that document the
current behaviour.

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 1ce1a1dd8faf..5f742b99cfdc 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -166,14 +166,22 @@ test_string(void)
 	test("", "%s%.0s", "", "123");
 	test("ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, "123456");
 	test("1  |  2|3  |  4|5  ", "%-3s|%3s|%-*s|%*s|%*s", "1", "2", 3, "3", 3, "4", -3, "5");
+	test("1234      ", "%-10.4s", "123456");
+	test("      1234", "%10.4s", "123456");
 	/*
-	 * POSIX and C99 say that a missing precision should be
-	 * treated as a precision of 0. However, the kernel's printf
-	 * implementation treats this case as if the . wasn't
-	 * present. Let's add a test case documenting the current
-	 * behaviour; should anyone ever feel the need to follow the
-	 * standards more closely, this can be revisited.
+	 * POSIX and C99 say that a negative precision (which is only
+	 * possible to pass via a * argument) should be treated as if
+	 * the precision wasn't present, and that if the precision is
+	 * omitted (as in %.s), the precision should be taken to be
+	 * 0. However, the kernel's printf behave exactly opposite,
+	 * treating a negative precision as 0 and treating an omitted
+	 * precision specifier as if no precision was given.
+	 *
+	 * These test cases document the current behaviour; should
+	 * anyone ever feel the need to follow the standards more
+	 * closely, this can be revisited.
 	 */
+	test("    ", "%4.*s", -5, "123456");
 	test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c");
 	test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
 }
-- 
2.6.1

[PATCH 12/14] lib/test_printf.c: account for kvasprintf tests

[Rasmus Villemoes]

These should also count as performed tests.

Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 5f742b99cfdc..fc9169d360ba 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -127,6 +127,7 @@ __test(const char *expect, int elen, const char *fmt, ...)
 
 	p = kvasprintf(GFP_KERNEL, fmt, ap);
 	if (p) {
+		total_tests++;
 		if (memcmp(p, expect, elen+1)) {
 			pr_warn("kvasprintf(..., \"%s\", ...) returned '%s', expected '%s'\n",
 				fmt, p, expect);
-- 
2.6.1

[PATCH 13/14] lib/test_printf.c: add test for large bitmaps

[Rasmus Villemoes]

Following "lib/vsprintf.c: expand field_width to 24 bits", let's add a
test to see that we now actually support bitmaps with 65536 bits.

Cc: Maurizio Lombardi <mlombard@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index fc9169d360ba..705907aec901 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 
+#include <linux/bitmap.h>
 #include <linux/socket.h>
 #include <linux/in.h>
 
@@ -316,6 +317,20 @@ struct_clk(void)
 }
 
 static void __init
+large_bitmap(void)
+{
+	const int nbits = 1 << 16;
+	unsigned long *bits = kcalloc(BITS_TO_LONGS(nbits), sizeof(long), GFP_KERNEL);
+	if (!bits)
+		return;
+
+	bitmap_set(bits, 1, 20);
+	bitmap_set(bits, 60000, 15);
+	test("1-20,60000-60014", "%*pbl", nbits, bits);
+	kfree(bits);
+}
+
+static void __init
 bitmap(void)
 {
 	DECLARE_BITMAP(bits, 20);
@@ -334,6 +349,8 @@ bitmap(void)
 	bitmap_fill(bits, 20);
 	test("fffff|fffff", "%20pb|%*pb", bits, 20, bits);
 	test("0-19|0-19", "%20pbl|%*pbl", bits, 20, bits);
+
+	large_bitmap();
 }
 
 static void __init
-- 
2.6.1

[PATCH 14/14] lib/test_printf.c: test dentry printing

[Rasmus Villemoes]

Cc: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 705907aec901..6b7c4a1ec141 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -13,6 +13,7 @@
 #include <linux/string.h>
 
 #include <linux/bitmap.h>
+#include <linux/dcache.h>
 #include <linux/socket.h>
 #include <linux/in.h>
 
@@ -301,9 +302,35 @@ uuid(void)
 	test("03020100-0504-0706-0809-0A0B0C0D0E0F", "%pUL", uuid);
 }
 
+static struct dentry test_dentry[4] __initdata = {
+	{ .d_parent = &test_dentry[0],
+	  .d_name = { .len = 3, .name = test_dentry[0].d_iname },
+	  .d_iname = "foo" },
+	{ .d_parent = &test_dentry[0],
+	  .d_name = { .len = 5, .name = test_dentry[1].d_iname },
+	  .d_iname = "bravo" },
+	{ .d_parent = &test_dentry[1],
+	  .d_name = { .len = 4, .name = test_dentry[2].d_iname },
+	  .d_iname = "alfa" },
+	{ .d_parent = &test_dentry[2],
+	  .d_name = { .len = 5, .name = test_dentry[3].d_iname },
+	  .d_iname = "romeo" },
+};
+
 static void __init
 dentry(void)
 {
+	test("foo", "%pd", &test_dentry[0]);
+	test("foo", "%pd2", &test_dentry[0]);
+
+	test("romeo", "%pd", &test_dentry[3]);
+	test("alfa/romeo", "%pd2", &test_dentry[3]);
+	test("bravo/alfa/romeo", "%pd3", &test_dentry[3]);
+	test("/bravo/alfa/romeo", "%pd4", &test_dentry[3]);
+	test("/bravo/alfa", "%pd4", &test_dentry[2]);
+
+	test("bravo/alfa  |bravo/alfa  ", "%-12pd2|%*pd2", &test_dentry[2], -12, &test_dentry[2]);
+	test("  bravo/alfa|  bravo/alfa", "%12pd2|%*pd2", &test_dentry[2], 12, &test_dentry[2]);
 }
 
 static void __init
-- 
2.6.1