Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1666278 > unrolled thread

[PATCH 0/4] firmware: fix fallback mechanism by ignoring SIGCHLD

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] firmware: fix fallback mechanism by ignoring SIGCHLD "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-15 00:30 +0200
    [PATCH 4/4] firmware: send -EINTR on signal abort on fallback mechanism "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-15 00:30 +0200
    [PATCH 1/4] test_firmware: add test case for SIGCHLD on sync fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-15 00:30 +0200
    [PATCH 3/4] firmware: avoid invalid fallback aborts by using killable swait "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-15 00:30 +0200
    Re: [PATCH 0/4] firmware: fix fallback mechanism by ignoring SIGCHLD Martin Fuzzey <mfuzzey@parkeon.com> - 2017-06-15 09:50 +0200

#1666278 — [PATCH 0/4] firmware: fix fallback mechanism by ignoring SIGCHLD

Martin reported an issue with Android where if sysfs is used to trigger a sync
fw load which *relies* on the fallback mechanism and a background job completes
while the trigger is ongoing in the foreground it will immediately fail the fw
request.  The issue can be observed in this simple test script using the
test_firmware driver:

	set -e
	/etc/init.d/udev stop
	modprobe test_firmware
	DIR=/sys/devices/virtual/misc/test_firmware
	echo 10 >/sys/class/firmware/timeout
	sleep 2 &
	echo -n "does-not-exist-file.bin" > "$DIR"/trigger_request

The background sleep triggers the SIGCHLD signal and we fail the firmware
request on the fallback mechanism. This was due to the type of wait used which
captures all signals, but we currently lack the killable swaits which would
only allow killing the fallback wait on SIGKILL. This adds the missing killable
swaits and fixes the firmware API to use it. This goes along with a test case to
demo the issue clearly and how its fixed afterwards.  Lastly, ensure to use
-EINTR when interrupted so callers can distinguish between an interrupted
failure and other types of failure.

As suggested I've tagged the addition of the killable swaits and the firmware
fix as stable. Between v4.0 and v4.10 the stable fix is to instead change
the firmware to use wait_for_completion_killable_timeout() as the firmware
API was only converted to swait as of v4.10.

The last patch must be applied only after the killable wait is applied to
ensure only SIGKILL triggers an interruption. Otherwise it has been observed
using -EINTR on other signals like SIGCHLD will trigger a restart of the call,
if a sysfs write() is used to trigger the sync firmware request. This might be
due what signal(7) says about using the SA_RESTART flag when write() is called,
in such cases the call will be automatically restarted after the signal handler
returns. The excemption here naturally seems to be on SIGKILL.

Note that although I *feared* this might implicate any use of non-killable waits
on other system calls, such as finit_module(), initial testing confirms this to
not be the case.  For instance replacing the echo with modprobe on a module
which does the same on init does not present the same issues. This could be due
to the special SA_RESTART flag case on write() as noted above and sysfs...
however, its not perfectly clear yet to me.

As usual these patches are on my linux-next git tree, they are on the
20170614-fw-fixes branch based on linux-next 20170614 [0].

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170614-fw-fixes

Luis R. Rodriguez (4):
  test_firmware: add test case for SIGCHLD on sync fallback
  swait: add the missing killable swaits
  firmware: avoid invalid fallback aborts by using killable swait
  firmware: send -EINTR on signal abort on fallback mechanism

 drivers/base/firmware_class.c                   | 11 +++++----
 include/linux/swait.h                           | 25 ++++++++++++++++++++
 tools/testing/selftests/firmware/fw_fallback.sh | 31 +++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 4 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1666282 — [PATCH 4/4] firmware: send -EINTR on signal abort on fallback mechanism

Right now we send -EAGAIN to a syfs write which got interrupted.
Userspace can't tell what happened though, send -EINTR if we
were killed due to a signal so userspace can tell things apart.

This is only applicable to the fallback mechanism.

Reported-by: Martin Fuzzey <mfuzzey@parkeon.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 70fc42e5e0da..da043cb16e2f 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1089,9 +1089,12 @@ 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 = -EINTR;
+		else
+			retval = -EAGAIN;
+	} else if (buf->is_paged_buf && !buf->data)
 		retval = -ENOMEM;
 
 	device_del(f_dev);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1666289 — [PATCH 1/4] test_firmware: add test case for SIGCHLD on sync fallback

It has been reported that SIGCHLD will trigger an immediate abort
on sync firmware requests which rely on the sysfs interface for a
trigger. This is unexpected behaviour, this reproduces this issue.

This test case currenty fails.

Reported-by: Martin Fuzzey <mfuzzey@parkeon.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tools/testing/selftests/firmware/fw_fallback.sh | 31 +++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
index 2e4c22d5abf7..8f511035f783 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -134,6 +134,27 @@ load_fw_custom_cancel()
 	wait
 }
 
+load_fw_fallback_with_child()
+{
+	local name="$1"
+	local file="$2"
+
+	# This is the value already set but we want to be explicit
+	echo 4 >/sys/class/firmware/timeout
+
+	sleep 1 &
+	SECONDS_BEFORE=$(date +%s)
+	echo -n "$name" >"$DIR"/trigger_request 2>/dev/null
+	SECONDS_AFTER=$(date +%s)
+	SECONDS_DELTA=$(($SECONDS_AFTER - $SECONDS_BEFORE))
+	if [ "$SECONDS_DELTA" -lt 4 ]; then
+		RET=1
+	else
+		RET=0
+	fi
+	wait
+	return $RET
+}
 
 trap "test_finish" EXIT
 
@@ -221,4 +242,14 @@ else
 	echo "$0: cancelling custom fallback mechanism works"
 fi
 
+set +e
+load_fw_fallback_with_child "nope-signal-$NAME" "$FW"
+if [ "$?" -eq 0 ]; then
+	echo "$0: SIGCHLD on sync ignored as expected" >&2
+else
+	echo "$0: error - sync firmware request cancelled due to SIGCHLD" >&2
+	exit 1
+fi
+set -e
+
 exit 0
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1666291 — [PATCH 3/4] firmware: avoid invalid fallback aborts by using killable swait

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 -- for instance when a user hits CTRL-C. The abort was overly
*too* effective.

When a child process terminates (successful or not) the signal SIGCHLD can
be sent to the parent process which ran the child in the background and
later triggered a sync request for firmware through a sysfs interface which
relies on the fallback mechanism. This signal in turn can be recieved by the
interruptible swait we constructed on firmware_class and detects it as an
abort *before* userspace could get a chance to write the firmware. Upon
failure -EAGAIN is returned, so userspace is also kept in the dark about
exactly what happened.

We can reproduce the issue with the fw_fallback.sh selftest:

Before this patch:
$ sudo tools/testing/selftests/firmware/fw_fallback.sh
...
tools/testing/selftests/firmware/fw_fallback.sh: error - sync firmware request cancelled due to SIGCHLD

After this patch:
$ sudo tools/testing/selftests/firmware/fw_fallback.sh
...
tools/testing/selftests/firmware/fw_fallback.sh: SIGCHLD on sync ignored as expected

Fix this by making the swait killable -- only killable by SIGKILL (kill -9).
We loose the ability to allow userspace to cancel a write with CTRL-C
(SIGINT), however its been decided the compromise to require SIGKILL is
worth the gains.

Chances of this issue occuring are low due to the number of drivers upstream
exclusively relying on the fallback mechanism for firmware (2 drivers),
however this is observed in the field with custom drivers with sysfs
triggers to load firmware. Only distributions relying on the fallback
mechanism are impacted as well. An example reported issue was on Android,
as follows:

1) Android init (pid=1) fork()s (say pid=42) [this child process is totally
   unrelated to firmware loading, it could be sleep 2; for all we care ]
2) Android init (pid=1) does a write() on a (driver custom) sysfs file which
   ends up calling request_firmware() kernel side
3) The firmware loading fallback mechanism is used, the request is sent to
   userspace and pid 1 waits in the kernel on wait_*
4) before firmware loading completes pid 42 dies (for any reason, even
   normal termination)
5) Kernel delivers SIGCHLD to pid=1 to tell it a child has died, which
   causes -ERESTARTSYS to be returned from wait_*
6) The kernel's wait aborts and return -EAGAIN for the
   request_firmware() caller.

swait was introduced as of v4.6 and the firmware_class code was modified to
use swait only as of v4.10 via commit 5b029624948d6 ("firmware: do not use
fw_lock for fw_state protection"), as such stable kernels older than v4.10
must modify the old firmware_class call:

	from  wait_for_completion_interruptible_timeout()
	to    wait_for_completion_killable_timeout()

Cc: stable <stable@vger.kernel.org> # 4.0
Suggested-by: "Eric W. Biederman" <ebiederm@xmission.com>
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Tested-by: Martin Fuzzey <mfuzzey@parkeon.com>
Reported-by: Martin Fuzzey <mfuzzey@parkeon.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 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 b9f907eedbf7..70fc42e5e0da 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -131,7 +131,7 @@ static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
 {
 	long ret;
 
-	ret = swait_event_interruptible_timeout(fw_st->wq,
+	ret = swait_event_killable_timeout(fw_st->wq,
 				__fw_state_is_done(READ_ONCE(fw_st->status)),
 				timeout);
 	if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1666519

On 15/06/17 00:20, Luis R. Rodriguez wrote:
> Martin reported an issue with Android where if sysfs is used to trigger a sync
> fw load which *relies* on the fallback mechanism and a background job completes
> while the trigger is ongoing in the foreground it will immediately fail the fw
> request.  The issue can be observed in this simple test script using the
> test_firmware driver:
>
> 	set -e
> 	/etc/init.d/udev stop
> 	modprobe test_firmware
> 	DIR=/sys/devices/virtual/misc/test_firmware
> 	echo 10 >/sys/class/firmware/timeout
> 	sleep 2 &
> 	echo -n "does-not-exist-file.bin" > "$DIR"/trigger_request
>
> The background sleep triggers the SIGCHLD signal and we fail the firmware
> request on the fallback mechanism. This was due to the type of wait used which
> ...
>
> Note that although I *feared* this might implicate any use of non-killable waits
> on other system calls, such as finit_module(), initial testing confirms this to
> not be the case.  For instance replacing the echo with modprobe on a module
> which does the same on init does not present the same issues. This could be due
> to the special SA_RESTART flag case on write() as noted above and sysfs...
> however, its not perfectly clear yet to me.
>
>
The reason the problem does not occur with modprobe is that in that case 
the processes triggering the firmware load
(modprobe) and the process dying (sleep) are *siblings* rather than 
father and child.
So the modprobe process does *not* receive a SIGCHLD when its' *brother* 
dies.

echo is a shell built-in so the process triggering the firmware load 
(the shell) and the process dying (sleep) *are* father and child.

Martin

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web