Re: [RFC v1 3/3] mm: Use seprintf() instead of less ergonomic APIs

[Alejandro Colomar <alx@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 11784 bytes --]

On Sat, Jul 05, 2025 at 10:33:53PM +0200, 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.
> 
> 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

This is a typo; I meant s/do/don't/

> 	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.
> 
> Cc: Kees Cook <kees@kernel.org>
> Cc: Christopher Bazley <chris.bazley.wg14@gmail.com>
> 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/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]