Re: [PATCH v6 3/5] test: add new driver_data load tester

[Luis R. Rodriguez]

On Fri, May 12, 2017 at 09:20:24AM +0900, AKASHI Takahiro wrote:
> On Thu, May 11, 2017 at 08:26:29PM +0200, Luis R. Rodriguez wrote:
> > On Thu, May 11, 2017 at 07:46:27PM +0900, AKASHI Takahiro wrote:
> > > On Fri, Apr 28, 2017 at 03:45:35AM +0200, Luis R. Rodriguez wrote:
> > > > > > diff --git a/tools/testing/selftests/firmware/driver_data.sh b/tools/testing/selftests/firmware/driver_data.sh
> > > > ...
> > > > > > +ALL_TESTS="$ALL_TESTS 0012:1:1"
> > > > > > +ALL_TESTS="$ALL_TESTS 0013:1:1"
> > > > > 
> > > > > Do you have good reasons for "the number of times" here?
> > > > 
> > > > Just that 1 was not enough and more than 10 seemed too much. As is the tests
> > > 
> > > In my opinion, "1," or "2" given the nature of firmware caching, is good
> > > enough, but it's up to you.
> > 
> > No, firmware caching deserves its own test unit on its own, but that will be
> > enabled through a separate test once we move DRIVER_DATA_PRIV_REQ_NO_CACHE 
> > to DRIVER_DATA_REQ_NO_CACHE (a private feature to a publicly available
> > enabled feature). This will be enabled for the upcoming work which enables
> > FGPA firmware upload which will first enable request_firmware_into_buf()
> > through the driver data API which uses this.
> 
> I thought of implementing NO_CACHE option by myself, but came up with
> no practical use cases other than my test case :)

FWIW, my understanding is that such work is underway. I think it may be
ready first than your signature work so it may make sense to coordinate
with that developer on their work as you might be able to make use of their
tests, etc.

> > > BTW, firmware caching is a bit annoying in my signature tests
> > > because caching will bypass the verification checks when we iterates
> > > tests with different conditions.
> > 
> > It would seems to make sense to me to only need to verify files when read
> > for the first time, once its cache I don't see why we would re-verify them ?
> 
> Let me explain my test scenario:
> case (a): firmware w/o signature for signature-not-required driver
> case (b): firmware w/o signature for signature-required driver
>    ... and so on
> 
> Case (a) and (b) are exercised with the same firmware blob but with
> different 'test_config's of test_driver_data driver.
> 
> First do case (a) and it succeeds, caching the blob, then do case (b).
> It should fail, but actually succeeds due to firmware caching.
> 
> Yes, we can change the order, (b) then (a), to make tests run correctly.

I see. OK we should consider if firmware requirements can ever vary for
a file. If not then a cached firmware file can have a sig_check which
any request can fill in. Upon request firmware, if the cache is present
*but* the sig check requirements differ (in this simple case where
signature requirements cannot vary) then the cache present will not
suffice so we should allow a full new file check through to allow
to fill the sig_check. If signatures requirements can vary then we
could have a linked list of signature requirements and at cache check
time we'd have to check if they match the requirements.

Just one way to try to address this I think. But hey this is all firmware
signing related. I suppose a more relevant question to this thread is if
there might be current criteria or criteria being introduced in this patch
series which is similar to the signature check which should also be
extended in these sorts of caching checks.

> But again, when we iterate this set of test cases under "-c" repeatedly,
> this will happen again as you can imagine.

Makes sense.

> Thanks to firmware caching, each of iterations of tests is not executed
> under the exactly same condition. That is the problem.

Sure, specially since asynchornous lookups won't be serialized we can't
expect any proper order for which request goes in first. If the extra
caching requirements were however taken into consideration I would expect
this to not matter though.

> Let's think about more realistic case:
> we want to run the system without stopping/rebooting it, but also
> want to update some driver. OK, remove the driver module and insert
> a new one which, in this case, enables signature verification option.
> But, unlike we expect, loading a firmware will succeed anyway even
> if we don't provide its signature as it's already in cache.

Its a good point. If caching had the requirements checks (signature would be
one) then this would fail at the cache lookup and continue to chug on.

For files which are kept the same name, say db.txt the example you provide
also runs into the situation where one file might be signed but through time
can be expected to be updated, as such the signature will change as will the
contents of the file. In such cases the firmware cache would get in the way
of file updates, but let us be clear that the firmware cache is not a cache
of all firmware, its only used prior to suspend and cleared after resume.
As such it seems valid for that time period. Likewise if the cache is known
to not be a good idea for a file they can in the future use the no-cache
requirement criteria, but then they'd have to implement something to deal
with the suspend/resume race.

> I hope we can fix it by modifying the logic of caching.

Sure.

> > > > are rather simple compared to what we can do given the flexibility in how we
> > > > can perform tests due to the test driver structure, in the future this will
> > > > become more important. But best to just get in the basics before we hammer and
> > > > expand on this a lot. There is also the question of sharing this sort of logic
> > > > with the upper testing layers so that they deal with this and not us
> > > > (tools/testing/selftests/), in that sense all this is just sufficient for us to do
> > > > our own testing for now, but we may and should consider how to get the upper
> > > > layers to deal this for us. But we can address this later.
> > > > 
> > > > > > +# Not yet sure how to automate suspend test well yet.  For now we expect a
> > > > > > +# manual run. If using qemu you can resume a guest using something like the
> > > > > > +# following on the monitor pts.
> > > > > > +# system_wakeupakeup | socat - /dev/pts/7,raw,echo=0,crnl
> > > > > > +#ALL_TESTS="$ALL_TESTS 0014:0:1"
> > > > > > +
> > > > > > +test_modprobe()
> > > > > > +{
> > > > > > +       if [ ! -d $DIR ]; then
> > > > > > +               echo "$0: $DIR not present" >&2
> > > > > > +               echo "You must have the following enabled in your kernel:" >&2
> > > > > > +               cat $TEST_DIR/config >&2
> > > > > > +               exit 1
> > > > > > +       fi
> > > > > > +}
> > > > > > +
> > > > > > +function allow_user_defaults()
> > > > > > +{
> > > > > > +	if [ -z $DEFAULT_NUM_TESTS ]; then
> > > > > > +		DEFAULT_NUM_TESTS=50
> > > > > > +	fi
> > > > > > +
> > > > > > +	if [ -z $FW_SYSFSPATH ]; then
> > > > > > +		FW_SYSFSPATH="/sys/module/firmware_class/parameters/path"
> > > > > > +	fi
> > > > > > +
> > > > > > +	if [ -z $OLD_FWPATH ]; then
> > > > > > +		OLD_FWPATH=$(cat $FW_SYSFSPATH)
> > > > > > +	fi
> > > > > > +
> > > > > > +	if [ -z $FWPATH]; then
> > > > > > +		FWPATH=$(mktemp -d)
> > > > > > +	fi
> > > > > > +
> > > > > > +	if [ -z $DEFAULT_DRIVER_DATA ]; then
> > > > > > +		config_reset
> > > > > > +		DEFAULT_DRIVER_DATA=$(config_get_name)
> > > > > > +	fi
> > > > > > +
> > > > > > +	if [ -z $FW ]; then
> > > > > > +		FW="$FWPATH/$DEFAULT_DRIVER_DATA"
> > > > > > +	fi
> > > > > > +
> > > > > > +	if [ -z $SYS_STATE_PATH ]; then
> > > > > > +		SYS_STATE_PATH="/sys/power/state"
> > > > > > +	fi
> > > > > > +
> > > > > > +	# Set the kernel search path.
> > > > > > +	echo -n "$FWPATH" > $FW_SYSFSPATH
> > > > > > +
> > > > > > +	# This is an unlikely real-world firmware content. :)
> > > > > > +	echo "ABCD0123" >"$FW"
> > > > > 
> > > > > Do you always want to overwrite the firmware even if user explicitly
> > > > > provides it?
> > > > 
> > > > This is a test script so it constructs its own temporary path so it can
> > > > have the confidence to overwrite anything it pleases. So in this case yes.
> > > > Its just as the old firmware test script.
> > > 
> > > Right, but looking into the script, even if an user supplies a firmware
> > > blob, the script overwrites it unnecessarily.
> > 
> > FWPATH=$(mktemp -d)
> > ...
> > FW="$FWPATH/$DEFAULT_DRIVER_DATA"
> > ...
> > echo "ABCD0123" >"$FW"
> > 
> > So this is really just touching the custom path stuff, unless of course the
> > caller overrides the above variables, in which case its trusted they know what
> > they are doing, ie custom test cases / setup / system. That was the goal.
> 
> Even if an user gives a full-path name to $FW, do you want to overwrite it?

User cannot give a full path to a file, its one of the promises of the
firmware_class.c today, it uses its own prefix set of directories, it
could be security issue otherwise. In the future this may change and the
scope of what prefixes can be used might change but we're far from there.

If you meant if user gives a full-path to $FW by testing -- well this is
all part of the driver-data.sh and driver-data.sh uses its own custom
temporary directory for stashing tests, so what it does is specific to
it and compartamentalized there. If a users does not use driver-data.sh
and mucks with the knobs on their own, its expected they know what they
are doing, and by default the paths for lookups will be default so its
up to them to make sure to pick a proper file and not overwrite things.

I think its safe for test_driver.c to use the default path and assume
the test user will not go try to overwrite /lib/firwmare/iwlwifi-7260-17.ucode
for example.

If this is your concern we could devise the temporary directory logic in
the script into the test driver but I really would prefer to avoid that.

  Luis