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 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 > > Cc: Christopher Bazley > > Cc: Sven Schnelle > > Cc: Marco Elver > > Cc: Heiko Carstens > > Cc: Tvrtko Ursulin > > Cc: "Huang, Ying" > > Cc: Andrew Morton > > Cc: Lee Schermerhorn > > Cc: Linus Torvalds > > Cc: David Rientjes > > Cc: Christophe JAILLET > > Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com> > > Cc: Chao Yu > > Cc: Vlastimil Babka > > Signed-off-by: Alejandro Colomar > > --- > > 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 > > --