* [PATCH 00/14] printf stuff for 4.5
@ 2015-11-23 21:29 Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 01/14] lib/vsprintf.c: pull out padding code from dentry_name() Rasmus Villemoes
` (13 more replies)
0 siblings, 14 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton
Cc: Rasmus Villemoes, Al Viro, Ingo Molnar, Tejun Heo, Joe Perches,
Kees Cook, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 01/14] lib/vsprintf.c: pull out padding code from dentry_name()
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 22:56 ` Andy Shevchenko
2015-11-23 21:29 ` [PATCH 02/14] lib/vsprintf.c: move string() below widen_string() Rasmus Villemoes
` (12 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, Al Viro, Ingo Molnar, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 02/14] lib/vsprintf.c: move string() below widen_string()
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 01/14] lib/vsprintf.c: pull out padding code from dentry_name() Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string() Rasmus Villemoes
` (11 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, Ingo Molnar, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string()
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 01/14] lib/vsprintf.c: pull out padding code from dentry_name() Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 02/14] lib/vsprintf.c: move string() below widen_string() Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 22:51 ` Andy Shevchenko
2015-11-23 21:29 ` [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits Rasmus Villemoes
` (10 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, Ingo Molnar, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
` (2 preceding siblings ...)
2015-11-23 21:29 ` [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string() Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 23:05 ` Andy Shevchenko
2015-11-23 23:19 ` Tejun Heo
2015-11-23 21:29 ` [PATCH 05/14] lib/vsprintf.c: help gcc make number() smaller Rasmus Villemoes
` (9 subsequent siblings)
13 siblings, 2 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton
Cc: Rasmus Villemoes, Maurizio Lombardi, Tejun Heo, Joe Perches,
linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 05/14] lib/vsprintf.c: help gcc make number() smaller
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
` (3 preceding siblings ...)
2015-11-23 21:29 ` [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 22:17 ` Andy Shevchenko
2015-11-23 21:29 ` [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths Rasmus Villemoes
` (8 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
` (4 preceding siblings ...)
2015-11-23 21:29 ` [PATCH 05/14] lib/vsprintf.c: help gcc make number() smaller Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 22:34 ` Andy Shevchenko
2015-11-23 21:29 ` [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf() Rasmus Villemoes
` (7 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf()
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
` (5 preceding siblings ...)
2015-11-23 21:29 ` [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 22:39 ` Andy Shevchenko
2015-11-23 21:29 ` [PATCH 08/14] lib/kasprintf.c: add sanity check to kvasprintf Rasmus Villemoes
` (6 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 08/14] lib/kasprintf.c: add sanity check to kvasprintf
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
` (6 preceding siblings ...)
2015-11-23 21:29 ` [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf() Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 23:10 ` Andy Shevchenko
2015-11-23 21:29 ` [PATCH 09/14] lib/test_printf.c: don't BUG Rasmus Villemoes
` (5 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 09/14] lib/test_printf.c: don't BUG
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
` (7 preceding siblings ...)
2015-11-23 21:29 ` [PATCH 08/14] lib/kasprintf.c: add sanity check to kvasprintf Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 10/14] lib/test_printf.c: check for out-of-bound writes Rasmus Villemoes
` (4 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, Kees Cook, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 10/14] lib/test_printf.c: check for out-of-bound writes
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
` (8 preceding siblings ...)
2015-11-23 21:29 ` [PATCH 09/14] lib/test_printf.c: don't BUG Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 11/14] lib/test_printf.c: test precision quirks Rasmus Villemoes
` (3 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, Kees Cook, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 11/14] lib/test_printf.c: test precision quirks
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
` (9 preceding siblings ...)
2015-11-23 21:29 ` [PATCH 10/14] lib/test_printf.c: check for out-of-bound writes Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 12/14] lib/test_printf.c: account for kvasprintf tests Rasmus Villemoes
` (2 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, Kees Cook, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 12/14] lib/test_printf.c: account for kvasprintf tests
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
` (10 preceding siblings ...)
2015-11-23 21:29 ` [PATCH 11/14] lib/test_printf.c: test precision quirks Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 13/14] lib/test_printf.c: add test for large bitmaps Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 14/14] lib/test_printf.c: test dentry printing Rasmus Villemoes
13 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, Kees Cook, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 13/14] lib/test_printf.c: add test for large bitmaps
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
` (11 preceding siblings ...)
2015-11-23 21:29 ` [PATCH 12/14] lib/test_printf.c: account for kvasprintf tests Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 14/14] lib/test_printf.c: test dentry printing Rasmus Villemoes
13 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton
Cc: Rasmus Villemoes, Kees Cook, Maurizio Lombardi, Tejun Heo, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 14/14] lib/test_printf.c: test dentry printing
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
` (12 preceding siblings ...)
2015-11-23 21:29 ` [PATCH 13/14] lib/test_printf.c: add test for large bitmaps Rasmus Villemoes
@ 2015-11-23 21:29 ` Rasmus Villemoes
13 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-23 21:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Rasmus Villemoes, Al Viro, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 05/14] lib/vsprintf.c: help gcc make number() smaller
2015-11-23 21:29 ` [PATCH 05/14] lib/vsprintf.c: help gcc make number() smaller Rasmus Villemoes
@ 2015-11-23 22:17 ` Andy Shevchenko
0 siblings, 0 replies; 28+ messages in thread
From: Andy Shevchenko @ 2015-11-23 22:17 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths
2015-11-23 21:29 ` [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths Rasmus Villemoes
@ 2015-11-23 22:34 ` Andy Shevchenko
2015-11-26 21:10 ` Rasmus Villemoes
0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2015-11-23 22:34 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf()
2015-11-23 21:29 ` [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf() Rasmus Villemoes
@ 2015-11-23 22:39 ` Andy Shevchenko
2015-11-26 21:23 ` Rasmus Villemoes
0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2015-11-23 22:39 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string()
2015-11-23 21:29 ` [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string() Rasmus Villemoes
@ 2015-11-23 22:51 ` Andy Shevchenko
2015-11-26 21:31 ` Rasmus Villemoes
0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2015-11-23 22:51 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, Ingo Molnar, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 01/14] lib/vsprintf.c: pull out padding code from dentry_name()
2015-11-23 21:29 ` [PATCH 01/14] lib/vsprintf.c: pull out padding code from dentry_name() Rasmus Villemoes
@ 2015-11-23 22:56 ` Andy Shevchenko
2015-11-26 21:41 ` Rasmus Villemoes
0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2015-11-23 22:56 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, Al Viro, Ingo Molnar, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits
2015-11-23 21:29 ` [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits Rasmus Villemoes
@ 2015-11-23 23:05 ` Andy Shevchenko
2015-11-26 21:47 ` Rasmus Villemoes
2015-11-23 23:19 ` Tejun Heo
1 sibling, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2015-11-23 23:05 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Andrew Morton, Maurizio Lombardi, Tejun Heo, Joe Perches, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 08/14] lib/kasprintf.c: add sanity check to kvasprintf
2015-11-23 21:29 ` [PATCH 08/14] lib/kasprintf.c: add sanity check to kvasprintf Rasmus Villemoes
@ 2015-11-23 23:10 ` Andy Shevchenko
0 siblings, 0 replies; 28+ messages in thread
From: Andy Shevchenko @ 2015-11-23 23:10 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits
2015-11-23 21:29 ` [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits Rasmus Villemoes
2015-11-23 23:05 ` Andy Shevchenko
@ 2015-11-23 23:19 ` Tejun Heo
1 sibling, 0 replies; 28+ messages in thread
From: Tejun Heo @ 2015-11-23 23:19 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Andrew Morton, Maurizio Lombardi, Joe Perches, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths
2015-11-23 22:34 ` Andy Shevchenko
@ 2015-11-26 21:10 ` Rasmus Villemoes
0 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-26 21:10 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Andrew Morton, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf()
2015-11-23 22:39 ` Andy Shevchenko
@ 2015-11-26 21:23 ` Rasmus Villemoes
0 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-26 21:23 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Andrew Morton, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string()
2015-11-23 22:51 ` Andy Shevchenko
@ 2015-11-26 21:31 ` Rasmus Villemoes
0 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-26 21:31 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Andrew Morton, Ingo Molnar, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 01/14] lib/vsprintf.c: pull out padding code from dentry_name()
2015-11-23 22:56 ` Andy Shevchenko
@ 2015-11-26 21:41 ` Rasmus Villemoes
0 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-26 21:41 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Andrew Morton, Al Viro, Ingo Molnar, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits
2015-11-23 23:05 ` Andy Shevchenko
@ 2015-11-26 21:47 ` Rasmus Villemoes
0 siblings, 0 replies; 28+ messages in thread
From: Rasmus Villemoes @ 2015-11-26 21:47 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Morton, Maurizio Lombardi, Tejun Heo, Joe Perches, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2015-11-26 21:47 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-23 21:29 [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 01/14] lib/vsprintf.c: pull out padding code from dentry_name() Rasmus Villemoes
2015-11-23 22:56 ` Andy Shevchenko
2015-11-26 21:41 ` Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 02/14] lib/vsprintf.c: move string() below widen_string() Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string() Rasmus Villemoes
2015-11-23 22:51 ` Andy Shevchenko
2015-11-26 21:31 ` Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits Rasmus Villemoes
2015-11-23 23:05 ` Andy Shevchenko
2015-11-26 21:47 ` Rasmus Villemoes
2015-11-23 23:19 ` Tejun Heo
2015-11-23 21:29 ` [PATCH 05/14] lib/vsprintf.c: help gcc make number() smaller Rasmus Villemoes
2015-11-23 22:17 ` Andy Shevchenko
2015-11-23 21:29 ` [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths Rasmus Villemoes
2015-11-23 22:34 ` Andy Shevchenko
2015-11-26 21:10 ` Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf() Rasmus Villemoes
2015-11-23 22:39 ` Andy Shevchenko
2015-11-26 21:23 ` Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 08/14] lib/kasprintf.c: add sanity check to kvasprintf Rasmus Villemoes
2015-11-23 23:10 ` Andy Shevchenko
2015-11-23 21:29 ` [PATCH 09/14] lib/test_printf.c: don't BUG Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 10/14] lib/test_printf.c: check for out-of-bound writes Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 11/14] lib/test_printf.c: test precision quirks Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 12/14] lib/test_printf.c: account for kvasprintf tests Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 13/14] lib/test_printf.c: add test for large bitmaps Rasmus Villemoes
2015-11-23 21:29 ` [PATCH 14/14] lib/test_printf.c: test dentry printing Rasmus Villemoes
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.