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

[Luis R. Rodriguez]

On Wed, Jun 14, 2017 at 03:20:13PM -0700, 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
> 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

Greg, *poke*

  Luis