linux-kernel - Re: [PATCH v2 3/5] drivers/core: simplify variadic args handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aS8K5DnumUA6XUs9@pathway.suse.cz>
Date: Tue, 2 Dec 2025 16:51:00 +0100
From: Petr Mladek <pmladek@...e.com>
To: Andrzej Hajda <andrzej.hajda@...el.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Vlastimil Babka <vbabka@...e.cz>,
	Suren Baghdasaryan <surenb@...gle.com>,
	Michal Hocko <mhocko@...e.com>,
	Brendan Jackman <jackmanb@...gle.com>,
	Johannes Weiner <hannes@...xchg.org>, Zi Yan <ziy@...dia.com>,
	Christoph Lameter <cl@...two.org>,
	David Rientjes <rientjes@...gle.com>,
	Roman Gushchin <roman.gushchin@...ux.dev>,
	Harry Yoo <harry.yoo@...cle.com>, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH v2 3/5] drivers/core: simplify variadic args handling

I am adding Andy and Rasmus into Cc who are active vsprintf-related
code reviewers...

You might see the entire patchset at
https://lore.kernel.org/all/20251201-va_format_call-v2-0-2906f3093b60@intel.com/

On Mon 2025-12-01 10:31:24, Andrzej Hajda wrote:
> Changing argument type from va_list to struct va_format * allows
> to simplify variadic argument handling with va_format_call helper.
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 513e5ef8a6da..4d76b67a87e3 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4965,30 +4965,12 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>  #endif
>  
>  static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
> -			       const char *fmt, va_list vargsp)
> +			       const char *fmt, struct va_format *vaf)
>  {
> -	struct va_format vaf;
> -	va_list vargs;
> -
> -	/*
> -	 * On x86_64 and possibly on other architectures, va_list is actually a
> -	 * size-1 array containing a structure.  As a result, function parameter
> -	 * vargsp decays from T[1] to T*, and &vargsp has type T** rather than
> -	 * T(*)[1], which is expected by its assignment to vaf.va below.
> -	 *
> -	 * One standard way to solve this mess is by creating a copy in a local
> -	 * variable of type va_list and then using a pointer to that local copy
> -	 * instead, which is the approach employed here.
> -	 */
> -	va_copy(vargs, vargsp);
> -
> -	vaf.fmt = fmt;
> -	vaf.va = &vargs;

I am always a bit lost when using this API.
Why is it safe to remove the va_copy() here, please?

The va_format_call() uses va_start()/va_end() which is replacing
these calls in dev_err_probe() and dev_warn_probe().

It is possible that the original code was actually wrong because
it uses the same copy (&vaf) everywhere, see below.

>  	switch (err) {
>  	case -EPROBE_DEFER:
> -		device_set_deferred_probe_reason(dev, &vaf);

This function processes the arguments via:

  + device_set_deferred_probe_reason()
    + kasprintf()
      + va_start()/va_end()

> -		dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);

This function uses the already processed copy of the arguments.
IMHO, it might print a garbage because of this. IMHO, it should use
the original va_list() or might need its own copy.

Note that this call does not modify the va_list because it uses "%pV"
and vsprintf() creates its own copy in this case, see va_format()
in lib/vsprintf.c.

> +		device_set_deferred_probe_reason(dev, vaf);
> +		dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), vaf);
>  		break;
>  
>  	case -ENOMEM:
> @@ -4998,13 +4980,11 @@ static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
>  	default:
>  		/* Log fatal final failures as errors, otherwise produce warnings */
>  		if (fatal)
> -			dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
> +			dev_err(dev, "error %pe: %pV", ERR_PTR(err), vaf);
>  		else
> -			dev_warn(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
> +			dev_warn(dev, "error %pe: %pV", ERR_PTR(err), vaf);

This should be fine because of using "%pV".

>  		break;
>  	}
> -
> -	va_end(vargs);
>  }
>  
>  /**
> @@ -5042,15 +5022,7 @@ static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
>   */
>  int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
>  {
> -	va_list vargs;
> -
> -	va_start(vargs, fmt);
> -
> -	/* Use dev_err() for logging when err doesn't equal -EPROBE_DEFER */
> -	__dev_probe_failed(dev, err, true, fmt, vargs);
> -
> -	va_end(vargs);
> -
> +	va_format_call(fmt, __dev_probe_failed, dev, err, true, fmt, va_format_arg);
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(dev_err_probe);
> @@ -5090,15 +5062,7 @@ EXPORT_SYMBOL_GPL(dev_err_probe);
>   */
>  int dev_warn_probe(const struct device *dev, int err, const char *fmt, ...)
>  {
> -	va_list vargs;
> -
> -	va_start(vargs, fmt);
> -
> -	/* Use dev_warn() for logging when err doesn't equal -EPROBE_DEFER */
> -	__dev_probe_failed(dev, err, false, fmt, vargs);
> -
> -	va_end(vargs);
> -
> +	va_format_call(fmt, __dev_probe_failed, dev, err, false, fmt, va_format_arg);
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(dev_warn_probe);

Best Regards,
Petr