linux-kernel - Re: [PATCH 0/5] firmware: Add support for loading compressed files

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: <s5hwoil6jgd.wl-tiwai@suse.de>
Date:   Mon, 20 May 2019 20:59:14 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     shuah <shuah@...nel.org>
Cc:     Kees Cook <keescook@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/5] firmware: Add support for loading compressed files

On Mon, 20 May 2019 20:07:25 +0200,
Takashi Iwai wrote:
> 
> On Mon, 20 May 2019 19:26:59 +0200,
> shuah wrote:
> > 
> > On 5/20/19 10:22 AM, Takashi Iwai wrote:
> > > On Mon, 20 May 2019 17:18:48 +0200,
> > > Takashi Iwai wrote:
> > >>
> > >> On Mon, 20 May 2019 16:39:37 +0200,
> > >> Takashi Iwai wrote:
> > >>>
> > >>> On Mon, 20 May 2019 11:39:29 +0200,
> > >>> Greg Kroah-Hartman wrote:
> > >>>>
> > >>>> On Mon, May 20, 2019 at 11:26:42AM +0200, Takashi Iwai wrote:
> > >>>>> Hi,
> > >>>>>
> > >>>>> this is a patch set to add the support for loading compressed firmware
> > >>>>> files.
> > >>>>>
> > >>>>> The primary motivation is to reduce the storage size; e.g. currently
> > >>>>> the amount of /lib/firmware on my machine counts up to 419MB, and this
> > >>>>> can be reduced to 130MB file compression.  No bad deal.
> > >>>>>
> > >>>>> The feature adds only fallback to the compressed file, so it should
> > >>>>> work as it was as long as the normal firmware file is present.  The
> > >>>>> f/w loader decompresses the content, so that there is no change needed
> > >>>>> in the caller side.
> > >>>>>
> > >>>>> Currently only XZ format is supported.  A caveat is that the kernel XZ
> > >>>>> helper code supports only CRC32 (or none) integrity check type, so
> > >>>>> you'll have to compress the files via xz -C crc32 option.
> > >>>>>
> > >>>>> The patch set begins with a few other improvements and refactoring,
> > >>>>> followed by the compression support.
> > >>>>>
> > >>>>> In addition to this, dracut needs a small fix to deal with the *.xz
> > >>>>> files.
> > >>>>>
> > >>>>> Also, the latest patchset is found in topic/fw-decompress branch of my
> > >>>>> sound.git tree:
> > >>>>>    git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> > >>>>
> > >>>> After a quick review, these all look good to me, nice job.
> > >>>>
> > >>>> One recommendation, can we add support for testing this to the
> > >>>> tools/testing/selftests/firmware/ tests?  And you did run those
> > >>>> regression tests to verify that you didn't get any of the config options
> > >>>> messed up, right? :)
> > >>>
> > >>> Now I've been testing the firmware selftest, and this turned out to be
> > >>> surprisingly difficult on my system.  By some reason, the test always
> > >>> fails at the point triggering the request (line 58 of
> > >>> fw_filesystem.sh):
> > >>>
> > >>>    if ! echo -n "$NAME" >"$DIR"/trigger_request ; then
> > >>> 	....
> > >>>
> > >>> Judging from the strace output, this echo writes only the first byte
> > >>> of $NAME.  Then kernfs write op is invoked and it deals this one byte
> > >>> input as if a whole argument were passed, leading to an error.
> > >>>
> > >>> My temporary workaround was to replace the all "echo" call with
> > >>> "/usr/bin/echo".
> > >>>
> > >>> Then it hits a similar write error at the places like:
> > >>>
> > >>> 	echo 1 > $DIR/config_sync_direct
> > >>>
> > >>> This could be worked around by adding -n option to echo.
> > >>>
> > >>> Finally, I noticed that the user-fallback doesn't work on my system
> > >>> any longer and the test stopped.  This is expected, so it implies that
> > >>> all direct loading tests passed.
> > >>>
> > >>> FWIW, my system is openSUSE Leap 15.1.  Does anyone experience a
> > >>> similar problem?
> > >>
> > >> This seems to be a regression on 5.2-rc1.
> > >> The tests on 4.20 worked fine.  5.1 worked, but gave the error at
> > >> fallback test instead of skipping.  This is likely another regression,
> > >> but irrelevant with the major issue as above.
> > >>
> > >> Now bisecting...
> > >
> > > Still in bisection, but it's timeout, I'll have to leave now...
> > >
> > > FWIW, the regression seems to have been introduced around the latest
> > > kselftest merge.
> > > The commit 4c7b63a32d54 is bad, and the commit 9cbda1bddb4c good.
> > >
> > > Shuah, could you check it?
> > >
> > >
> > 
> > Kees,
> > 
> > Could this be related to your selftest Makefile test run output
> > refactoring work?
> > 
> > I did a quick test on 5.2 after my first kselftest update without
> > the refactor work - firmware test worked.
> > 
> > I am thinking this is related to
> > 
> > eff3ee303d0d
> > selftests: Extract single-test shell logic from lib.mk
> 
> OK, I could identify the culprit commit.  It's 5c069b6dedef
>     selftests: Move test output to diagnostic lines
> 
> It seems that stdbuf usage caused a regression, at least on my
> system.

So the problem is obvious: the commit above adjusts the stdout to be
unbuffered via stdbuf, hence each invocation like
  echo -n abc > /sys/....

would become writes of "a", "b" and "c", instead of "abc".

Although we can work around it in each test unit, I'm afraid that
enforcing the unbuffered stdio is too fragile for scripts like the
above case.


thanks,

Takashi

> 
> 
> Takashi
> 
> > 
> > bash vs. sh difference in "echo" command behavior is the cause
> > is my best guess. Will you be able to take a look at this?
> > 
> > sudo make -C tools/testing/selftests/firmware/ run_tests
> > 
> > will show the difference.
> > 
> > thanks,
> > -- Shuah
> >