[PATCH v3 00/14] printf stuff for 4.5

[PATCH v3 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.

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

8-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/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: add a few number() tests
  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 | 121 ++++++++++++++++++++++++++++++----
 lib/vsprintf.c    | 193 +++++++++++++++++++++++++++++++-----------------------
 3 files changed, 226 insertions(+), 98 deletions(-)

-- 
2.6.1

[PATCH v3 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

[PATCH v3 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 v3 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

[PATCH v3 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: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Joe Perches <joe@perches.com>
Acked-by: Tejun Heo <tj@kernel.org>
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

[PATCH v3 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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
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

[PATCH v3 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.

Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
v3: also do the check in bstr_printf (thanks Andy).

 lib/vsprintf.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7e27c54fa00..3db2281e6026 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 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 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: {
@@ -2326,11 +2346,11 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 		}
 
 		case FORMAT_TYPE_WIDTH:
-			spec.field_width = get_arg(int);
+			set_field_width(&spec, get_arg(int));
 			break;
 
 		case FORMAT_TYPE_PRECISION:
-			spec.precision = get_arg(int);
+			set_precision(&spec, get_arg(int));
 			break;
 
 		case FORMAT_TYPE_CHAR: {
-- 
2.6.1

[PATCH v3 07/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

[PATCH v3 08/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 v3 09/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 v3 10/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 | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 1ce1a1dd8faf..3393d667c6b8 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -166,14 +166,23 @@ 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("123456", "%.s", "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 v3 11/14] lib/test_printf.c: add a few number() tests

[Rasmus Villemoes]

This adds a few tests to test_number, one of which serves to document
another deviation from POSIX/C99 (printing 0 with an explicit
precision of 0).

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

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 3393d667c6b8..b23ce824766f 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -158,6 +158,30 @@ test_number(void)
 	test("0x1234abcd  ", "%#-12x", 0x1234abcd);
 	test("  0x1234abcd", "%#12x", 0x1234abcd);
 	test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
+	test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
+	test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
+	test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
+	/*
+	 * POSIX/C99: »The result of converting zero with an explicit
+	 * precision of zero shall be no characters.« Hence the output
+	 * from the below test should really be "00|0||| ". However,
+	 * the kernel's printf also produces a single 0 in that
+	 * case. This test case simply documents the current
+	 * behaviour.
+	 */
+	test("00|0|0|0|0", "%.2d|%.1d|%.0d|%.*d|%1.0d", 0, 0, 0, 0, 0, 0);
+#ifndef __CHAR_UNSIGNED__
+	{
+		/*
+		 * Passing a 'char' to a %02x specifier doesn't do
+		 * what was presumably the intention when char is
+		 * signed and the value is negative. One must either &
+		 * with 0xff or cast to u8.
+		 */
+		char val = -16;
+		test("0xfffffff0|0xf0|0xf0", "%#02x|%#02x|%#02x", val, val & 0xff, (u8)val);
+	}
+#endif
 }
 
 static void __init
-- 
2.6.1

[PATCH v3 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 b23ce824766f..3e21170d327d 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 v3 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 3e21170d327d..60740c10c3e8 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>
 
@@ -341,6 +342,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);
@@ -359,6 +374,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 v3 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 60740c10c3e8..0234356c6698 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>
 
@@ -326,9 +327,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

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

[Joe Perches]

On Thu, 2015-12-03 at 21:51 +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

Thanks for keeping at this Rasmus.
This seems quite reasonable.

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

[Andy Shevchenko]

On Thu, 2015-12-03 at 12:54 -0800, Joe Perches wrote:
> On Thu, 2015-12-03 at 21:51 +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
> 
> Thanks for keeping at this Rasmus.
> This seems quite reasonable.

I like most of the stuff here, though, Joe, can we avoid open-coded
BUILD_BUG_ON()?


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Andrew Morton]

On Thu, 03 Dec 2015 23:28:58 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, 2015-12-03 at 12:54 -0800, Joe Perches wrote:
> > On Thu, 2015-12-03 at 21:51 +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
> > 
> > Thanks for keeping at this Rasmus.
> > This seems quite reasonable.
> 
> I like most of the stuff here, though, Joe, can we avoid open-coded
> BUILD_BUG_ON()?

Well we could just do

--- a/lib/vsprintf.c~lib-vsprintfc-expand-field_width-to-24-bits-fix
+++ a/lib/vsprintf.c
@@ -386,7 +386,6 @@ struct printf_spec {
 	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,
@@ -400,6 +399,8 @@ char *number(char *buf, char *end, unsig
 	int i;
 	bool is_zero = num == 0LL;
 
+	BUILD_BUG_ON(sizeof(struct printf_spec) != 8);
+
 	/* locase = 0 or 0x20. ORing digits or letters with 'locase'
 	 * produces same digits or (maybe lowercased) letters */
 	locase = (spec.flags & SMALL);

Which is better than open-coding it, IMO.



I've been fiddling with a BUILD_BUG_ON which works outside functions
using gcc's __COUNTER__ - something like

#define BBO(expr) typedef char __bbo##__COUNTER__[1-2*(!!expr)]

BBO(1 == 1);
BBO(2 == 2);

but that comes out as

typedef char __bbo__COUNTER__[1-2*(!!1 == 1)];
typedef char __bbo__COUNTER__[1-2*(!!2 == 2)];

instead of

typedef char __bbo0[1-2*(!!1 == 1)];
typedef char __bbo1[1-2*(!!2 == 2)];

There's some trick here but I've forgotten what it is.

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

[Joe Perches]

On Thu, 2015-12-03 at 23:28 +0200, Andy Shevchenko wrote:
> On Thu, 2015-12-03 at 12:54 -0800, Joe Perches wrote:
> > On Thu, 2015-12-03 at 21:51 +0100, Rasmus Villemoes wrote:
[]
> > > 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
[]
> I like most of the stuff here, though, can we avoid open-coded
> BUILD_BUG_ON()?

Not so far as I can know.  Maybe another generic could be added.

It doesn't seem this specific check is all that useful.

sizeof(struct printf_spec) != 8

I suppose there could be an assert in some function instead.

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

[Joe Perches]

On Thu, 2015-12-03 at 15:34 -0800, Andrew Morton wrote:
> I've been fiddling with a BUILD_BUG_ON which works outside functions
> using gcc's __COUNTER__ - something like
> 
> #define BBO(expr) typedef char __bbo##__COUNTER__[1-2*(!!expr)]

nit:  you need another parenthesis around expr

> BBO(1 == 1);
> BBO(2 == 2);
> 
> but that comes out as
> 
> typedef char __bbo__COUNTER__[1-2*(!!1 == 1)];
> typedef char __bbo__COUNTER__[1-2*(!!2 == 2)];
> 
> instead of
> 
> typedef char __bbo0[1-2*(!!1 == 1)];
> typedef char __bbo1[1-2*(!!2 == 2)];
> 
> There's some trick here but I've forgotten what it is.

I believe it's something like:

#define __stringify_2(a, b)	a##b
#define __stringify2(a, b)	__stringify_2(a, b)

#define BBO(expr) typedef char __stringify2(bbo, __COUNTER__)[1 - 2*(!!(expr))]

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

[Andrew Morton]

On Thu,  3 Dec 2015 21:51:13 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

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

Confused.  qstr has no .len.

lib/test_printf.c:332: error: unknown field 'len' specified in initializer

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

[Rasmus Villemoes]

On Fri, Dec 04 2015, Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu,  3 Dec 2015 21:51:13 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
>> +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" },
>
> Confused.  qstr has no .len.
>
> lib/test_printf.c:332: error: unknown field 'len' specified in initializer

Huh? It goes without saying that I've compiled and run this, and never
seen this problem. struct qstr does have a len member, though it's
hidden inside an anonymous struct inside an anonymous union. But at
least my gcc (4.9) has no problem with those initializers - and
fs/dcache.c happily accesses name->len all the time (how else would one
get at the member). Also note the lack of 0day complaints. What tool
gave that error?

Also confused.

Rasmus

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

[Andrew Morton]

On Fri, 04 Dec 2015 09:16:02 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On Fri, Dec 04 2015, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Thu,  3 Dec 2015 21:51:13 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >
> >> +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" },
> >
> > Confused.  qstr has no .len.
> >
> > lib/test_printf.c:332: error: unknown field 'len' specified in initializer
> 
> Huh? It goes without saying that I've compiled and run this, and never
> seen this problem. struct qstr does have a len member, though it's
> hidden inside an anonymous struct inside an anonymous union. But at
> least my gcc (4.9) has no problem with those initializers - and
> fs/dcache.c happily accesses name->len all the time (how else would one
> get at the member). Also note the lack of 0day complaints. What tool
> gave that error?
> 

Ah, OK, that's the gcc-4.4.4 bug with initialization of anonymous
unions.

I really should get a new compiler, but running old compilers has value
- it finds problems with old compilers!

I've never found a workaround for this problem so the fix thus far has
been to do the initialization in regular old C code.

However it appears that the QSTR_INIT() macro (which you should have
used anyway!) contains some magic sauce.  This fixes it:

--- a/lib/test_printf.c~lib-test_printfc-test-dentry-printing-fix
+++ a/lib/test_printf.c
@@ -329,16 +329,16 @@ uuid(void)
 
 static struct dentry test_dentry[4] __initdata = {
 	{ .d_parent = &test_dentry[0],
-	  .d_name = { .len = 3, .name = test_dentry[0].d_iname },
+	  .d_name = QSTR_INIT(test_dentry[0].d_iname, 3),
 	  .d_iname = "foo" },
 	{ .d_parent = &test_dentry[0],
-	  .d_name = { .len = 5, .name = test_dentry[1].d_iname },
+	  .d_name = QSTR_INIT(test_dentry[1].d_iname, 5),
 	  .d_iname = "bravo" },
 	{ .d_parent = &test_dentry[1],
-	  .d_name = { .len = 4, .name = test_dentry[2].d_iname },
+	  .d_name = QSTR_INIT(test_dentry[2].d_iname, 4),
 	  .d_iname = "alfa" },
 	{ .d_parent = &test_dentry[2],
-	  .d_name = { .len = 5, .name = test_dentry[3].d_iname },
+	  .d_name = QSTR_INIT(test_dentry[3].d_iname, 5),
 	  .d_iname = "romeo" },
 };
 
I assume the code still works ;)



And while I'm there...

From: Andrew Morton <akpm@linux-foundation.org>
Subject: include/linux/dcache.h: remove semicolons from HASH_LEN_DECLARE

A little cleanup - the invocation site provdes the semicolon.

Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/dcache.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN include/linux/dcache.h~include-linux-dcacheh-remove-semicolons-from-hash_len_declare include/linux/dcache.h
--- a/include/linux/dcache.h~include-linux-dcacheh-remove-semicolons-from-hash_len_declare
+++ a/include/linux/dcache.h
@@ -27,10 +27,10 @@ struct vfsmount;
 
 /* The hash is always the low bits of hash_len */
 #ifdef __LITTLE_ENDIAN
- #define HASH_LEN_DECLARE u32 hash; u32 len;
+ #define HASH_LEN_DECLARE u32 hash; u32 len
  #define bytemask_from_count(cnt)	(~(~0ul << (cnt)*8))
 #else
- #define HASH_LEN_DECLARE u32 len; u32 hash;
+ #define HASH_LEN_DECLARE u32 len; u32 hash
  #define bytemask_from_count(cnt)	(~(~0ul >> (cnt)*8))
 #endif
 
_

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

[Rasmus Villemoes]

On Fri, Dec 04 2015, Joe Perches <joe@perches.com> wrote:

> On Thu, 2015-12-03 at 15:34 -0800, Andrew Morton wrote:
>> I've been fiddling with a BUILD_BUG_ON which works outside functions
>> using gcc's __COUNTER__ - something like
>> 
>> #define BBO(expr) typedef char __bbo##__COUNTER__[1-2*(!!expr)]
>
> nit:  you need another parenthesis around expr
>
>> BBO(1 == 1);
>> BBO(2 == 2);
>> 
>> but that comes out as
>> 
>> typedef char __bbo__COUNTER__[1-2*(!!1 == 1)];
>> typedef char __bbo__COUNTER__[1-2*(!!2 == 2)];
>> 
>> instead of
>> 
>> typedef char __bbo0[1-2*(!!1 == 1)];
>> typedef char __bbo1[1-2*(!!2 == 2)];
>> 
>> There's some trick here but I've forgotten what it is.
>
> I believe it's something like:
>
> #define __stringify_2(a, b)	a##b
> #define __stringify2(a, b)	__stringify_2(a, b)
>
> #define BBO(expr) typedef char __stringify2(bbo, __COUNTER__)[1 - 2*(!!(expr))]

Let's at least not reinvent two wheels. __UNIQUE_ID exists and does the
gluing (which we have __PASTE for, not stringify) etc., and uses
__LINE__ as a poor man's fallback for compilers without __COUNTER__ (gcc
< 4.3).

But I don't see why we even need the unique identifier. What's wrong
with 'extern char blabla[1 - 2*(!!(expr))]'? blabla can be declared
multiple times without problems - and when it fails, we either get a
'negative size' error or at least some complaint about conflicting
declarations. Maybe stick a __always_unused in to prevent gcc from
complaining if this declaration is inside a function.

Rasmus

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

[Rasmus Villemoes]

On Fri, Dec 04 2015, Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 03 Dec 2015 23:28:58 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
>> I like most of the stuff here, though, Joe, can we avoid open-coded
>> BUILD_BUG_ON()?
>
> Well we could just do
>
[snip]
>
> Which is better than open-coding it, IMO.

I'd really prefer to have the assertion near the type definition (which
is why I did it that way), but if this is what it takes to get this
patch in, fine by me.

Rasmus