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: <aTBNj5HEfejyW5TK@smile.fi.intel.com>
Date: Wed, 3 Dec 2025 16:47:43 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: "Hajda, Andrzej" <andrzej.hajda@...el.com>
Cc: Petr Mladek <pmladek@...e.com>, 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, Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH v2 3/5] drivers/core: simplify variadic args handling

On Tue, Dec 02, 2025 at 07:03:37PM +0100, Hajda, Andrzej wrote:
> W dniu 02.12.2025 o 16:51, Petr Mladek pisze:
> > 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/

TBH, I don't like the result. There are two problems with readability:

1) macro well hides the actual low-level call, hard to parse from its
parameters;

2) sometimes it has va_format_call(fmt, ..., fmt, ...) which is confusing.

Implementation is also doubtful (to me) as GCC extension. Can't it rather
return an error code and use something like do { } while (0) inside? OTOH,
may be this is not feasible in a clean way...

And what is the motivation? Just make less LoCs? I would really like to see
at least vmlinux sizes, the reports that GCC _and_ clang are both happy with
the compilation as of `make W=1` of this on both 32- and 64-bit cases.

Does it solve any issue? Does it bring any consistency or standardisation here?

> > On Mon 2025-12-01 10:31:24, Andrzej Hajda wrote:

...

> > > -	/*
> > > -	 * 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?
> 
> Not very familiar with this workaround, just my thoughts about it.
> 
> It is just va_list is compiler's private implementation, which can be
> anything.
> 
> And if it happens to be T[1], it's type decays to T* if it is type of
> argument of the function.
> 
> So vargsp is in fact of type T*, and &vargs is of type T** and it does not
> point to va_list anymore.
> 
> So in short passing va_list to a function, which takes a pointer to the arg
> is problematic.
> 
> va_format_call DOES NOT pass va_list to a function, so it seems to be safe.

I'm sorry, I can't be helpful here, as I am not well familiar
with va_*() stuff. The idea is interesting, nevertheless, but
see above.

> > 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()
> 
> This va_start/va_end is for var_args of kasprintf, not for &vaf, I hope
> parsing %pV uses va_copy.

Yes, it does call va_copy().

-- 
With Best Regards,
Andy Shevchenko