linux-kernel - Re: [RFC PATCH 0/7] platform/chrome: Add Kunit tests for protocol device drivers

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] [day] [month] [year] [list]

Message-ID: <CAGS_qxrcH0+mJTO4nJqXnk2Bh7oO_PEur=ytcxL8wxJNCu20Tw@mail.gmail.com>
Date: Tue, 20 May 2025 09:04:53 -0700
From: Daniel Latypov <dlatypov@...gle.com>
To: Tzung-Bi Shih <tzungbi@...nel.org>
Cc: bleung@...omium.org, brendan.higgins@...ux.dev, davidgow@...gle.com, 
	rmoar@...gle.com, rostedt@...dmis.org, mhiramat@...nel.org, naveen@...nel.org, 
	anil.s.keshavamurthy@...el.com, davem@...emloft.net, 
	chrome-platform@...ts.linux.dev, linux-kselftest@...r.kernel.org, 
	kunit-dev@...glegroups.com, linux-trace-kernel@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/7] platform/chrome: Add Kunit tests for protocol
 device drivers

On Tue, May 20, 2025 at 1:25 AM 'Tzung-Bi Shih' via KUnit Development
<kunit-dev@...glegroups.com> wrote:
>
> The protocol device drivers under drivers/platform/chrome/ are responsible
> to communicate to the ChromeOS EC (Embedded Controller).  They need to pack
> the data in a pre-defined format and check if the EC responds accordingly.
>
> The series adds some fundamental unit tests for the protocol.  It calls the
> .cmd_xfer() and .pkt_xfer() callbacks (which are the most crucial parts for
> the protocol), mocks the rest of the system, and checks if the interactions
> are all correct.
>
> The series isn't ready for landing.  It's more like a PoC for the
> binary-level function redirection and its use cases.
>
> The 1st patch adds ftrace stub which is originally from [1][2].  There is no
> follow-up discussion about the ftrace stub.  As a result, the patch is still
> on the mailing list.
>
> The 2nd patch adds Kunit tests for cros_ec_i2c.  It relies on the ftrace stub
> for redirecting cros_ec_{un,}register().
>
> The 3rd patch uses static stub instead (if ftrace stub isn't really an option).
> However, I'm not a big fan to change the production code (i.e. adding the
> prologue in cros_ec_{un,}register()) for testing.
>
> The 4th patch adds Kunit tests for cros_ec_spi.  It relies on the ftrace stub
> for redirecting cros_ec_{un,}register() again.
>
> The 5th patch calls .probe() directly instead of forcing the driver probe
> needs to be synchronous.  In comparison with the 4th patch, I don't think
> this is simpler.  I'd prefer to the way in the 4th patch.
>
> After talked to Masami about the work, he suggested to use Kprobes for
> function redirection.  The 6th patch adds kprobes stub.
>
> The 7th patch uses kprobes stub instead for cros_ec_spi.
>
> Questions:
> - Are we going to support ftrace stub so that tests can use it?
>
> - If ftrace stub isn't on the plate (e.g. due to too many dependencies), how
>   about the kprobes stub?  Is it something we could pursue?

Quick comment,
If I recall, the thought process was that we could consider it in the
future if there was enough demand for it.

We have these drawbacks with the current ftrace stubs:
* doesn't compile on all arches
* silently doesn't work on inlined functions <== scariest one to me
* is more complicated and has more dependencies

So it felt like the better move to go with static stubs which has none
of those drawbacks (works on all arches, all functions, and is dead
simple) as opposed to simultaneously introducing two ways to do the
same thing.

You mention you don't like how static stubs requires modifying the
code-under-test.
Since it gets eliminated by the preprocessor unless you're compiling
for KUnit, is the concern more so about how it conceptually feels
wrong to do so?
For the Android GKI kernel, they have (or had) KUnit enabled so there
is potentially concern about real runtime cost there, not sure if you
have something similar in mind.

But stepping back, ftrace_stubs technically require modifying the code
to make sure funcs are marked as `noinline`, which this patch series
does not do.
I've not looked at cros_ec_{un,}register() to check if they're at risk
of inlining, but wanted to call that out, that ftrace stubs
technically don't handle your usage pattern 100% properly.

>
> - (minor) I'm unsure if people would prefer 'kprobes stub' vs. 'kprobe stub'.
>

I'd personally vote for kprobe_stub.

Daniel