linux-kernel - Re: [PATCHv2 0/3] Enable no_cache flag to driver

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]

Date:   Wed, 24 May 2017 22:32:46 +0200
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        John Ewalt <jewalt@...innovations.com>
Cc:     yi1.li@...ux.intel.com, atull@...nel.org,
        gregkh@...uxfoundation.org, wagi@...om.org, dwmw2@...radead.org,
        rafal@...ecki.pl, arend.vanspriel@...adcom.com, rjw@...ysocki.net,
        moritz.fischer@...us.com, pmladek@...e.com,
        johannes.berg@...el.com, emmanuel.grumbach@...el.com,
        luciano.coelho@...el.com, kvalo@...eaurora.org, luto@...nel.org,
        takahiro.akashi@...aro.org, dhowells@...hat.com, pjones@...hat.com,
        linux-kernel@...r.kernel.org, linux-fpga@...r.kernel.org
Subject: Re: [PATCHv2 0/3] Enable no_cache flag to driver_data

On Wed, May 24, 2017 at 09:03:57PM +0200, Luis R. Rodriguez wrote:
> __fw_lookup_buf() really should be
> renamed to something that reflects this is a cache lookup.

Actually I take this back, other than the cache, note that when we
fw_lookup_and_allocate_buf() we first __fw_lookup_buf() but if the buf is not
there we allocate a new one and list_add() it to the cache regardless of
whether or not the cache thing has been enabled.

What this does then *also* other than caching for suspend/resume (which we
should document more formally) is to gather up contending lookups together
with one buf, and share the final status of just one lookup. If a buf is
found we fw_state_wait() on _request_firmware_prepare() until the buf clears.

John Ewalt recently reported some issues with loadng multiple files at the
same time. He also provided a patch. The swake_up() fix seems sensible
and would seem to have been caused by the swait transformation, but in
inspecting the other proposed changes it would seem we have had tons of
other lingering bugs which have probably existed for ages.

For instance, if there are pending requests for a leader request to send back
info, and one is about to complete but in the last moment on
assign_firmware_buf() fails, all the error paths lack a wake up call. As such
all pending requests may just wait and linger, and since none of these have
a timeout I would expect these to just linger forever. I'm not even sure we
kref_get() properly on the buf for pending requests when we are waiting for
a serialized request, ie, we might be able to take a buf underneath the nose
of a waiter.

Although some are new bugs, some seem to be really old bugs.

These are the sorts of issues I wish for a test driver to be able to uncover,
test and ensure we never regress again. This is also why I am being careful
about enabling a feature, we should *really* think things through well before
enabling on the new API.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=195477

  Luis