[PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

[PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

[Luis R. Rodriguez]

From: Martin Fuzzey <mfuzzey@parkeon.com>

Commit 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
is interrupted") added via 4.0 added support to abort the fallback mechanism
when a signal was detected and wait_for_completion_interruptible() returned
-ERESTARTSYS. Although the abort was effective we were unfortunately never
really propagating this error though and as such userspace could not know
why the abort happened.

The error code was always being lost to an even older change, commit
0542ad88fbdd81bb ("firmware loader: Fix _request_firmware_load() return val
for fw load abort") by Shuah Khan which was merged since v3.17. Back then
though we never were capturing these signals or bailing on a signal. After
this change though only only -EAGAIN was being relayed back to userspace
on non-memory errors including signals trying to interrupt our fallback
process.

It only makes sense to fix capturing -ERESTARTSYS since we were capturing
the error but when it was actually effective, since commit 0cb64249ca500
("firmware_loader: abort request if wait_for_completion is interrupted").

Only distributions relying on the fallback mechanism are impacted. An
example issue is on Android, when request_firmware() is called through
the firmware fallback mechanism -- syfs write call, sysfs .store(), and
Android sends a SIGCHLD to fail the write call -- in such cases the fact
that we failed due to a signal is lost.

Fix this and ensure we propagate -ERESTARTSYS so that handlers can
whether or not to restart write calls.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
Cc: stable <stable@vger.kernel.org> # 4.0
Acked-by: Luis R. Rodriguez <mcgrof@kernel.org>
[mcgrof: gave the commit log some serious love]
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---

For those following drivers/base/firmware_class.c development -- I've pushed
out two new branches based on linux-next tag next-20170524, one is just the
driver-data API [0], and the other one just has this patch applied on top
of the driver-data API [1]. You'll want to use driver-data-stable if you are
working on the cache or the fallback mechanism since this fix does provide
a fix for the cache / fallback mechanism.

https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170524-driver-data
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170524-driver-data-stable

  Luis

 drivers/base/firmware_class.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7af430a2d656..77c0e0792c30 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1309,9 +1309,10 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		mutex_unlock(&fw_lock);
 	}
 
-	if (fw_state_is_aborted(&buf->fw_st))
-		retval = -EAGAIN;
-	else if (buf->is_paged_buf && !buf->data)
+	if (fw_state_is_aborted(&buf->fw_st)) {
+		if (retval != -ERESTARTSYS)
+			retval = -EAGAIN;
+	} else if (buf->is_paged_buf && !buf->data)
 		retval = -ENOMEM;
 
 	device_del(f_dev);
-- 
2.10.2

Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

[Andy Lutomirski]

On Wed, May 24, 2017 at 2:40 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> From: Martin Fuzzey <mfuzzey@parkeon.com>
>
> Commit 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
> is interrupted") added via 4.0 added support to abort the fallback mechanism
> when a signal was detected and wait_for_completion_interruptible() returned
> -ERESTARTSYS. Although the abort was effective we were unfortunately never
> really propagating this error though and as such userspace could not know
> why the abort happened.

Can you give a simple example of what's going on and why it matters?

ERESTARTSYS and friends are highly magical, and I'm not convinced that
allowing _request_firmware_load to return -ERESTARTSYS is actually a
good idea.  What if there are system calls that can't handle this
style of restart that start being restarted as a result?