linux-kernel - Re: [PATCH driver-core/master] firmware: Correct handling of fw_state_wait

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:   Mon, 16 Jan 2017 19:29:12 +0100
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     gregkh@...uxfoundation.org,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Daniel Wagner <daniel.wagner@...-carit.de>,
        "Luis R . Rodriguez" <mcgrof@...nel.org>,
        Ming Lei <ming.lei@...onical.com>,
        linux-kernel@...r.kernel.org, oss-drivers@...ronome.com
Subject: Re: [PATCH driver-core/master] firmware: Correct handling of
 fw_state_wait_timeout() return value

On Mon, Jan 16, 2017 at 02:57:06PM +0000, Jakub Kicinski wrote:
> Commit 5d47ec02c37e ("firmware: Correct handling of fw_state_wait()
> return value") made the assumption that any error returned from
> fw_state_wait_timeout() means FW load has to be aborted.  This is
> incorrect FW load only has to be aborted when load timed out or

You want a comma before FW -- but also:

> has been interrupted,

__fw_state_wait_common() returns -ENOENT when:

if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
	return -ENOENT;

Why not for when -ENOENT is returned ?

> otherwise the waking thread had already
> cleaned up for us.

What code in what waking thread would have done precisely what cleanup?
And why can't fw_load_abort() handle being called twice and why not just
instead allow for that?

> Fixes: 5d47ec02c37e ("firmware: Correct handling of fw_state_wait() return value")

What does this fix exactly? A fix should describe the impact, what
issues are in place without the fix. What also happens after the fix
and why. In this commit log none of this is clear.

> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> ---
>  drivers/base/firmware_class.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 4497d263209f..ce142e6b2c72 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1020,7 +1020,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>  	}
>  
>  	retval = fw_state_wait_timeout(&buf->fw_st, timeout);
> -	if (retval < 0) {
> +	if (retval == -ETIMEDOUT || retval == -ERESTARTSYS) {

Also, if your change is correct I will also note fw_state_wait_timeout()
is just a wrapper for __fw_state_wait_common(), but we also have
another wrapper for __fw_state_wait_common() now:

#define fw_state_wait(fw_st)                                    \               
        __fw_state_wait_common(fw_st, MAX_SCHEDULE_TIMEOUT) 

Do we need to fix anything for fw_state_wait() ?

Clarifying all this would help review your proposed changes. If you
consider them a fix please be very clear as to the exact issue and
what is fixed with your patch.

  Luis