Re: [PATCH 4.8 50/96] firmware: fix usermode helper fallback loading
On Fri, Mar 24, 2017 at 04:01:58PM -0400, Ben Gamari wrote: > Greg Kroah-Hartman writes: > > > On Fri, Jan 06, 2017 at 10:54:38PM +0100, Yves-Alexis Perez wrote: > >> On Fri, 2017-01-06 at 22:43 +0100, Greg Kroah-Hartman wrote: > >> > 4.8-stable review patch. If anyone has any objections, please let me > >> > know. > >> > >> Hi Greg, > >> > >> Ben Gamari think there was a regression in that patch so I'm adding him to > >> recipients so he can voice concerns if needed. > > > > Given the lack of response, I'm going to assume all is fine :) > > > Oh dear, sorry for the late response; this was stuck in the pergatory of > my inbox. > > It's been a while since I've looked at this, but I believe the alleged > regression in this pastch is the reason I have the attached patch in my > tree. I seem to recall that it was the ath10k driver which triggered the > issue. > > Unfortunately I can't recall which driver was affected by this. I'll > have to see what happens when I revert the attached patch. > > Cheers, > > - Ben > > > > Author: Ben Gamari > Date: Mon Jan 2 00:38:05 2017 -0500 > > firmware_class: Ensure buf is non-NULL in __fw_load_abort > > I have observed that this can be NULL. > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index ac350c518e0c..fd0be24911fc 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -546,7 +546,8 @@ static void __fw_load_abort(struct firmware_buf *buf) > * There is a small window in which user can write to 'loading' > * between loading done and disappearance of 'loading' > */ > - if (fw_state_is_done(&buf->fw_st)) > + > + if (!buf || fw_state_is_done(&buf->fw_st)) > return; > > list_del_init(&buf->pending_list); We discussed this a while back and went with a more elegant fix, see: commit 191e885a2e130e639bb0c8ee350d7047294f2ce6 Luis
Re: [PATCH 4.8 50/96] firmware: fix usermode helper fallback loading
Greg Kroah-Hartman writes: > On Fri, Jan 06, 2017 at 10:54:38PM +0100, Yves-Alexis Perez wrote: >> On Fri, 2017-01-06 at 22:43 +0100, Greg Kroah-Hartman wrote: >> > 4.8-stable review patch. If anyone has any objections, please let me know. >> >> Hi Greg, >> >> Ben Gamari think there was a regression in that patch so I'm adding him to >> recipients so he can voice concerns if needed. > > Given the lack of response, I'm going to assume all is fine :) > Oh dear, sorry for the late response; this was stuck in the pergatory of my inbox. It's been a while since I've looked at this, but I believe the alleged regression in this pastch is the reason I have the attached patch in my tree. I seem to recall that it was the ath10k driver which triggered the issue. Unfortunately I can't recall which driver was affected by this. I'll have to see what happens when I revert the attached patch. Cheers, - Ben Author: Ben Gamari Date: Mon Jan 2 00:38:05 2017 -0500 firmware_class: Ensure buf is non-NULL in __fw_load_abort I have observed that this can be NULL. diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index ac350c518e0c..fd0be24911fc 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -546,7 +546,8 @@ static void __fw_load_abort(struct firmware_buf *buf) * There is a small window in which user can write to 'loading' * between loading done and disappearance of 'loading' */ - if (fw_state_is_done(&buf->fw_st)) + + if (!buf || fw_state_is_done(&buf->fw_st)) return; list_del_init(&buf->pending_list); signature.asc Description: PGP signature
Re: [PATCH 4.8 50/96] firmware: fix usermode helper fallback loading
On Fri, Jan 06, 2017 at 10:54:38PM +0100, Yves-Alexis Perez wrote: > On Fri, 2017-01-06 at 22:43 +0100, Greg Kroah-Hartman wrote: > > 4.8-stable review patch. If anyone has any objections, please let me know. > > Hi Greg, > > Ben Gamari think there was a regression in that patch so I'm adding him to > recipients so he can voice concerns if needed. Given the lack of response, I'm going to assume all is fine :) If not, 4.10-rc3 needs to be fixed... thanks, greg k-h
Re: [PATCH 4.8 50/96] firmware: fix usermode helper fallback loading
On Fri, 2017-01-06 at 22:43 +0100, Greg Kroah-Hartman wrote:
> 4.8-stable review patch. If anyone has any objections, please let me know.
Hi Greg,
Ben Gamari think there was a regression in that patch so I'm adding him to
recipients so he can voice concerns if needed.
Regards,
>
> --
>
> From: Yves-Alexis Perez
>
> commit 2e700f8d85975f516ccaad821278c1fe66b2cc98 upstream.
>
> When you use the firmware usermode helper fallback with a timeout value set
> to a
> value greater than INT_MAX (2147483647) a cast overflow issue causes the
> timeout value to go negative and breaks all usermode helper loading. This
> regression was introduced through commit 68ff2a00dbf5 ("firmware_loader:
> handle timeout via wait_for_completion_interruptible_timeout()") on kernel
> v4.0.
>
> The firmware_class drivers relies on the firmware usermode helper
> fallback as a mechanism to look for firmware if the direct filesystem
> search failed only if:
>
> a) You've enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK (not many
> distros):
>
> Then all of these callers will rely on the fallback mechanism in case
> the firmware is not found through an initial direct filesystem lookup:
>
> o request_firmware()
> o request_firmware_into_buf()
> o request_firmware_nowait()
>
> b) If you've only enabled CONFIG_FW_LOADER_USER_HELPER (most distros):
>
> Then only callers using request_firmware_nowait() with the second
> argument set to false, this explicitly is requesting the UMH firmware
> fallback to be relied on in case the first filesystem lookup fails.
>
> Using Coccinelle SmPL grammar we have identified only two drivers
> explicitly requesting the UMH firmware fallback mechanism:
>
> - drivers/firmware/dell_rbu.c
> - drivers/leds/leds-lp55xx-common.c
>
> Since most distributions only enable CONFIG_FW_LOADER_USER_HELPER the
> biggest impact of this regression are users of the dell_rbu and
> leds-lp55xx-common device driver which required the UMH to find their
> respective needed firmwares.
>
> The default timeout for the UMH is set to 60 seconds always, as of
> commit 68ff2a00dbf5 ("firmware_loader: handle timeout via
> wait_for_completion_interruptible_timeout()") the timeout was bumped
> to MAX_JIFFY_OFFSET ((LONG_MAX >> 1)-1). Additionally the MAX_JIFFY_OFFSET
> value was also used if the timeout was configured by a user to 0.
>
> The following works:
>
> echo 2147483647 > /sys/class/firmware/timeout
>
> But both of the following set the timeout to MAX_JIFFY_OFFSET even if
> we display 0 back to userspace:
>
> echo 2147483648 > /sys/class/firmware/timeout
> cat /sys/class/firmware/timeout
> 0
>
> echo 0> /sys/class/firmware/timeout
> cat /sys/class/firmware/timeout
> 0
>
> A max value of INT_MAX (2147483647) seconds is therefore implicit due to the
> another cast with simple_strtol().
>
> This fixes the secondary cast (the first one is simple_strtol() but its an
> issue only by forcing an implicit limit) by re-using the timeout variable
> and
> only setting retval in appropriate cases.
>
> Lastly worth noting systemd had ripped out the UMH firmware fallback
> mechanism from udev since udev 2014 via commit be2ea723b1d023b3d
> ("udev: remove userspace firmware loading support"), so as of systemd v217.
>
> Signed-off-by: Yves-Alexis Perez
> Fixes: 68ff2a00dbf5 "firmware_loader: handle timeout via
> wait_for_completion_interruptible_timeout()"
> Cc: Luis R. Rodriguez
> Cc: Ming Lei
> Cc: Bjorn Andersson
> Cc: Greg Kroah-Hartman
> Acked-by: Luis R. Rodriguez
> Reviewed-by: Bjorn Andersson
> [mcg...@kernel.org: gave commit log a whole lot of love]
> Signed-off-by: Luis R. Rodriguez
> Signed-off-by: Greg Kroah-Hartman
> Signed-off-by: Greg Kroah-Hartman
>
> ---
> drivers/base/firmware_class.c |7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -955,13 +955,14 @@ static int _request_firmware_load(struct
> timeout = MAX_JIFFY_OFFSET;
> }
>
> - retval = wait_for_completion_interruptible_timeout(&buf-
> >completion,
> + timeout = wait_for_completion_interruptible_timeout(&buf-
> >completion,
> timeout);
> - if (retval == -ERESTARTSYS || !retval) {
> + if (timeout == -ERESTARTSYS || !timeout) {
> + retval = timeout;
> mutex_lock(&fw_lock);
> fw_load_abort(fw_priv);
> mutex_unlock(&fw_lock);
> - } else if (retval > 0) {
> + } else if (timeout > 0) {
> retval = 0;
> }
>
>
>
--
Yves-Alexis
signature.asc
Description: This is a digitally signed message part
