Hi,
Changes in v6:
[As commented in private to Linus, I assume the NAK from Linus in v5
applies to the macro that evaluates twice. This is resolved in v6, so
I send assuming no NAKs to the overall patch set.]
- Don't try to have a single function. Have sprintf_end() for chaining
calls and sprintf_trunc() --which is the fmt version of strscpy()--
for single calls. Then sprintf_array() --which is the fmt version of
the 2-argument strscpy()-- for single calls with an array as input.
- Fix implementation of sprintf_array() to not evaluate twice.
These changes are essentially a roll-back to the general idea in v3,
except for the more explicit names.
Remaining questions:
- There are only 3 remaining calls to snprintf(3) under mm/. They are
just fine for now, which is why I didn't replace them. If anyone
wants to replace them, to get rid of all snprintf(3), we could that.
I think for now we can leave them, to minimize the churn.
$ grep -rnI snprintf mm/
mm/hugetlb_cgroup.c:674: snprintf(buf, size, "%luGB",
hsize / SZ_1G);
mm/hugetlb_cgroup.c:676: snprintf(buf, size, "%luMB",
hsize / SZ_1M);
mm/hugetlb_cgroup.c:678: snprintf(buf, size, "%luKB",
hsize / SZ_1K);
They could be replaced by sprintf_trunc().
- There are only 2 remaining calls to the kernel's scnprintf(). This
one I would really like to get rid of. Also, those calls are quite
suspicious of not being what we want. Please do have a look at them
and confirm what's the appropriate behavior in the 2 cases when the
string is truncated or not copied at all. That code is very scary
for me to try to guess.
$ grep -rnI scnprintf mm/
mm/kfence/report.c:75: int len = scnprintf(buf, sizeof(buf),
"%ps", (void *)stack_entries[skipnr]);
mm/kfence/kfence_test.mod.c:22: { 0x96848186, "scnprintf" },
mm/kmsan/report.c:42: len = scnprintf(buf, sizeof(buf), "%ps",
Apart from two calls, I see a string literal with that name. Please
let me know if I should do anything about it. I don't know what that
is.
- I think we should remove one error handling check in
"mm/page_owner.c" (marked with an XXX comment), but I'm not 100%
sure. Please confirm.
Other comments:
- This is still not complying to coding style. I'll keep it like that
while questions remain open.
- I've tested the tests under CONFIG_KFENCE_KUNIT_TEST=y, and this has
no regressions at all.
- With the current style of the sprintf_end() prototyope, this triggers
a diagnostic due to a GCC bug:
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108036>
It would be interesting to ask GCC to fix that bug. (Added relevant
GCC maintainers and contributors to CC in this cover letter.)
- The call sprintf_end(p, end, "") in lib/stackdepot.c, within
stack_depot_sprint_end(), produces a warning for having an empty
string. This could be replaced by a strcpy_end(p, end, "") if/when
we add that function.
For anyone new to the thread, sprintf_end() will be proposed for
standardization soon as seprintf():
<https://lore.kernel.org/linux-hardening/20250710024745.143955-1-...@kernel.org/T/#u>
Have a lovely night!
Alex
Alejandro Colomar (8):
vsprintf: Add [v]sprintf_trunc()
vsprintf: Add [v]sprintf_end()
sprintf: Add [v]sprintf_array()
stacktrace, stackdepot: Add sprintf_end()-like variants of functions
mm: Use sprintf_end() instead of less ergonomic APIs
array_size.h: Add ENDOF()
mm: Fix benign off-by-one bugs
mm: Use [v]sprintf_array() to avoid specifying the array size
include/linux/array_size.h | 6 +++
include/linux/sprintf.h | 8 +++
include/linux/stackdepot.h | 13 +++++
include/linux/stacktrace.h | 3 ++
kernel/stacktrace.c | 28 ++++++++++
lib/stackdepot.c | 13 +++++
lib/vsprintf.c | 107 +++++++++++++++++++++++++++++++++++++
mm/backing-dev.c | 2 +-
mm/cma.c | 4 +-
mm/cma_debug.c | 2 +-
mm/hugetlb.c | 3 +-
mm/hugetlb_cgroup.c | 2 +-
mm/hugetlb_cma.c | 2 +-
mm/kasan/report.c | 3 +-
mm/kfence/kfence_test.c | 28 +++++-----
mm/kmsan/kmsan_test.c | 6 +--
mm/memblock.c | 4 +-
mm/mempolicy.c | 18 +++----
mm/page_owner.c | 32 +++++------
mm/percpu.c | 2 +-
mm/shrinker_debug.c | 2 +-
mm/slub.c | 5 +-
mm/zswap.c | 2 +-
23 files changed, 237 insertions(+), 58 deletions(-)
Range-diff against v5:
-: ------------ > 1: dab6068bef5c vsprintf: Add [v]sprintf_trunc()
1: 2c4f793de0b8 ! 2: c801c9a1a90d vsprintf: Add [v]sprintf_end()
@@ Commit message
Signed-off-by: Alejandro Colomar <a...@kernel.org>
## include/linux/sprintf.h ##
-@@ include/linux/sprintf.h: __printf(3, 4) int snprintf(char *buf, size_t
size, const char *fmt, ...);
- __printf(3, 0) int vsnprintf(char *buf, size_t size, const char *fmt,
va_list args);
- __printf(3, 4) int scnprintf(char *buf, size_t size, const char *fmt,
...);
+@@ include/linux/sprintf.h: __printf(3, 4) int scnprintf(char *buf, size_t
size, const char *fmt, ...);
__printf(3, 0) int vscnprintf(char *buf, size_t size, const char *fmt,
va_list args);
+ __printf(3, 4) int sprintf_trunc(char *buf, size_t size, const char *fmt,
...);
+ __printf(3, 0) int vsprintf_trunc(char *buf, size_t size, const char
*fmt, va_list args);
+__printf(3, 4) char *sprintf_end(char *p, const char end[0], const char
*fmt, ...);
+__printf(3, 0) char *vsprintf_end(char *p, const char end[0], const char
*fmt, va_list args);
__printf(2, 3) __malloc char *kasprintf(gfp_t gfp, const char *fmt, ...);
@@ include/linux/sprintf.h: __printf(3, 4) int snprintf(char *buf, size_t
size, con
__printf(2, 0) const char *kvasprintf_const(gfp_t gfp, const char *fmt,
va_list args);
## lib/vsprintf.c ##
-@@ lib/vsprintf.c: int vscnprintf(char *buf, size_t size, const char *fmt,
va_list args)
+@@ lib/vsprintf.c: int vsprintf_trunc(char *buf, size_t size, const char
*fmt, va_list args)
}
- EXPORT_SYMBOL(vscnprintf);
+ EXPORT_SYMBOL(vsprintf_trunc);
+/**
+ * vsprintf_end - va_list string end-delimited print formatted
@@ lib/vsprintf.c: int vscnprintf(char *buf, size_t size, const char *fmt,
va_list
+char *vsprintf_end(char *p, const char end[0], const char *fmt, va_list
args)
+{
+ int len;
-+ size_t size;
+
+ if (unlikely(p == NULL))
+ return NULL;
+
-+ size = end - p;
-+ if (WARN_ON_ONCE(size == 0 || size > INT_MAX))
-+ return NULL;
-+
-+ len = vsnprintf(p, size, fmt, args);
-+ if (unlikely(len >= size))
++ len = vsprintf_trunc(p, end - p, fmt, args);
++ if (unlikely(len < 0))
+ return NULL;
+
+ return p + len;
@@ lib/vsprintf.c: int vscnprintf(char *buf, size_t size, const char *fmt,
va_list
/**
* snprintf - Format a string and place it in a buffer
* @buf: The buffer to place the result into
-@@ lib/vsprintf.c: int scnprintf(char *buf, size_t size, const char *fmt,
...)
+@@ lib/vsprintf.c: int sprintf_trunc(char *buf, size_t size, const char
*fmt, ...)
}
- EXPORT_SYMBOL(scnprintf);
+ EXPORT_SYMBOL(sprintf_trunc);
+/**
+ * sprintf_end - string end-delimited print formatted
6: 04c1e026a67f ! 3: 9348d5df2d9f sprintf: Add [v]sprintf_array()
@@ Commit message
array.
These macros are essentially the same as the 2-argument version of
- strscpy(), but with a formatted string, and returning a pointer to the
- terminating '\0' (or NULL, on error).
+ strscpy(), but with a formatted string.
Cc: Rasmus Villemoes <li...@rasmusvillemoes.dk>
Cc: Marco Elver <el...@google.com>
@@ include/linux/sprintf.h
#include <linux/types.h>
+#include <linux/array_size.h>
+
-+#define sprintf_array(a, fmt, ...) sprintf_end(a, ENDOF(a), fmt,
##__VA_ARGS__)
-+#define vsprintf_array(a, fmt, ap) vsprintf_end(a, ENDOF(a), fmt, ap)
++#define sprintf_array(a, fmt, ...) sprintf_trunc(a, ARRAY_SIZE(a), fmt,
##__VA_ARGS__)
++#define vsprintf_array(a, fmt, ap) vsprintf_trunc(a, ARRAY_SIZE(a), fmt,
ap)
int num_to_str(char *buf, int size, unsigned long long num, unsigned int
width);
2: 894d02b08056 = 4: 6c5d8e6012f0 stacktrace, stackdepot: Add
sprintf_end()-like variants of functions
3: 690ed4d22f57 = 5: 8a0ffc1bf43d mm: Use sprintf_end() instead of less
ergonomic APIs
4: e05c5afabb3c = 6: 37b1088dbd01 array_size.h: Add ENDOF()
5: 515445ae064d = 7: c88780354e13 mm: Fix benign off-by-one bugs
7: e53d87e684ef = 8: aa6323cbea64 mm: Use [v]sprintf_array() to avoid
specifying the array size
base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
--
2.50.0
