While doing this, I detected some anomalies in the existing code:
mm/kfence/kfence_test.c:
- The last call to scnprintf() did increment 'cur', but it's
unused after that, so it was dead code. I've removed the dead
code in this patch.
- 'end' is calculated as
end = &expect[0][sizeof(expect[0] - 1)];
However, the '-1' doesn't seem to be necessary. When passing
$2 to scnprintf(), the size was specified as 'end - cur'.
And scnprintf() --just like snprintf(3)--, won't write more
than $2 bytes (including the null byte). That means that
scnprintf() wouldn't write more than
&expect[0][sizeof(expect[0]) - 1] - expect[0]
which simplifies to
sizeof(expect[0]) - 1
bytes. But we have sizeof(expect[0]) bytes available, so
we're wasting one byte entirely. This is a benign off-by-one
bug. The two occurrences of this bug will be fixed in a
following patch in this series.
mm/kmsan/kmsan_test.c:
The same benign off-by-one bug calculating the remaining size.
mm/mempolicy.c:
This file uses the 'p += snprintf()' anti-pattern. That will
overflow the pointer on truncation, which has undefined
behavior. Using seprintf(), this bug is fixed.
As in the previous file, here there was also dead code in the
last scnprintf() call, by incrementing a pointer that is not
used after the call. I've removed the dead code.
mm/page_owner.c:
Within print_page_owner(), there are some calls to scnprintf(),
which do report truncation. And then there are other calls to
snprintf(), where we handle errors (there are two 'goto err').
I've kept the existing error handling, as I trust it's there for
a good reason (i.e., we may want to avoid calling
print_page_owner_memcg() if we truncated before). Please review
if this amount of error handling is the right one, or if we want
to add or remove some. For seprintf(), a single test for null
after the last call is enough to detect truncation.
mm/slub.c:
Again, the 'p += snprintf()' anti-pattern. This is UB, and by
using seprintf() we've fixed the bug.
Fixes: f99e12b21b84 (2021-07-30; "kfence: add function to mask address bits")
[alx: that commit introduced dead code]
Fixes: af649773fb25 (2024-07-17; "mm/numa_balancing: teach mpol_to_str about the balancing mode")
[alx: that commit added p+=snprintf() calls, which are UB]
Fixes: 2291990ab36b (2008-04-28; "mempolicy: clean-up mpol-to-str() mempolicy formatting")
[alx: that commit changed p+=sprintf() into p+=snprintf(), which is still UB]
Fixes: 948927ee9e4f (2013-11-13; "mm, mempolicy: make mpol_to_str robust and always succeed")
[alx: that commit changes old code into p+=snprintf(), which is still UB]
[alx: that commit also produced dead code by leaving the last 'p+=...']
Fixes: d65360f22406 (2022-09-26; "mm/slub: clean up create_unique_id()")
[alx: that commit changed p+=sprintf() into p+=snprintf(), which is still UB]
Cc: Kees Cook <kees@kernel.org>
Cc: Christopher Bazley <chris.bazley.wg14@gmail.com>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Marco Elver <elver@google.com>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: "Huang, Ying" <ying.huang@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Cc: Chao Yu <chao.yu@oppo.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
mm/kfence/kfence_test.c | 24 ++++++++++++------------
mm/kmsan/kmsan_test.c | 4 ++--
mm/mempolicy.c | 18 +++++++++---------
mm/page_owner.c | 32 +++++++++++++++++---------------
mm/slub.c | 5 +++--
5 files changed, 43 insertions(+), 40 deletions(-)
diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
index 00034e37bc9f..ff734c514c03 100644
--- a/mm/kfence/kfence_test.c
+++ b/mm/kfence/kfence_test.c
@@ -113,26 +113,26 @@ static bool report_matches(const struct expect_report *r)
end = &expect[0][sizeof(expect[0]) - 1];
switch (r->type) {
case KFENCE_ERROR_OOB:
- cur += scnprintf(cur, end - cur, "BUG: KFENCE: out-of-bounds %s",
+ cur = seprintf(cur, end, "BUG: KFENCE: out-of-bounds %s",
get_access_type(r));
break;
case KFENCE_ERROR_UAF:
- cur += scnprintf(cur, end - cur, "BUG: KFENCE: use-after-free %s",
+ cur = seprintf(cur, end, "BUG: KFENCE: use-after-free %s",
get_access_type(r));
break;
case KFENCE_ERROR_CORRUPTION:
- cur += scnprintf(cur, end - cur, "BUG: KFENCE: memory corruption");
+ cur = seprintf(cur, end, "BUG: KFENCE: memory corruption");
break;
case KFENCE_ERROR_INVALID:
- cur += scnprintf(cur, end - cur, "BUG: KFENCE: invalid %s",
+ cur = seprintf(cur, end, "BUG: KFENCE: invalid %s",
get_access_type(r));
break;
case KFENCE_ERROR_INVALID_FREE:
- cur += scnprintf(cur, end - cur, "BUG: KFENCE: invalid free");
+ cur = seprintf(cur, end, "BUG: KFENCE: invalid free");
break;
}
- scnprintf(cur, end - cur, " in %pS", r->fn);
+ seprintf(cur, end, " in %pS", r->fn);
/* The exact offset won't match, remove it; also strip module name. */
cur = strchr(expect[0], '+');
if (cur)
@@ -144,26 +144,26 @@ static bool report_matches(const struct expect_report *r)
switch (r->type) {
case KFENCE_ERROR_OOB:
- cur += scnprintf(cur, end - cur, "Out-of-bounds %s at", get_access_type(r));
+ cur = seprintf(cur, end, "Out-of-bounds %s at", get_access_type(r));
addr = arch_kfence_test_address(addr);
break;
case KFENCE_ERROR_UAF:
- cur += scnprintf(cur, end - cur, "Use-after-free %s at", get_access_type(r));
+ cur = seprintf(cur, end, "Use-after-free %s at", get_access_type(r));
addr = arch_kfence_test_address(addr);
break;
case KFENCE_ERROR_CORRUPTION:
- cur += scnprintf(cur, end - cur, "Corrupted memory at");
+ cur = seprintf(cur, end, "Corrupted memory at");
break;
case KFENCE_ERROR_INVALID:
- cur += scnprintf(cur, end - cur, "Invalid %s at", get_access_type(r));
+ cur = seprintf(cur, end, "Invalid %s at", get_access_type(r));
addr = arch_kfence_test_address(addr);
break;
case KFENCE_ERROR_INVALID_FREE:
- cur += scnprintf(cur, end - cur, "Invalid free of");
+ cur = seprintf(cur, end, "Invalid free of");
break;
}
- cur += scnprintf(cur, end - cur, " 0x%p", (void *)addr);
+ seprintf(cur, end, " 0x%p", (void *)addr);
spin_lock_irqsave(&observed.lock, flags);
if (!report_available())
diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
index 9733a22c46c1..a062a46b2d24 100644
--- a/mm/kmsan/kmsan_test.c
+++ b/mm/kmsan/kmsan_test.c
@@ -107,9 +107,9 @@ static bool report_matches(const struct expect_report *r)
cur = expected_header;
end = &expected_header[sizeof(expected_header) - 1];
- cur += scnprintf(cur, end - cur, "BUG: KMSAN: %s", r->error_type);
+ cur = seprintf(cur, end, "BUG: KMSAN: %s", r->error_type);
- scnprintf(cur, end - cur, " in %s", r->symbol);
+ seprintf(cur, end, " in %s", r->symbol);
/* The exact offset won't match, remove it; also strip module name. */
cur = strchr(expected_header, '+');
if (cur)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index b28a1e6ae096..c696e4a6f4c2 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -3359,6 +3359,7 @@ int mpol_parse_str(char *str, struct mempolicy **mpol)
void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
{
char *p = buffer;
+ char *e = buffer + maxlen;
nodemask_t nodes = NODE_MASK_NONE;
unsigned short mode = MPOL_DEFAULT;
unsigned short flags = 0;
@@ -3384,33 +3385,32 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
break;
default:
WARN_ON_ONCE(1);
- snprintf(p, maxlen, "unknown");
+ seprintf(p, e, "unknown");
return;
}
- p += snprintf(p, maxlen, "%s", policy_modes[mode]);
+ p = seprintf(p, e, "%s", policy_modes[mode]);
if (flags & MPOL_MODE_FLAGS) {
- p += snprintf(p, buffer + maxlen - p, "=");
+ p = seprintf(p, e, "=");
/*
* Static and relative are mutually exclusive.
*/
if (flags & MPOL_F_STATIC_NODES)
- p += snprintf(p, buffer + maxlen - p, "static");
+ p = seprintf(p, e, "static");
else if (flags & MPOL_F_RELATIVE_NODES)
- p += snprintf(p, buffer + maxlen - p, "relative");
+ p = seprintf(p, e, "relative");
if (flags & MPOL_F_NUMA_BALANCING) {
if (!is_power_of_2(flags & MPOL_MODE_FLAGS))
- p += snprintf(p, buffer + maxlen - p, "|");
- p += snprintf(p, buffer + maxlen - p, "balancing");
+ p = seprintf(p, e, "|");
+ p = seprintf(p, e, "balancing");
}
}
if (!nodes_empty(nodes))
- p += scnprintf(p, buffer + maxlen - p, ":%*pbl",
- nodemask_pr_args(&nodes));
+ seprintf(p, e, ":%*pbl", nodemask_pr_args(&nodes));
}
#ifdef CONFIG_SYSFS
diff --git a/mm/page_owner.c b/mm/page_owner.c
index cc4a6916eec6..5811738e3320 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -496,7 +496,7 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m,
/*
* Looking for memcg information and print it out
*/
-static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
+static inline char *print_page_owner_memcg(char *p, const char end[0],
struct page *page)
{
#ifdef CONFIG_MEMCG
@@ -511,8 +511,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
goto out_unlock;
if (memcg_data & MEMCG_DATA_OBJEXTS)
- ret += scnprintf(kbuf + ret, count - ret,
- "Slab cache page\n");
+ p = seprintf(p, end, "Slab cache page\n");
memcg = page_memcg_check(page);
if (!memcg)
@@ -520,7 +519,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
online = (memcg->css.flags & CSS_ONLINE);
cgroup_name(memcg->css.cgroup, name, sizeof(name));
- ret += scnprintf(kbuf + ret, count - ret,
+ p = seprintf(p, end,
"Charged %sto %smemcg %s\n",
PageMemcgKmem(page) ? "(via objcg) " : "",
online ? "" : "offline ",
@@ -529,7 +528,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
rcu_read_unlock();
#endif /* CONFIG_MEMCG */
- return ret;
+ return p;
}
static ssize_t
@@ -538,14 +537,16 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
depot_stack_handle_t handle)
{
int ret, pageblock_mt, page_mt;
- char *kbuf;
+ char *kbuf, *p, *e;
count = min_t(size_t, count, PAGE_SIZE);
kbuf = kmalloc(count, GFP_KERNEL);
if (!kbuf)
return -ENOMEM;
- ret = scnprintf(kbuf, count,
+ p = kbuf;
+ e = kbuf + count;
+ p = seprintf(p, e,
"Page allocated via order %u, mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu ns\n",
page_owner->order, page_owner->gfp_mask,
&page_owner->gfp_mask, page_owner->pid,
@@ -555,7 +556,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
/* Print information relevant to grouping pages by mobility */
pageblock_mt = get_pageblock_migratetype(page);
page_mt = gfp_migratetype(page_owner->gfp_mask);
- ret += scnprintf(kbuf + ret, count - ret,
+ p = seprintf(p, e,
"PFN 0x%lx type %s Block %lu type %s Flags %pGp\n",
pfn,
migratetype_names[page_mt],
@@ -563,22 +564,23 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
migratetype_names[pageblock_mt],
&page->flags);
- ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0);
- if (ret >= count)
- goto err;
+ p = stack_depot_seprint(handle, p, e, 0);
+ if (p == NULL)
+ goto err; // XXX: Should we remove this error handling?
if (page_owner->last_migrate_reason != -1) {
- ret += scnprintf(kbuf + ret, count - ret,
+ p = seprintf(p, e,
"Page has been migrated, last migrate reason: %s\n",
migrate_reason_names[page_owner->last_migrate_reason]);
}
- ret = print_page_owner_memcg(kbuf, count, ret, page);
+ p = print_page_owner_memcg(p, e, page);
- ret += snprintf(kbuf + ret, count - ret, "\n");
- if (ret >= count)
+ p = seprintf(p, e, "\n");
+ if (p == NULL)
goto err;
+ ret = p - kbuf;
if (copy_to_user(buf, kbuf, ret))
ret = -EFAULT;
diff --git a/mm/slub.c b/mm/slub.c
index be8b09e09d30..b67c6ca0d0f7 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -7451,6 +7451,7 @@ static char *create_unique_id(struct kmem_cache *s)
{
char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
char *p = name;
+ char *e = name + ID_STR_LENGTH;
if (!name)
return ERR_PTR(-ENOMEM);
@@ -7475,9 +7476,9 @@ static char *create_unique_id(struct kmem_cache *s)
*p++ = 'A';
if (p != name + 1)
*p++ = '-';
- p += snprintf(p, ID_STR_LENGTH - (p - name), "%07u", s->size);
+ p = seprintf(p, e, "%07u", s->size);
- if (WARN_ON(p > name + ID_STR_LENGTH - 1)) {
+ if (WARN_ON(p == NULL)) {
kfree(name);
return ERR_PTR(-EINVAL);
}
--
2.50.0On Sun, 6 Jul 2025 at 22:06, Alejandro Colomar <alx@kernel.org> wrote:
>
> - p += snprintf(p, ID_STR_LENGTH - (p - name), "%07u", s->size);
> + p = seprintf(p, e, "%07u", s->size);
I am *really* not a fan of introducing yet another random non-standard
string function.
This 'seprintf' thing really seems to be a completely made-up thing.
Let's not go there. It just adds more confusion - it may be a simpler
interface, but it's another cogniitive load thing, and honestly, that
"beginning and end" interface is not great.
I think we'd be better off with real "character buffer" interfaces,
and they should be *named* that way, not be yet another "random
character added to the printf family".
The whole "add a random character" thing is a disease. But at least
with printf/fprintf/vprintf/vsnprintf/etc, it's a _standard_ disease,
so people hopefully know about it.
So I really *really* don't like things like seprintf(). It just makes me go WTF?
Interfaces that have worked for us are things like "seq_printf()", which
(a) has sane naming, not "add random characters"
(b) has real abstractions (in that case 'struct seq_file') rather
than adding random extra arguments to the argument list.
and we do have something like that in 'struct seq_buf'. I'm not
convinced that's the optimal interface, but I think it's *better*.
Because it does both encapsulate a proper "this is my buffer" type,
and has a proper "this is a buffer operation" function name.
So I'd *much* rather people would try to convert their uses to things
like that, than add random letter combinations.
Linus
Hi Linus, On Mon, Jul 7, 2025 at 8:17 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Sun, 6 Jul 2025 at 22:06, Alejandro Colomar <alx@kernel.org> wrote: > > > > - p += snprintf(p, ID_STR_LENGTH - (p - name), "%07u", s->size); > > + p = seprintf(p, e, "%07u", s->size); > > I am *really* not a fan of introducing yet another random non-standard > string function. > > This 'seprintf' thing really seems to be a completely made-up thing. > Let's not go there. It just adds more confusion - it may be a simpler > interface, but it's another cogniitive load thing, and honestly, that > "beginning and end" interface is not great. > > I think we'd be better off with real "character buffer" interfaces, > and they should be *named* that way, not be yet another "random > character added to the printf family". I was really interested to see this comment because I presented a design for a standard character buffer interface, "strb_t", to WG14 in summer of 2014. The latest published version of that paper is https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3306.pdf (very long) and the slides (which cover most of the important points) are https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3276.pdf I contacted you beforehand, for permission to include kasprintf and kvasprintf in the 'prior art' section of my paper. At the time, you gave me useful information about the history of those and related functions. (As an aside, Alejandro has since written a proposal to standardise a similar function named aprintf, which I support: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3630.txt ) Going back to "strb_t", I did not bother you about it again because I didn't anticipate it being used in kernel space, which has its own interfaces for most things. I'd be interested to hear what you think of it though. My intent was to make it impossible to abuse, insofar as that is possible. That led me to make choices (such as use of an incomplete struct type) that some might consider strange or overengineered. I didn't see the point in trying to replace one set of error-prone functions with another. Alejandro has put a lot of thought into his proposed seprintf function, but it still fundamentally relies on the programmer passing the right arguments and it doesn't seem to extend the functionality of snprintf in any way that I actually need. For example, some of my goals for the character buffer interface were: - A buffer should be specified using a single parameter. - Impossible to accidentally shallow-copy a buffer instead of copying a reference to it. - No aspect of character consumption delegated to character producers, e.g.: * whether to insert or overwrite. * whether to prepend, insert or append. * whether to allocate extra storage, and how to do that. - Minimize the effect of ignoring return values and not require ubiquitous error-handling. - Able to put strings directly into a buffer from any source. - Allow diverse implementations (mostly to allow tailoring to different platforms). This small program demonstrates some of those ideas: https://godbolt.org/z/66Gnre6dx It uses my ugly hacked-together prototype. Chris
On Sat, Jul 12, 2025 at 9:58 PM Christopher Bazley <chris.bazley.wg14@gmail.com> wrote: > > Hi Linus, > > On Mon, Jul 7, 2025 at 8:17 PM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > On Sun, 6 Jul 2025 at 22:06, Alejandro Colomar <alx@kernel.org> wrote: > > > > > > - p += snprintf(p, ID_STR_LENGTH - (p - name), "%07u", s->size); > > > + p = seprintf(p, e, "%07u", s->size); > > > > I am *really* not a fan of introducing yet another random non-standard > > string function. > > > > This 'seprintf' thing really seems to be a completely made-up thing. > > Let's not go there. It just adds more confusion - it may be a simpler > > interface, but it's another cogniitive load thing, and honestly, that > > "beginning and end" interface is not great. > > > > I think we'd be better off with real "character buffer" interfaces, > > and they should be *named* that way, not be yet another "random > > character added to the printf family". > > I was really interested to see this comment because I presented a > design for a standard character buffer interface, "strb_t", to WG14 in > summer of 2014. Ugh, that should have been 2024. I'm getting old! Chris
Hi Linus, On Mon, Jul 07, 2025 at 12:17:11PM -0700, Linus Torvalds wrote: > On Sun, 6 Jul 2025 at 22:06, Alejandro Colomar <alx@kernel.org> wrote: > > > > - p += snprintf(p, ID_STR_LENGTH - (p - name), "%07u", s->size); > > + p = seprintf(p, e, "%07u", s->size); > > I am *really* not a fan of introducing yet another random non-standard > string function. I am in the C Committee, and have proposed this API for standardization. I have a feeling that the committee might be open to it. > This 'seprintf' thing really seems to be a completely made-up thing. > Let's not go there. It just adds more confusion - it may be a simpler > interface, but it's another cogniitive load thing, I understand the part of your concern that relates to <https://xkcd.com/927/>. However, I've shown how in mm/, I got rid of most snprintf() and scnprintf() calls. I could even get rid of the remaining snprintf() ones; I didn't do it to avoid churn, but they're just 3, so I could do it, as a way to remove all uses of snprintf(3). I also got rid of all scnprintf() uses except for 2. Not because those two cannot be removed, but because the code was scary enough that I didn't dare touch it. I'd like someone to read it and confirm that it can be replaced. > and honestly, that > "beginning and end" interface is not great. Just look at the diffs. It is great, in terms of writing less code. In some cases, it makes sense to pass a size. Those cases are when you don't want to chain several calls. That's the case of stprintf(), and it's wrapper STPRINTF(), which calls ARRAY_SIZE() internally. But most of the time you want to chain calls, and 'end' beats 'size' there. > I think we'd be better off with real "character buffer" interfaces, > and they should be *named* that way, not be yet another "random > character added to the printf family". You might want to do that, but I doubt it's an easy change. On the other hand, this change is trivial, and can be done incrementally, without needing to modify the buffer since its inception. And you can come back later to wrap this in some API that does what you want. Nothing stops you from doing that. But this fixes several cases of UB in a few files that I've looked at, with minimal diffs. > The whole "add a random character" thing is a disease. But at least > with printf/fprintf/vprintf/vsnprintf/etc, it's a _standard_ disease, > so people hopefully know about it. seprint(2) was implemented in Plan9 many decades ago. It's not standard, because somehow Plan9 has been ignored by history, but the name has a long history. <https://plan9.io/magic/man2html/2/print> Plus, I'm making seprintf() standard (if I can convince the committee). Yesterday night, I presented the proposal to the committee, informally (via email). You can read a copy here: <https://lore.kernel.org/linux-hardening/cover.1751747518.git.alx@kernel.org/T/#m9311035d60b4595db62273844d16671601e77a50> I'll present it formally in a month, since I have a batch of proposals for the committee in the works. Have a lovely day! Alex > So I really *really* don't like things like seprintf(). It just makes me go WTF? > > Interfaces that have worked for us are things like "seq_printf()", which > > (a) has sane naming, not "add random characters" > > (b) has real abstractions (in that case 'struct seq_file') rather > than adding random extra arguments to the argument list. > > and we do have something like that in 'struct seq_buf'. I'm not > convinced that's the optimal interface, but I think it's *better*. > Because it does both encapsulate a proper "this is my buffer" type, > and has a proper "this is a buffer operation" function name. > > So I'd *much* rather people would try to convert their uses to things > like that, than add random letter combinations. > > Linus -- <https://www.alejandro-colomar.es/>
On Mon, 7 Jul 2025 at 13:29, Alejandro Colomar <alx@kernel.org> wrote:
>
> I am in the C Committee, and have proposed this API for standardization.
> I have a feeling that the committee might be open to it.
Honestly, how about fixing the serious problems with the language instead?
Get rid of the broken "strict aliasing" garbage.
Get rid of the random "undefined behavior" stuff that is literally
designed to let compilers intentionally mis-compile code.
Because as things are, "I am on the C committee" isn't a
recommendation. It's a "we have decades of bad decisions to show our
credentials".
In the kernel, I have made it very very clear that we do not use
standard C, because standard C is broken.
I stand by my "let's not add random letters to existing functions that
are already too confusing".
Linus
Hi Linus, On Mon, Jul 07, 2025 at 01:49:20PM -0700, Linus Torvalds wrote: > On Mon, 7 Jul 2025 at 13:29, Alejandro Colomar <alx@kernel.org> wrote: > > > > I am in the C Committee, and have proposed this API for standardization. > > I have a feeling that the committee might be open to it. > > Honestly, how about fixing the serious problems with the language instead? I'm doing some work on that. See the new _Countof() operator? That was my first introduction in the standard, last year. I'm working on an extension to it that I believe will make array parameters safer. > Get rid of the broken "strict aliasing" garbage. I don't feel qualified to comment on that. > Get rid of the random "undefined behavior" stuff that is literally > designed to let compilers intentionally mis-compile code. We're indeed working on that. The last committee meeting removed a large number of undefined behaviors, and turned them into mandatory diagnostics. And there's ongoing work on removing more of those. > Because as things are, "I am on the C committee" isn't a > recommendation. It's a "we have decades of bad decisions to show our > credentials". I joined in 2024 because I was fed up with the shit they were producing and wanted to influence it. You don't need to convince me. > In the kernel, I have made it very very clear that we do not use > standard C, because standard C is broken. I agree. I personally use GNU C and tend to ignore the standard. But I'm still working on improving the standard, even if just to avoid having to learn Rust (and also because GCC and glibc don't accept any improvements or fixes if they don't go through the standard, these days). Have a lovely day! Alex > I stand by my "let's not add random letters to existing functions that > are already too confusing". > > Linus -- <https://www.alejandro-colomar.es/>
On Mon, Jul 07, 2025 at 11:06:06PM +0200, Alejandro Colomar wrote: > > I stand by my "let's not add random letters to existing functions that > > are already too confusing". If the name is your main concern, we can discuss a more explicit name in the kernel. I still plan to propose it as seprintf() for standardization, and for libc, but if that reads as a letter soup to you, I guess we can call it sprintf_end() or whatever, for the kernel. Does that sound reasonable enough? What do you think about the diff itself ignoring the function name? Cheers, Alex -- <https://www.alejandro-colomar.es/>
On Mon, 7 Jul 2025 at 14:27, Alejandro Colomar <alx@kernel.org> wrote:
>
> If the name is your main concern, we can discuss a more explicit name in
> the kernel.
So as they say: "There are only two hard problems in computer science:
cache invalidation, naming and off-by-one errors".
And the *worst* model for naming is the "add random characters" (ok, I
still remember when people believed the insane "Hungarian Notation"
BS, *that* particular braindamage seems to thankfully have faded away
and was probably even worse, because it was both pointless, unreadable
_and_ caused long identifiers).
Now, we obviously tend to have the usual bike-shedding discussions
that come from naming, but my *personal* preference is to avoid the
myriad of random "does almost the same thing with different
parameters" by using generics.
This is actually something that the kernel has done for decades, with
various odd macro games - things like "get_user()" just automatically
doing the RightThing(tm) based on the size of the argument, rather
than having N different versions for different types.
So we actually have a fair number of "generics" in the kernel, and
while admittedly the header file contortions to implement them can
often be horrendous - the *use* cases tend to be fairly readable.
It's not just get_user() and friends, it's things like our
type-checking min/max macros etc. Lots of small helpers that
And while the traditional C model for this is indeed macro games with
sizeof() and other oddities, these days at least we have _Generic() to
help.
So my personal preference would actually be to not make up new names
at all, but just have the normal names DoTheRightThing(tm)
automatically.
But honestly, that works best when you have good data structure
abstraction - *not* when you pass just random "char *" pointers
around. It tends to help those kinds of _Generic() users, but even
without the use of _Generic() and friends, it helps static type
checking and makes things much less ambiguous even in general.
IOW, there's never any question about "is this string the source or
the destination?" or "is this the start or the end of the buffer", if
you just have a struct with clear naming that contains the arguments.
And while C doesn't have named arguments, it *does* have named
structure initializers, and we use them pretty religiously in the
kernel. Exactly because it helps so much both for readability and for
stability (ie it catches things when you intentionally rename members
because the semantics changed).
Linus
Hi Linus, On Mon, Jul 07, 2025 at 03:17:50PM -0700, Linus Torvalds wrote: > On Mon, 7 Jul 2025 at 14:27, Alejandro Colomar <alx@kernel.org> wrote: > > > > If the name is your main concern, we can discuss a more explicit name in > > the kernel. > > So as they say: "There are only two hard problems in computer science: > cache invalidation, naming and off-by-one errors". Indeed. And we have two of these classes here. :) > And the *worst* model for naming is the "add random characters" (ok, I > still remember when people believed the insane "Hungarian Notation" > BS, *that* particular braindamage seems to thankfully have faded away > and was probably even worse, because it was both pointless, unreadable > _and_ caused long identifiers). To be fair, one letter is enough if you're used to the name. Everything of the form s*printf() people know that the differentiating part is that single letter between 's' and 'p', and a quick look at the function prototype usually explains the rest. More than that, and it's unnecessarily noisy to my taste. But not everyone does string work all the time, so I get why you'd be less prone to liking the name. I won't press for the name. Unless you say anything, my next revision of the series will call it sprintf_end(). > Now, we obviously tend to have the usual bike-shedding discussions > that come from naming, but my *personal* preference is to avoid the > myriad of random "does almost the same thing with different > parameters" by using generics. > > This is actually something that the kernel has done for decades, with > various odd macro games - things like "get_user()" just automatically > doing the RightThing(tm) based on the size of the argument, rather > than having N different versions for different types. In this case, I wouldn't want to go that way and reuse the name snprintf(3), because the kernel implementation of snprintf(3) is non-conforming, and both the standard and the kernel snprintf() have semantics that are importantly different than this API in terms of handling errors. I think reusing the name with slightly different semantics would be prone to bugs. Anyway, sprintf_end() should be clear enough that I don't expect much bikeshedding for the name. Feel free to revisit this in the future and merge names if you don't like it; I won't complain. :) Have a lovely night! Alex P.S.: I'm not able to sign this email. > So we actually have a fair number of "generics" in the kernel, and > while admittedly the header file contortions to implement them can > often be horrendous - the *use* cases tend to be fairly readable. > > It's not just get_user() and friends, it's things like our > type-checking min/max macros etc. Lots of small helpers that > > And while the traditional C model for this is indeed macro games with > sizeof() and other oddities, these days at least we have _Generic() to > help. > > So my personal preference would actually be to not make up new names > at all, but just have the normal names DoTheRightThing(tm) > automatically. > > But honestly, that works best when you have good data structure > abstraction - *not* when you pass just random "char *" pointers > around. It tends to help those kinds of _Generic() users, but even > without the use of _Generic() and friends, it helps static type > checking and makes things much less ambiguous even in general. > > IOW, there's never any question about "is this string the source or > the destination?" or "is this the start or the end of the buffer", if > you just have a struct with clear naming that contains the arguments. > > And while C doesn't have named arguments, it *does* have named > structure initializers, and we use them pretty religiously in the > kernel. Exactly because it helps so much both for readability and for > stability (ie it catches things when you intentionally rename members > because the semantics changed). > > Linus -- <https://www.alejandro-colomar.es/>
On Mon, Jul 07, 2025 at 12:17:11PM -0700, Linus Torvalds wrote: > and we do have something like that in 'struct seq_buf'. I'm not > convinced that's the optimal interface, but I think it's *better*. > Because it does both encapsulate a proper "this is my buffer" type, > and has a proper "this is a buffer operation" function name. > > So I'd *much* rather people would try to convert their uses to things > like that, than add random letter combinations. Lifting struct membuf out of include/linux/regset.h, perhaps, and adding printf to the family?
On Mon, 7 Jul 2025 at 12:35, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Lifting struct membuf out of include/linux/regset.h, perhaps, and
> adding printf to the family?
membuf has its own problems. It doesn't remember the beginning of the
buffer, so while it's good for "fill in this buffer with streaming
data", it's pretty bad for "let's declare a buffer, fill it in, and
then use the buffer for something".
So with membuf, you can do that "fill this buffer" cleanly.
But you can't then do that "ok, it's filled, now flush it" - not
without passing in some other data (namely the original buffer data).
I don't exactly love "struct seq_buf" either - it's big and wasteful
because it has 64-bit sizes - but it at least *retains* the full
state, so you can do things like "print to this buffer" and "flush
this buffer" *without* passing around extra data.
Linus
On Mon, 7 Jul 2025 at 07:06, Alejandro Colomar <alx@kernel.org> wrote:
>
> While doing this, I detected some anomalies in the existing code:
>
> mm/kfence/kfence_test.c:
>
> - The last call to scnprintf() did increment 'cur', but it's
> unused after that, so it was dead code. I've removed the dead
> code in this patch.
That was done to be consistent with the other code for readability,
and to be clear where the next bytes should be appended (if someone
decides to append more). There is no runtime dead code, the compiler
optimizes away the assignment. But I'm indifferent, so removing the
assignment is fine if you prefer that.
Did you run the tests? Do they pass?
> - 'end' is calculated as
>
> end = &expect[0][sizeof(expect[0] - 1)];
>
> However, the '-1' doesn't seem to be necessary. When passing
> $2 to scnprintf(), the size was specified as 'end - cur'.
> And scnprintf() --just like snprintf(3)--, won't write more
> than $2 bytes (including the null byte). That means that
> scnprintf() wouldn't write more than
>
> &expect[0][sizeof(expect[0]) - 1] - expect[0]
>
> which simplifies to
>
> sizeof(expect[0]) - 1
>
> bytes. But we have sizeof(expect[0]) bytes available, so
> we're wasting one byte entirely. This is a benign off-by-one
> bug. The two occurrences of this bug will be fixed in a
> following patch in this series.
>
> mm/kmsan/kmsan_test.c:
>
> The same benign off-by-one bug calculating the remaining size.
Same - does the test pass?
> mm/mempolicy.c:
>
> This file uses the 'p += snprintf()' anti-pattern. That will
> overflow the pointer on truncation, which has undefined
> behavior. Using seprintf(), this bug is fixed.
>
> As in the previous file, here there was also dead code in the
> last scnprintf() call, by incrementing a pointer that is not
> used after the call. I've removed the dead code.
>
> mm/page_owner.c:
>
> Within print_page_owner(), there are some calls to scnprintf(),
> which do report truncation. And then there are other calls to
> snprintf(), where we handle errors (there are two 'goto err').
>
> I've kept the existing error handling, as I trust it's there for
> a good reason (i.e., we may want to avoid calling
> print_page_owner_memcg() if we truncated before). Please review
> if this amount of error handling is the right one, or if we want
> to add or remove some. For seprintf(), a single test for null
> after the last call is enough to detect truncation.
>
> mm/slub.c:
>
> Again, the 'p += snprintf()' anti-pattern. This is UB, and by
> using seprintf() we've fixed the bug.
>
> Fixes: f99e12b21b84 (2021-07-30; "kfence: add function to mask address bits")
> [alx: that commit introduced dead code]
> Fixes: af649773fb25 (2024-07-17; "mm/numa_balancing: teach mpol_to_str about the balancing mode")
> [alx: that commit added p+=snprintf() calls, which are UB]
> Fixes: 2291990ab36b (2008-04-28; "mempolicy: clean-up mpol-to-str() mempolicy formatting")
> [alx: that commit changed p+=sprintf() into p+=snprintf(), which is still UB]
> Fixes: 948927ee9e4f (2013-11-13; "mm, mempolicy: make mpol_to_str robust and always succeed")
> [alx: that commit changes old code into p+=snprintf(), which is still UB]
> [alx: that commit also produced dead code by leaving the last 'p+=...']
> Fixes: d65360f22406 (2022-09-26; "mm/slub: clean up create_unique_id()")
> [alx: that commit changed p+=sprintf() into p+=snprintf(), which is still UB]
> Cc: Kees Cook <kees@kernel.org>
> Cc: Christopher Bazley <chris.bazley.wg14@gmail.com>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: "Huang, Ying" <ying.huang@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Cc: Chao Yu <chao.yu@oppo.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> ---
> mm/kfence/kfence_test.c | 24 ++++++++++++------------
> mm/kmsan/kmsan_test.c | 4 ++--
> mm/mempolicy.c | 18 +++++++++---------
> mm/page_owner.c | 32 +++++++++++++++++---------------
> mm/slub.c | 5 +++--
> 5 files changed, 43 insertions(+), 40 deletions(-)
>
> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
> index 00034e37bc9f..ff734c514c03 100644
> --- a/mm/kfence/kfence_test.c
> +++ b/mm/kfence/kfence_test.c
> @@ -113,26 +113,26 @@ static bool report_matches(const struct expect_report *r)
> end = &expect[0][sizeof(expect[0]) - 1];
> switch (r->type) {
> case KFENCE_ERROR_OOB:
> - cur += scnprintf(cur, end - cur, "BUG: KFENCE: out-of-bounds %s",
> + cur = seprintf(cur, end, "BUG: KFENCE: out-of-bounds %s",
> get_access_type(r));
> break;
> case KFENCE_ERROR_UAF:
> - cur += scnprintf(cur, end - cur, "BUG: KFENCE: use-after-free %s",
> + cur = seprintf(cur, end, "BUG: KFENCE: use-after-free %s",
> get_access_type(r));
> break;
> case KFENCE_ERROR_CORRUPTION:
> - cur += scnprintf(cur, end - cur, "BUG: KFENCE: memory corruption");
> + cur = seprintf(cur, end, "BUG: KFENCE: memory corruption");
> break;
> case KFENCE_ERROR_INVALID:
> - cur += scnprintf(cur, end - cur, "BUG: KFENCE: invalid %s",
> + cur = seprintf(cur, end, "BUG: KFENCE: invalid %s",
> get_access_type(r));
> break;
> case KFENCE_ERROR_INVALID_FREE:
> - cur += scnprintf(cur, end - cur, "BUG: KFENCE: invalid free");
> + cur = seprintf(cur, end, "BUG: KFENCE: invalid free");
> break;
> }
>
> - scnprintf(cur, end - cur, " in %pS", r->fn);
> + seprintf(cur, end, " in %pS", r->fn);
> /* The exact offset won't match, remove it; also strip module name. */
> cur = strchr(expect[0], '+');
> if (cur)
> @@ -144,26 +144,26 @@ static bool report_matches(const struct expect_report *r)
>
> switch (r->type) {
> case KFENCE_ERROR_OOB:
> - cur += scnprintf(cur, end - cur, "Out-of-bounds %s at", get_access_type(r));
> + cur = seprintf(cur, end, "Out-of-bounds %s at", get_access_type(r));
> addr = arch_kfence_test_address(addr);
> break;
> case KFENCE_ERROR_UAF:
> - cur += scnprintf(cur, end - cur, "Use-after-free %s at", get_access_type(r));
> + cur = seprintf(cur, end, "Use-after-free %s at", get_access_type(r));
> addr = arch_kfence_test_address(addr);
> break;
> case KFENCE_ERROR_CORRUPTION:
> - cur += scnprintf(cur, end - cur, "Corrupted memory at");
> + cur = seprintf(cur, end, "Corrupted memory at");
> break;
> case KFENCE_ERROR_INVALID:
> - cur += scnprintf(cur, end - cur, "Invalid %s at", get_access_type(r));
> + cur = seprintf(cur, end, "Invalid %s at", get_access_type(r));
> addr = arch_kfence_test_address(addr);
> break;
> case KFENCE_ERROR_INVALID_FREE:
> - cur += scnprintf(cur, end - cur, "Invalid free of");
> + cur = seprintf(cur, end, "Invalid free of");
> break;
> }
>
> - cur += scnprintf(cur, end - cur, " 0x%p", (void *)addr);
> + seprintf(cur, end, " 0x%p", (void *)addr);
>
> spin_lock_irqsave(&observed.lock, flags);
> if (!report_available())
> diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
> index 9733a22c46c1..a062a46b2d24 100644
> --- a/mm/kmsan/kmsan_test.c
> +++ b/mm/kmsan/kmsan_test.c
> @@ -107,9 +107,9 @@ static bool report_matches(const struct expect_report *r)
> cur = expected_header;
> end = &expected_header[sizeof(expected_header) - 1];
>
> - cur += scnprintf(cur, end - cur, "BUG: KMSAN: %s", r->error_type);
> + cur = seprintf(cur, end, "BUG: KMSAN: %s", r->error_type);
>
> - scnprintf(cur, end - cur, " in %s", r->symbol);
> + seprintf(cur, end, " in %s", r->symbol);
> /* The exact offset won't match, remove it; also strip module name. */
> cur = strchr(expected_header, '+');
> if (cur)
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index b28a1e6ae096..c696e4a6f4c2 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -3359,6 +3359,7 @@ int mpol_parse_str(char *str, struct mempolicy **mpol)
> void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
> {
> char *p = buffer;
> + char *e = buffer + maxlen;
> nodemask_t nodes = NODE_MASK_NONE;
> unsigned short mode = MPOL_DEFAULT;
> unsigned short flags = 0;
> @@ -3384,33 +3385,32 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
> break;
> default:
> WARN_ON_ONCE(1);
> - snprintf(p, maxlen, "unknown");
> + seprintf(p, e, "unknown");
> return;
> }
>
> - p += snprintf(p, maxlen, "%s", policy_modes[mode]);
> + p = seprintf(p, e, "%s", policy_modes[mode]);
>
> if (flags & MPOL_MODE_FLAGS) {
> - p += snprintf(p, buffer + maxlen - p, "=");
> + p = seprintf(p, e, "=");
>
> /*
> * Static and relative are mutually exclusive.
> */
> if (flags & MPOL_F_STATIC_NODES)
> - p += snprintf(p, buffer + maxlen - p, "static");
> + p = seprintf(p, e, "static");
> else if (flags & MPOL_F_RELATIVE_NODES)
> - p += snprintf(p, buffer + maxlen - p, "relative");
> + p = seprintf(p, e, "relative");
>
> if (flags & MPOL_F_NUMA_BALANCING) {
> if (!is_power_of_2(flags & MPOL_MODE_FLAGS))
> - p += snprintf(p, buffer + maxlen - p, "|");
> - p += snprintf(p, buffer + maxlen - p, "balancing");
> + p = seprintf(p, e, "|");
> + p = seprintf(p, e, "balancing");
> }
> }
>
> if (!nodes_empty(nodes))
> - p += scnprintf(p, buffer + maxlen - p, ":%*pbl",
> - nodemask_pr_args(&nodes));
> + seprintf(p, e, ":%*pbl", nodemask_pr_args(&nodes));
> }
>
> #ifdef CONFIG_SYSFS
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index cc4a6916eec6..5811738e3320 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -496,7 +496,7 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m,
> /*
> * Looking for memcg information and print it out
> */
> -static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
> +static inline char *print_page_owner_memcg(char *p, const char end[0],
> struct page *page)
> {
> #ifdef CONFIG_MEMCG
> @@ -511,8 +511,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
> goto out_unlock;
>
> if (memcg_data & MEMCG_DATA_OBJEXTS)
> - ret += scnprintf(kbuf + ret, count - ret,
> - "Slab cache page\n");
> + p = seprintf(p, end, "Slab cache page\n");
>
> memcg = page_memcg_check(page);
> if (!memcg)
> @@ -520,7 +519,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
>
> online = (memcg->css.flags & CSS_ONLINE);
> cgroup_name(memcg->css.cgroup, name, sizeof(name));
> - ret += scnprintf(kbuf + ret, count - ret,
> + p = seprintf(p, end,
> "Charged %sto %smemcg %s\n",
> PageMemcgKmem(page) ? "(via objcg) " : "",
> online ? "" : "offline ",
> @@ -529,7 +528,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
> rcu_read_unlock();
> #endif /* CONFIG_MEMCG */
>
> - return ret;
> + return p;
> }
>
> static ssize_t
> @@ -538,14 +537,16 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
> depot_stack_handle_t handle)
> {
> int ret, pageblock_mt, page_mt;
> - char *kbuf;
> + char *kbuf, *p, *e;
>
> count = min_t(size_t, count, PAGE_SIZE);
> kbuf = kmalloc(count, GFP_KERNEL);
> if (!kbuf)
> return -ENOMEM;
>
> - ret = scnprintf(kbuf, count,
> + p = kbuf;
> + e = kbuf + count;
> + p = seprintf(p, e,
> "Page allocated via order %u, mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu ns\n",
> page_owner->order, page_owner->gfp_mask,
> &page_owner->gfp_mask, page_owner->pid,
> @@ -555,7 +556,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
> /* Print information relevant to grouping pages by mobility */
> pageblock_mt = get_pageblock_migratetype(page);
> page_mt = gfp_migratetype(page_owner->gfp_mask);
> - ret += scnprintf(kbuf + ret, count - ret,
> + p = seprintf(p, e,
> "PFN 0x%lx type %s Block %lu type %s Flags %pGp\n",
> pfn,
> migratetype_names[page_mt],
> @@ -563,22 +564,23 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
> migratetype_names[pageblock_mt],
> &page->flags);
>
> - ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0);
> - if (ret >= count)
> - goto err;
> + p = stack_depot_seprint(handle, p, e, 0);
> + if (p == NULL)
> + goto err; // XXX: Should we remove this error handling?
>
> if (page_owner->last_migrate_reason != -1) {
> - ret += scnprintf(kbuf + ret, count - ret,
> + p = seprintf(p, e,
> "Page has been migrated, last migrate reason: %s\n",
> migrate_reason_names[page_owner->last_migrate_reason]);
> }
>
> - ret = print_page_owner_memcg(kbuf, count, ret, page);
> + p = print_page_owner_memcg(p, e, page);
>
> - ret += snprintf(kbuf + ret, count - ret, "\n");
> - if (ret >= count)
> + p = seprintf(p, e, "\n");
> + if (p == NULL)
> goto err;
>
> + ret = p - kbuf;
> if (copy_to_user(buf, kbuf, ret))
> ret = -EFAULT;
>
> diff --git a/mm/slub.c b/mm/slub.c
> index be8b09e09d30..b67c6ca0d0f7 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -7451,6 +7451,7 @@ static char *create_unique_id(struct kmem_cache *s)
> {
> char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
> char *p = name;
> + char *e = name + ID_STR_LENGTH;
>
> if (!name)
> return ERR_PTR(-ENOMEM);
> @@ -7475,9 +7476,9 @@ static char *create_unique_id(struct kmem_cache *s)
> *p++ = 'A';
> if (p != name + 1)
> *p++ = '-';
> - p += snprintf(p, ID_STR_LENGTH - (p - name), "%07u", s->size);
> + p = seprintf(p, e, "%07u", s->size);
>
> - if (WARN_ON(p > name + ID_STR_LENGTH - 1)) {
> + if (WARN_ON(p == NULL)) {
> kfree(name);
> return ERR_PTR(-EINVAL);
> }
> --
> 2.50.0
>
Hi Marco,
On Mon, Jul 07, 2025 at 09:44:09AM +0200, Marco Elver wrote:
> On Mon, 7 Jul 2025 at 07:06, Alejandro Colomar <alx@kernel.org> wrote:
> >
> > While doing this, I detected some anomalies in the existing code:
> >
> > mm/kfence/kfence_test.c:
> >
> > - The last call to scnprintf() did increment 'cur', but it's
> > unused after that, so it was dead code. I've removed the dead
> > code in this patch.
>
> That was done to be consistent with the other code for readability,
> and to be clear where the next bytes should be appended (if someone
> decides to append more). There is no runtime dead code, the compiler
> optimizes away the assignment. But I'm indifferent, so removing the
> assignment is fine if you prefer that.
Yeah, I guessed that might be the reason. I'm fine restoring it if you
prefer it. I tend to use -Wunused-but-set-variable, but if it is not
used here and doesn't trigger, I guess it's fine to keep it.
> Did you run the tests? Do they pass?
I don't know how to run them. I've only built the kernel. If you point
me to instructions on how to run them, I'll do so. Thanks!
> > - 'end' is calculated as
> >
> > end = &expect[0][sizeof(expect[0] - 1)];
> >
> > However, the '-1' doesn't seem to be necessary. When passing
> > $2 to scnprintf(), the size was specified as 'end - cur'.
> > And scnprintf() --just like snprintf(3)--, won't write more
> > than $2 bytes (including the null byte). That means that
> > scnprintf() wouldn't write more than
> >
> > &expect[0][sizeof(expect[0]) - 1] - expect[0]
> >
> > which simplifies to
> >
> > sizeof(expect[0]) - 1
> >
> > bytes. But we have sizeof(expect[0]) bytes available, so
> > we're wasting one byte entirely. This is a benign off-by-one
> > bug. The two occurrences of this bug will be fixed in a
> > following patch in this series.
> >
> > mm/kmsan/kmsan_test.c:
> >
> > The same benign off-by-one bug calculating the remaining size.
>
>
> Same - does the test pass?
Same; built the kernel, but didn't know how to run tests.
Have a lovely day!
Alex
> > mm/mempolicy.c:
> >
> > This file uses the 'p += snprintf()' anti-pattern. That will
> > overflow the pointer on truncation, which has undefined
> > behavior. Using seprintf(), this bug is fixed.
> >
> > As in the previous file, here there was also dead code in the
> > last scnprintf() call, by incrementing a pointer that is not
> > used after the call. I've removed the dead code.
> >
> > mm/page_owner.c:
> >
> > Within print_page_owner(), there are some calls to scnprintf(),
> > which do report truncation. And then there are other calls to
> > snprintf(), where we handle errors (there are two 'goto err').
> >
> > I've kept the existing error handling, as I trust it's there for
> > a good reason (i.e., we may want to avoid calling
> > print_page_owner_memcg() if we truncated before). Please review
> > if this amount of error handling is the right one, or if we want
> > to add or remove some. For seprintf(), a single test for null
> > after the last call is enough to detect truncation.
> >
> > mm/slub.c:
> >
> > Again, the 'p += snprintf()' anti-pattern. This is UB, and by
> > using seprintf() we've fixed the bug.
> >
> > Fixes: f99e12b21b84 (2021-07-30; "kfence: add function to mask address bits")
> > [alx: that commit introduced dead code]
> > Fixes: af649773fb25 (2024-07-17; "mm/numa_balancing: teach mpol_to_str about the balancing mode")
> > [alx: that commit added p+=snprintf() calls, which are UB]
> > Fixes: 2291990ab36b (2008-04-28; "mempolicy: clean-up mpol-to-str() mempolicy formatting")
> > [alx: that commit changed p+=sprintf() into p+=snprintf(), which is still UB]
> > Fixes: 948927ee9e4f (2013-11-13; "mm, mempolicy: make mpol_to_str robust and always succeed")
> > [alx: that commit changes old code into p+=snprintf(), which is still UB]
> > [alx: that commit also produced dead code by leaving the last 'p+=...']
> > Fixes: d65360f22406 (2022-09-26; "mm/slub: clean up create_unique_id()")
> > [alx: that commit changed p+=sprintf() into p+=snprintf(), which is still UB]
> > Cc: Kees Cook <kees@kernel.org>
> > Cc: Christopher Bazley <chris.bazley.wg14@gmail.com>
> > Cc: Sven Schnelle <svens@linux.ibm.com>
> > Cc: Marco Elver <elver@google.com>
> > Cc: Heiko Carstens <hca@linux.ibm.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > Cc: "Huang, Ying" <ying.huang@intel.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> > Cc: Chao Yu <chao.yu@oppo.com>
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Signed-off-by: Alejandro Colomar <alx@kernel.org>
> > ---
> > mm/kfence/kfence_test.c | 24 ++++++++++++------------
> > mm/kmsan/kmsan_test.c | 4 ++--
> > mm/mempolicy.c | 18 +++++++++---------
> > mm/page_owner.c | 32 +++++++++++++++++---------------
> > mm/slub.c | 5 +++--
> > 5 files changed, 43 insertions(+), 40 deletions(-)
> >
> > diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
> > index 00034e37bc9f..ff734c514c03 100644
> > --- a/mm/kfence/kfence_test.c
> > +++ b/mm/kfence/kfence_test.c
> > @@ -113,26 +113,26 @@ static bool report_matches(const struct expect_report *r)
> > end = &expect[0][sizeof(expect[0]) - 1];
> > switch (r->type) {
> > case KFENCE_ERROR_OOB:
> > - cur += scnprintf(cur, end - cur, "BUG: KFENCE: out-of-bounds %s",
> > + cur = seprintf(cur, end, "BUG: KFENCE: out-of-bounds %s",
> > get_access_type(r));
> > break;
> > case KFENCE_ERROR_UAF:
> > - cur += scnprintf(cur, end - cur, "BUG: KFENCE: use-after-free %s",
> > + cur = seprintf(cur, end, "BUG: KFENCE: use-after-free %s",
> > get_access_type(r));
> > break;
> > case KFENCE_ERROR_CORRUPTION:
> > - cur += scnprintf(cur, end - cur, "BUG: KFENCE: memory corruption");
> > + cur = seprintf(cur, end, "BUG: KFENCE: memory corruption");
> > break;
> > case KFENCE_ERROR_INVALID:
> > - cur += scnprintf(cur, end - cur, "BUG: KFENCE: invalid %s",
> > + cur = seprintf(cur, end, "BUG: KFENCE: invalid %s",
> > get_access_type(r));
> > break;
> > case KFENCE_ERROR_INVALID_FREE:
> > - cur += scnprintf(cur, end - cur, "BUG: KFENCE: invalid free");
> > + cur = seprintf(cur, end, "BUG: KFENCE: invalid free");
> > break;
> > }
> >
> > - scnprintf(cur, end - cur, " in %pS", r->fn);
> > + seprintf(cur, end, " in %pS", r->fn);
> > /* The exact offset won't match, remove it; also strip module name. */
> > cur = strchr(expect[0], '+');
> > if (cur)
> > @@ -144,26 +144,26 @@ static bool report_matches(const struct expect_report *r)
> >
> > switch (r->type) {
> > case KFENCE_ERROR_OOB:
> > - cur += scnprintf(cur, end - cur, "Out-of-bounds %s at", get_access_type(r));
> > + cur = seprintf(cur, end, "Out-of-bounds %s at", get_access_type(r));
> > addr = arch_kfence_test_address(addr);
> > break;
> > case KFENCE_ERROR_UAF:
> > - cur += scnprintf(cur, end - cur, "Use-after-free %s at", get_access_type(r));
> > + cur = seprintf(cur, end, "Use-after-free %s at", get_access_type(r));
> > addr = arch_kfence_test_address(addr);
> > break;
> > case KFENCE_ERROR_CORRUPTION:
> > - cur += scnprintf(cur, end - cur, "Corrupted memory at");
> > + cur = seprintf(cur, end, "Corrupted memory at");
> > break;
> > case KFENCE_ERROR_INVALID:
> > - cur += scnprintf(cur, end - cur, "Invalid %s at", get_access_type(r));
> > + cur = seprintf(cur, end, "Invalid %s at", get_access_type(r));
> > addr = arch_kfence_test_address(addr);
> > break;
> > case KFENCE_ERROR_INVALID_FREE:
> > - cur += scnprintf(cur, end - cur, "Invalid free of");
> > + cur = seprintf(cur, end, "Invalid free of");
> > break;
> > }
> >
> > - cur += scnprintf(cur, end - cur, " 0x%p", (void *)addr);
> > + seprintf(cur, end, " 0x%p", (void *)addr);
> >
> > spin_lock_irqsave(&observed.lock, flags);
> > if (!report_available())
> > diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
> > index 9733a22c46c1..a062a46b2d24 100644
> > --- a/mm/kmsan/kmsan_test.c
> > +++ b/mm/kmsan/kmsan_test.c
> > @@ -107,9 +107,9 @@ static bool report_matches(const struct expect_report *r)
> > cur = expected_header;
> > end = &expected_header[sizeof(expected_header) - 1];
> >
> > - cur += scnprintf(cur, end - cur, "BUG: KMSAN: %s", r->error_type);
> > + cur = seprintf(cur, end, "BUG: KMSAN: %s", r->error_type);
> >
> > - scnprintf(cur, end - cur, " in %s", r->symbol);
> > + seprintf(cur, end, " in %s", r->symbol);
> > /* The exact offset won't match, remove it; also strip module name. */
> > cur = strchr(expected_header, '+');
> > if (cur)
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index b28a1e6ae096..c696e4a6f4c2 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -3359,6 +3359,7 @@ int mpol_parse_str(char *str, struct mempolicy **mpol)
> > void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
> > {
> > char *p = buffer;
> > + char *e = buffer + maxlen;
> > nodemask_t nodes = NODE_MASK_NONE;
> > unsigned short mode = MPOL_DEFAULT;
> > unsigned short flags = 0;
> > @@ -3384,33 +3385,32 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
> > break;
> > default:
> > WARN_ON_ONCE(1);
> > - snprintf(p, maxlen, "unknown");
> > + seprintf(p, e, "unknown");
> > return;
> > }
> >
> > - p += snprintf(p, maxlen, "%s", policy_modes[mode]);
> > + p = seprintf(p, e, "%s", policy_modes[mode]);
> >
> > if (flags & MPOL_MODE_FLAGS) {
> > - p += snprintf(p, buffer + maxlen - p, "=");
> > + p = seprintf(p, e, "=");
> >
> > /*
> > * Static and relative are mutually exclusive.
> > */
> > if (flags & MPOL_F_STATIC_NODES)
> > - p += snprintf(p, buffer + maxlen - p, "static");
> > + p = seprintf(p, e, "static");
> > else if (flags & MPOL_F_RELATIVE_NODES)
> > - p += snprintf(p, buffer + maxlen - p, "relative");
> > + p = seprintf(p, e, "relative");
> >
> > if (flags & MPOL_F_NUMA_BALANCING) {
> > if (!is_power_of_2(flags & MPOL_MODE_FLAGS))
> > - p += snprintf(p, buffer + maxlen - p, "|");
> > - p += snprintf(p, buffer + maxlen - p, "balancing");
> > + p = seprintf(p, e, "|");
> > + p = seprintf(p, e, "balancing");
> > }
> > }
> >
> > if (!nodes_empty(nodes))
> > - p += scnprintf(p, buffer + maxlen - p, ":%*pbl",
> > - nodemask_pr_args(&nodes));
> > + seprintf(p, e, ":%*pbl", nodemask_pr_args(&nodes));
> > }
> >
> > #ifdef CONFIG_SYSFS
> > diff --git a/mm/page_owner.c b/mm/page_owner.c
> > index cc4a6916eec6..5811738e3320 100644
> > --- a/mm/page_owner.c
> > +++ b/mm/page_owner.c
> > @@ -496,7 +496,7 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m,
> > /*
> > * Looking for memcg information and print it out
> > */
> > -static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
> > +static inline char *print_page_owner_memcg(char *p, const char end[0],
> > struct page *page)
> > {
> > #ifdef CONFIG_MEMCG
> > @@ -511,8 +511,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
> > goto out_unlock;
> >
> > if (memcg_data & MEMCG_DATA_OBJEXTS)
> > - ret += scnprintf(kbuf + ret, count - ret,
> > - "Slab cache page\n");
> > + p = seprintf(p, end, "Slab cache page\n");
> >
> > memcg = page_memcg_check(page);
> > if (!memcg)
> > @@ -520,7 +519,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
> >
> > online = (memcg->css.flags & CSS_ONLINE);
> > cgroup_name(memcg->css.cgroup, name, sizeof(name));
> > - ret += scnprintf(kbuf + ret, count - ret,
> > + p = seprintf(p, end,
> > "Charged %sto %smemcg %s\n",
> > PageMemcgKmem(page) ? "(via objcg) " : "",
> > online ? "" : "offline ",
> > @@ -529,7 +528,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
> > rcu_read_unlock();
> > #endif /* CONFIG_MEMCG */
> >
> > - return ret;
> > + return p;
> > }
> >
> > static ssize_t
> > @@ -538,14 +537,16 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
> > depot_stack_handle_t handle)
> > {
> > int ret, pageblock_mt, page_mt;
> > - char *kbuf;
> > + char *kbuf, *p, *e;
> >
> > count = min_t(size_t, count, PAGE_SIZE);
> > kbuf = kmalloc(count, GFP_KERNEL);
> > if (!kbuf)
> > return -ENOMEM;
> >
> > - ret = scnprintf(kbuf, count,
> > + p = kbuf;
> > + e = kbuf + count;
> > + p = seprintf(p, e,
> > "Page allocated via order %u, mask %#x(%pGg), pid %d, tgid %d (%s), ts %llu ns\n",
> > page_owner->order, page_owner->gfp_mask,
> > &page_owner->gfp_mask, page_owner->pid,
> > @@ -555,7 +556,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
> > /* Print information relevant to grouping pages by mobility */
> > pageblock_mt = get_pageblock_migratetype(page);
> > page_mt = gfp_migratetype(page_owner->gfp_mask);
> > - ret += scnprintf(kbuf + ret, count - ret,
> > + p = seprintf(p, e,
> > "PFN 0x%lx type %s Block %lu type %s Flags %pGp\n",
> > pfn,
> > migratetype_names[page_mt],
> > @@ -563,22 +564,23 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
> > migratetype_names[pageblock_mt],
> > &page->flags);
> >
> > - ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0);
> > - if (ret >= count)
> > - goto err;
> > + p = stack_depot_seprint(handle, p, e, 0);
> > + if (p == NULL)
> > + goto err; // XXX: Should we remove this error handling?
> >
> > if (page_owner->last_migrate_reason != -1) {
> > - ret += scnprintf(kbuf + ret, count - ret,
> > + p = seprintf(p, e,
> > "Page has been migrated, last migrate reason: %s\n",
> > migrate_reason_names[page_owner->last_migrate_reason]);
> > }
> >
> > - ret = print_page_owner_memcg(kbuf, count, ret, page);
> > + p = print_page_owner_memcg(p, e, page);
> >
> > - ret += snprintf(kbuf + ret, count - ret, "\n");
> > - if (ret >= count)
> > + p = seprintf(p, e, "\n");
> > + if (p == NULL)
> > goto err;
> >
> > + ret = p - kbuf;
> > if (copy_to_user(buf, kbuf, ret))
> > ret = -EFAULT;
> >
> > diff --git a/mm/slub.c b/mm/slub.c
> > index be8b09e09d30..b67c6ca0d0f7 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -7451,6 +7451,7 @@ static char *create_unique_id(struct kmem_cache *s)
> > {
> > char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
> > char *p = name;
> > + char *e = name + ID_STR_LENGTH;
> >
> > if (!name)
> > return ERR_PTR(-ENOMEM);
> > @@ -7475,9 +7476,9 @@ static char *create_unique_id(struct kmem_cache *s)
> > *p++ = 'A';
> > if (p != name + 1)
> > *p++ = '-';
> > - p += snprintf(p, ID_STR_LENGTH - (p - name), "%07u", s->size);
> > + p = seprintf(p, e, "%07u", s->size);
> >
> > - if (WARN_ON(p > name + ID_STR_LENGTH - 1)) {
> > + if (WARN_ON(p == NULL)) {
> > kfree(name);
> > return ERR_PTR(-EINVAL);
> > }
> > --
> > 2.50.0
> >
--
<https://www.alejandro-colomar.es/>
On Mon, 7 Jul 2025 at 16:39, Alejandro Colomar <alx@kernel.org> wrote: > > Hi Marco, > > On Mon, Jul 07, 2025 at 09:44:09AM +0200, Marco Elver wrote: > > On Mon, 7 Jul 2025 at 07:06, Alejandro Colomar <alx@kernel.org> wrote: > > > > > > While doing this, I detected some anomalies in the existing code: > > > > > > mm/kfence/kfence_test.c: > > > > > > - The last call to scnprintf() did increment 'cur', but it's > > > unused after that, so it was dead code. I've removed the dead > > > code in this patch. > > > > That was done to be consistent with the other code for readability, > > and to be clear where the next bytes should be appended (if someone > > decides to append more). There is no runtime dead code, the compiler > > optimizes away the assignment. But I'm indifferent, so removing the > > assignment is fine if you prefer that. > > Yeah, I guessed that might be the reason. I'm fine restoring it if you > prefer it. I tend to use -Wunused-but-set-variable, but if it is not > used here and doesn't trigger, I guess it's fine to keep it. Feel free to make it warning-free, I guess that's useful. > > Did you run the tests? Do they pass? > > I don't know how to run them. I've only built the kernel. If you point > me to instructions on how to run them, I'll do so. Thanks! Should just be CONFIG_KFENCE_KUNIT_TEST=y -- then boot kernel and check that the test reports "ok". Thanks, -- marco
Hi Marco, On Mon, Jul 07, 2025 at 04:58:53PM +0200, Marco Elver wrote: > Feel free to make it warning-free, I guess that's useful. Thanks! > > > Did you run the tests? Do they pass? > > > > I don't know how to run them. I've only built the kernel. If you point > > me to instructions on how to run them, I'll do so. Thanks! > > Should just be CONFIG_KFENCE_KUNIT_TEST=y -- then boot kernel and > check that the test reports "ok". Hmmm, I can't see the results. Did I miss anything? alx@debian:~$ uname -a Linux debian 6.15.0-seprintf-mm+ #5 SMP PREEMPT_DYNAMIC Mon Jul 7 19:16:40 CEST 2025 x86_64 GNU/Linux alx@debian:~$ cat /boot/config-6.15.0-seprintf-mm+ | grep KFENCE CONFIG_HAVE_ARCH_KFENCE=y CONFIG_KFENCE=y CONFIG_KFENCE_SAMPLE_INTERVAL=0 CONFIG_KFENCE_NUM_OBJECTS=255 # CONFIG_KFENCE_DEFERRABLE is not set # CONFIG_KFENCE_STATIC_KEYS is not set CONFIG_KFENCE_STRESS_TEST_FAULTS=0 CONFIG_KFENCE_KUNIT_TEST=y alx@debian:~$ sudo dmesg | grep -i kfence alx@debian:~$ I see a lot of new stuff in dmesg, but nothing with 'kfence' in it. Cheers, Alex -- <https://www.alejandro-colomar.es/>
On Mon, 7 Jul 2025 at 20:51, Alejandro Colomar <alx@kernel.org> wrote:
>
> Hi Marco,
>
> On Mon, Jul 07, 2025 at 04:58:53PM +0200, Marco Elver wrote:
> > Feel free to make it warning-free, I guess that's useful.
>
> Thanks!
>
> > > > Did you run the tests? Do they pass?
> > >
> > > I don't know how to run them. I've only built the kernel. If you point
> > > me to instructions on how to run them, I'll do so. Thanks!
> >
> > Should just be CONFIG_KFENCE_KUNIT_TEST=y -- then boot kernel and
> > check that the test reports "ok".
>
> Hmmm, I can't see the results. Did I miss anything?
>
> alx@debian:~$ uname -a
> Linux debian 6.15.0-seprintf-mm+ #5 SMP PREEMPT_DYNAMIC Mon Jul 7 19:16:40 CEST 2025 x86_64 GNU/Linux
> alx@debian:~$ cat /boot/config-6.15.0-seprintf-mm+ | grep KFENCE
> CONFIG_HAVE_ARCH_KFENCE=y
> CONFIG_KFENCE=y
> CONFIG_KFENCE_SAMPLE_INTERVAL=0
^^ This means KFENCE is off.
Not sure why it's 0 (distro default config?), but if you switch it to
something like:
CONFIG_KFENCE_SAMPLE_INTERVAL=10
The test should run. Alternatively set 'kfence.sample_interval=10' as
boot param.
> CONFIG_KFENCE_NUM_OBJECTS=255
> # CONFIG_KFENCE_DEFERRABLE is not set
> # CONFIG_KFENCE_STATIC_KEYS is not set
> CONFIG_KFENCE_STRESS_TEST_FAULTS=0
> CONFIG_KFENCE_KUNIT_TEST=y
> alx@debian:~$ sudo dmesg | grep -i kfence
> alx@debian:~$
>
> I see a lot of new stuff in dmesg, but nothing with 'kfence' in it.
>
>
> Cheers,
> Alex
>
> --
> <https://www.alejandro-colomar.es/>
Hi Marco, On Mon, Jul 07, 2025 at 09:08:29PM +0200, Marco Elver wrote: > > > > > Did you run the tests? Do they pass? > > > > > > > > I don't know how to run them. I've only built the kernel. If you point > > > > me to instructions on how to run them, I'll do so. Thanks! > > > > > > Should just be CONFIG_KFENCE_KUNIT_TEST=y -- then boot kernel and > > > check that the test reports "ok". > > > > Hmmm, I can't see the results. Did I miss anything? > > > > alx@debian:~$ uname -a > > Linux debian 6.15.0-seprintf-mm+ #5 SMP PREEMPT_DYNAMIC Mon Jul 7 19:16:40 CEST 2025 x86_64 GNU/Linux > > alx@debian:~$ cat /boot/config-6.15.0-seprintf-mm+ | grep KFENCE > > CONFIG_HAVE_ARCH_KFENCE=y > > CONFIG_KFENCE=y > > CONFIG_KFENCE_SAMPLE_INTERVAL=0 > > ^^ This means KFENCE is off. > > Not sure why it's 0 (distro default config?), but if you switch it to > something like: Yup, Debian default config plus what you told me. :) > > CONFIG_KFENCE_SAMPLE_INTERVAL=10 Thanks! Now I see the tests. I see no regressions. I've tested both v6.15 and my branch, and see no differences: This was generated with the kernel built from my branch: $ sudo dmesg | grep -inC2 kfence | sed 's/^....//' > tmp/log_after This was generated with a v6.15 kernel with the same exact config: $ sudo dmesg | grep -inC2 kfence | sed 's/^....//' > tmp/log_before And here's a diff, ignoring some numbers that were easy to filter out: $ diff -U999 \ <(cat tmp/log_before \ | sed 's/0x[0-9a-f]*/0x????/g' \ | sed 's/[[:digit:]]\.[[:digit:]]\+/?.?/g' \ | sed 's/#[[:digit:]]\+/#???/g') \ <(cat tmp/log_after \ | sed 's/0x[0-9a-f]*/0x????/g' \ | sed 's/[[:digit:]]\.[[:digit:]]\+/?.?/g' \ | sed 's/#[[:digit:]]\+/#???/g'); --- /dev/fd/63 2025-07-07 22:47:37.395608776 +0200 +++ /dev/fd/62 2025-07-07 22:47:37.395608776 +0200 @@ -1,303 +1,303 @@ [ ?.?] NR_IRQS: 524544, nr_irqs: 1096, preallocated irqs: 16 [ ?.?] rcu: srcu_init: Setting srcu_struct sizes based on contention. [ ?.?] kfence: initialized - using 2097152 bytes for 255 objects at 0x????(____ptrval____)-0x????(____ptrval____) [ ?.?] Console: colour dummy device 80x???? [ ?.?] printk: legacy console [tty0] enabled -- [ ?.?] ok 7 sysctl_test [ ?.?] KTAP version 1 [ ?.?] # Subtest: kfence [ ?.?] 1..27 [ ?.?] # test_out_of_bounds_read: test_alloc: size=32, gfp=cc0, policy=left, cache=0 [ ?.?] ================================================================== [ ?.?] BUG: KFENCE: out-of-bounds read in test_out_of_bounds_read+0x????/0x???? [ ?.?] Out-of-bounds read at 0x???? (1B left of kfence-#???): [ ?.?] test_out_of_bounds_read+0x????/0x???? [ ?.?] kunit_try_run_case+0x????/0x???? -- [ ?.?] ret_from_fork_asm+0x????/0x???? [ ?.?] kfence-#???: 0x????-0x????, size=32, cache=kmalloc-32 -[ ?.?] allocated by task 281 on cpu 6 at ?.?s (?.?s ago): +[ ?.?] allocated by task 286 on cpu 8 at ?.?s (?.?s ago): -- [ ?.?] # test_out_of_bounds_read: test_alloc: size=32, gfp=cc0, policy=right, cache=0 [ ?.?] ================================================================== [ ?.?] BUG: KFENCE: out-of-bounds read in test_out_of_bounds_read.cold+0x????/0x???? [ ?.?] Out-of-bounds read at 0x???? (32B right of kfence-#???): [ ?.?] test_out_of_bounds_read.cold+0x????/0x???? [ ?.?] kunit_try_run_case+0x????/0x???? -- [ ?.?] ret_from_fork_asm+0x????/0x???? [ ?.?] kfence-#???: 0x????-0x????, size=32, cache=kmalloc-32 -[ ?.?] allocated by task 281 on cpu 6 at ?.?s (?.?s ago): +[ ?.?] allocated by task 286 on cpu 11 at ?.?s (?.?s ago): -- [ ?.?] # test_out_of_bounds_read-memcache: test_alloc: size=32, gfp=cc0, policy=left, cache=1 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: out-of-bounds read in test_out_of_bounds_read+0x????/0x???? - :[ ?.?] Out-of-bounds read at 0x???? (1B left of kfence-#???): -[ ?.?] test_out_of_bounds_read+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=test - --[ ?.?] allocated by task 284 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 289 on cpu 8 at ?.?s (?.?s ago): -- -[ ?.?] # test_out_of_bounds_read-memcache: test_alloc: size=32, gfp=cc0, policy=right, cache=1 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: out-of-bounds read in test_out_of_bounds_read.cold+0x????/0x???? - :[ ?.?] Out-of-bounds read at 0x???? (32B right of kfence-#???): -[ ?.?] test_out_of_bounds_read.cold+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=test - --[ ?.?] allocated by task 284 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 289 on cpu 8 at ?.?s (?.?s ago): -- -[ ?.?] # test_out_of_bounds_write: test_alloc: size=32, gfp=cc0, policy=left, cache=0 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: out-of-bounds write in test_out_of_bounds_write+0x????/0x???? - :[ ?.?] Out-of-bounds write at 0x???? (1B left of kfence-#???): -[ ?.?] test_out_of_bounds_write+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=kmalloc-32 - --[ ?.?] allocated by task 288 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 291 on cpu 6 at ?.?s (?.?s ago): -- --[ ?.?] # test_out_of_bounds_write-memcache: test_alloc: size=32, gfp=cc0, policy=left, cache=1 -[ ?.?] ================================================================== +-[ ?.?] clocksource: tsc: mask: 0x???? max_cycles: 0x????, max_idle_ns: 881590599626 ns :[ ?.?] BUG: KFENCE: out-of-bounds write in test_out_of_bounds_write+0x????/0x???? - :[ ?.?] Out-of-bounds write at 0x???? (1B left of kfence-#???): -[ ?.?] test_out_of_bounds_write+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=test - --[ ?.?] allocated by task 290 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 293 on cpu 10 at ?.?s (?.?s ago): -- -[ ?.?] # test_use_after_free_read: test_alloc: size=32, gfp=cc0, policy=any, cache=0 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: use-after-free read in test_use_after_free_read+0x????/0x???? - :[ ?.?] Use-after-free read at 0x???? (in kfence-#???): -[ ?.?] test_use_after_free_read+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=kmalloc-32 - --[ ?.?] allocated by task 292 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 296 on cpu 10 at ?.?s (?.?s ago): -- -[ ?.?] # test_use_after_free_read-memcache: test_alloc: size=32, gfp=cc0, policy=any, cache=1 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: use-after-free read in test_use_after_free_read+0x????/0x???? - :[ ?.?] Use-after-free read at 0x???? (in kfence-#???): -[ ?.?] test_use_after_free_read+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=test - --[ ?.?] allocated by task 294 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 298 on cpu 10 at ?.?s (?.?s ago): -- -[ ?.?] # test_double_free: test_alloc: size=32, gfp=cc0, policy=any, cache=0 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: invalid free in test_double_free+0x????/0x???? - :[ ?.?] Invalid free of 0x???? (in kfence-#???): -[ ?.?] test_double_free+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=kmalloc-32 - --[ ?.?] allocated by task 300 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 304 on cpu 6 at ?.?s (?.?s ago): -- -[ ?.?] # test_double_free-memcache: test_alloc: size=32, gfp=cc0, policy=any, cache=1 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: invalid free in test_double_free+0x????/0x???? - :[ ?.?] Invalid free of 0x???? (in kfence-#???): -[ ?.?] test_double_free+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=test - --[ ?.?] allocated by task 302 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 306 on cpu 8 at ?.?s (?.?s ago): -- -[ ?.?] # test_invalid_addr_free: test_alloc: size=32, gfp=cc0, policy=any, cache=0 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: invalid free in test_invalid_addr_free+0x????/0x???? - :[ ?.?] Invalid free of 0x???? (in kfence-#???): -[ ?.?] test_invalid_addr_free+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=kmalloc-32 - --[ ?.?] allocated by task 304 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 308 on cpu 8 at ?.?s (?.?s ago): -- -[ ?.?] # test_invalid_addr_free-memcache: test_alloc: size=32, gfp=cc0, policy=any, cache=1 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: invalid free in test_invalid_addr_free+0x????/0x???? - :[ ?.?] Invalid free of 0x???? (in kfence-#???): -[ ?.?] test_invalid_addr_free+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=test - --[ ?.?] allocated by task 306 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 310 on cpu 8 at ?.?s (?.?s ago): -- -[ ?.?] # test_corruption: test_alloc: size=32, gfp=cc0, policy=left, cache=0 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: memory corruption in test_corruption+0x????/0x???? - :[ ?.?] Corrupted memory at 0x???? [ ! . . . . . . . . . . . . . . . ] (in kfence-#???): -[ ?.?] test_corruption+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=kmalloc-32 - --[ ?.?] allocated by task 308 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 312 on cpu 6 at ?.?s (?.?s ago): -- -[ ?.?] # test_corruption: test_alloc: size=32, gfp=cc0, policy=right, cache=0 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: memory corruption in test_corruption+0x????/0x???? - :[ ?.?] Corrupted memory at 0x???? [ ! ] (in kfence-#???): -[ ?.?] test_corruption+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=kmalloc-32 - --[ ?.?] allocated by task 308 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 312 on cpu 6 at ?.?s (?.?s ago): -- -[ ?.?] # test_corruption-memcache: test_alloc: size=32, gfp=cc0, policy=left, cache=1 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: memory corruption in test_corruption+0x????/0x???? - :[ ?.?] Corrupted memory at 0x???? [ ! . . . . . . . . . . . . . . . ] (in kfence-#???): -[ ?.?] test_corruption+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=test - --[ ?.?] allocated by task 310 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 314 on cpu 6 at ?.?s (?.?s ago): -- -[ ?.?] # test_corruption-memcache: test_alloc: size=32, gfp=cc0, policy=right, cache=1 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: memory corruption in test_corruption+0x????/0x???? - :[ ?.?] Corrupted memory at 0x???? [ ! ] (in kfence-#???): -[ ?.?] test_corruption+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=test - --[ ?.?] allocated by task 310 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 314 on cpu 6 at ?.?s (?.?s ago): -- -[ ?.?] # test_kmalloc_aligned_oob_read: test_alloc: size=73, gfp=cc0, policy=right, cache=0 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: out-of-bounds read in test_kmalloc_aligned_oob_read+0x????/0x???? - :[ ?.?] Out-of-bounds read at 0x???? (105B right of kfence-#???): -[ ?.?] test_kmalloc_aligned_oob_read+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=73, cache=kmalloc-96 - --[ ?.?] allocated by task 320 on cpu 10 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 326 on cpu 6 at ?.?s (?.?s ago): -- -[ ?.?] # test_kmalloc_aligned_oob_write: test_alloc: size=73, gfp=cc0, policy=right, cache=0 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: memory corruption in test_kmalloc_aligned_oob_write+0x????/0x???? - :[ ?.?] Corrupted memory at 0x???? [ ! . . . . . . . . . . . . . . . ] (in kfence-#???): -[ ?.?] test_kmalloc_aligned_oob_write+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=73, cache=kmalloc-96 - --[ ?.?] allocated by task 326 on cpu 8 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 328 on cpu 4 at ?.?s (?.?s ago): -- -[ ?.?] ok 22 test_memcache_ctor -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: invalid read in test_invalid_access+0x????/0x???? - -[ ?.?] Invalid read at 0x????: -- -[ ?.?] # test_memcache_typesafe_by_rcu: test_alloc: size=32, gfp=cc0, policy=any, cache=1 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: use-after-free read in test_memcache_typesafe_by_rcu.cold+0x????/0x???? - :[ ?.?] Use-after-free read at 0x???? (in kfence-#???): -[ ?.?] test_memcache_typesafe_by_rcu.cold+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=test - --[ ?.?] allocated by task 336 on cpu 6 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 338 on cpu 10 at ?.?s (?.?s ago): -- -[ ?.?] # test_krealloc: test_alloc: size=32, gfp=cc0, policy=any, cache=0 -[ ?.?] ================================================================== :[ ?.?] BUG: KFENCE: use-after-free read in test_krealloc+0x????/0x???? - :[ ?.?] Use-after-free read at 0x???? (in kfence-#???): -[ ?.?] test_krealloc+0x????/0x???? -[ ?.?] kunit_try_run_case+0x????/0x???? -- -[ ?.?] ret_from_fork_asm+0x????/0x???? - :[ ?.?] kfence-#???: 0x????-0x????, size=32, cache=kmalloc-32 - --[ ?.?] allocated by task 338 on cpu 4 at ?.?s (?.?s ago): +-[ ?.?] allocated by task 340 on cpu 6 at ?.?s (?.?s ago): -- -[ ?.?] # test_memcache_alloc_bulk: setup_test_cache: size=32, ctor=0x???? -[ ?.?] ok 27 test_memcache_alloc_bulk :[ ?.?] # kfence: pass:25 fail:0 skip:2 total:27 -[ ?.?] # Totals: pass:25 fail:0 skip:2 total:27 :[ ?.?] ok 8 kfence -[ ?.?] KTAP version 1 -[ ?.?] # Subtest: damon If you'd like me to grep for something more specific, please let me know. Cheers, Alex -- <https://www.alejandro-colomar.es/>
© 2016 - 2025 Red Hat, Inc.