Hi Greg,
This patch series contains improvements to driver_override handling in
the AMBA bus driver, including two bugfixes that are based on similar
fixes for the PCI and platform buses, and which Todd Kjos would like to
get into the android common branches to fix possible double free.
Changes compared to v1:
- RMK stepped down as amba maintainer.
Thanks!
Geert Uytterhoeven (4):
ARM: amba: Make driver_override output consistent with other buses
ARM: amba: Fix race condition with driver_override
ARM: amba: Don't read past the end of sysfs "driver_override" buffer
ARM: amba: Fix wrong indentation in driver_override_store()
drivers/amba/bus.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
--
2.7.4
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
When printing the driver_override parameter when it is 4095 and 4094
bytes long, the printing code would access invalid memory because we
need count + 1 bytes for printing.
Cfr. commits 4efe874aace57dba ("PCI: Don't read past the end of sysfs
"driver_override" buffer") and bf563b01c2895a4b ("driver core: platform:
Don't read past the end of "driver_override" buffer").
Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
drivers/amba/bus.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 36c5653ced5742b7..4a3ac31c07d0ee49 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -84,7 +84,8 @@ static ssize_t driver_override_store(struct device *_dev,
struct amba_device *dev = to_amba_device(_dev);
char *driver_override, *old, *cp;
- if (count > PATH_MAX)
+ /* We need to keep extra room for a newline */
+ if (count >= (PAGE_SIZE - 1))
return -EINVAL;
driver_override = kstrndup(buf, count, GFP_KERNEL);
--
2.7.4
The driver_override implementation is susceptible to a race condition
when different threads are reading vs storing a different driver
override. Add locking to avoid this race condition.
Cfr. commits 6265539776a0810b ("driver core: platform: fix race
condition with driver_override") and 9561475db680f714 ("PCI: Fix race
condition with driver_override").
Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
drivers/amba/bus.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 6ffd778352e6d953..36c5653ced5742b7 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -69,8 +69,12 @@ static ssize_t driver_override_show(struct device *_dev,
struct device_attribute *attr, char *buf)
{
struct amba_device *dev = to_amba_device(_dev);
+ ssize_t len;
- return sprintf(buf, "%s\n", dev->driver_override);
+ device_lock(_dev);
+ len = sprintf(buf, "%s\n", dev->driver_override);
+ device_unlock(_dev);
+ return len;
}
static ssize_t driver_override_store(struct device *_dev,
@@ -78,7 +82,7 @@ static ssize_t driver_override_store(struct device *_dev,
const char *buf, size_t count)
{
struct amba_device *dev = to_amba_device(_dev);
- char *driver_override, *old = dev->driver_override, *cp;
+ char *driver_override, *old, *cp;
if (count > PATH_MAX)
return -EINVAL;
@@ -91,12 +95,15 @@ static ssize_t driver_override_store(struct device *_dev,
if (cp)
*cp = '\0';
+ device_lock(_dev);
+ old = dev->driver_override;
if (strlen(driver_override)) {
dev->driver_override = driver_override;
} else {
kfree(driver_override);
dev->driver_override = NULL;
}
+ device_unlock(_dev);
kfree(old);
--
2.7.4
For AMBA devices with unconfigured driver override, the
"driver_override" sysfs virtual file is empty, while it contains
"(null)" for platform and PCI devices.
Make AMBA consistent with other buses by dropping the test for a NULL
pointer.
Note that contrary to popular belief, sprintf() handles NULL pointers
fine; they are printed as "(null)".
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
drivers/amba/bus.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 594c228d2f021123..6ffd778352e6d953 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -70,9 +70,6 @@ static ssize_t driver_override_show(struct device *_dev,
{
struct amba_device *dev = to_amba_device(_dev);
- if (!dev->driver_override)
- return 0;
-
return sprintf(buf, "%s\n", dev->driver_override);
}
--
2.7.4
Indentation is one TAB and 7 spaces instead of 2 TABs.
Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
drivers/amba/bus.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 4a3ac31c07d0ee49..842314a439fdd4eb 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -101,8 +101,8 @@ static ssize_t driver_override_store(struct device *_dev,
if (strlen(driver_override)) {
dev->driver_override = driver_override;
} else {
- kfree(driver_override);
- dev->driver_override = NULL;
+ kfree(driver_override);
+ dev->driver_override = NULL;
}
device_unlock(_dev);
--
2.7.4
On Tue, Apr 10, 2018 at 03:21:42PM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
>
> This patch series contains improvements to driver_override handling in
> the AMBA bus driver, including two bugfixes that are based on similar
> fixes for the PCI and platform buses, and which Todd Kjos would like to
> get into the android common branches to fix possible double free.
>
> Changes compared to v1:
> - RMK stepped down as amba maintainer.
No, in the same way as I haven't stepped down as ARM maintainer.
I've said that I'm not going to be putting as much effort in as I
have done, specifically I'm not going to be as active reading the
mailing lists.
This is the first time I've seen your patches, they look fine to
me, so please put them in the patch system as normal.
>
> Thanks!
>
> Geert Uytterhoeven (4):
> ARM: amba: Make driver_override output consistent with other buses
> ARM: amba: Fix race condition with driver_override
> ARM: amba: Don't read past the end of sysfs "driver_override" buffer
> ARM: amba: Fix wrong indentation in driver_override_store()
>
> drivers/amba/bus.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> --
> 2.7.4
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
Reviewed-by: Todd Kjos <[email protected]>
On Tue, Apr 10, 2018 at 6:21 AM, Geert Uytterhoeven
<[email protected]> wrote:
> For AMBA devices with unconfigured driver override, the
> "driver_override" sysfs virtual file is empty, while it contains
> "(null)" for platform and PCI devices.
>
> Make AMBA consistent with other buses by dropping the test for a NULL
> pointer.
>
> Note that contrary to popular belief, sprintf() handles NULL pointers
> fine; they are printed as "(null)".
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> drivers/amba/bus.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 594c228d2f021123..6ffd778352e6d953 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -70,9 +70,6 @@ static ssize_t driver_override_show(struct device *_dev,
> {
> struct amba_device *dev = to_amba_device(_dev);
>
> - if (!dev->driver_override)
> - return 0;
> -
> return sprintf(buf, "%s\n", dev->driver_override);
> }
>
> --
> 2.7.4
>
Reviewed-by: Todd Kjos <[email protected]>
On Tue, Apr 10, 2018 at 6:21 AM, Geert Uytterhoeven
<[email protected]> wrote:
> When printing the driver_override parameter when it is 4095 and 4094
> bytes long, the printing code would access invalid memory because we
> need count + 1 bytes for printing.
>
> Cfr. commits 4efe874aace57dba ("PCI: Don't read past the end of sysfs
> "driver_override" buffer") and bf563b01c2895a4b ("driver core: platform:
> Don't read past the end of "driver_override" buffer").
>
> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> drivers/amba/bus.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 36c5653ced5742b7..4a3ac31c07d0ee49 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -84,7 +84,8 @@ static ssize_t driver_override_store(struct device *_dev,
> struct amba_device *dev = to_amba_device(_dev);
> char *driver_override, *old, *cp;
>
> - if (count > PATH_MAX)
> + /* We need to keep extra room for a newline */
> + if (count >= (PAGE_SIZE - 1))
> return -EINVAL;
>
> driver_override = kstrndup(buf, count, GFP_KERNEL);
> --
> 2.7.4
>
Reviewed-by: Todd Kjos <[email protected]>
On Tue, Apr 10, 2018 at 6:21 AM, Geert Uytterhoeven
<[email protected]> wrote:
> The driver_override implementation is susceptible to a race condition
> when different threads are reading vs storing a different driver
> override. Add locking to avoid this race condition.
>
> Cfr. commits 6265539776a0810b ("driver core: platform: fix race
> condition with driver_override") and 9561475db680f714 ("PCI: Fix race
> condition with driver_override").
>
> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> drivers/amba/bus.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 6ffd778352e6d953..36c5653ced5742b7 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -69,8 +69,12 @@ static ssize_t driver_override_show(struct device *_dev,
> struct device_attribute *attr, char *buf)
> {
> struct amba_device *dev = to_amba_device(_dev);
> + ssize_t len;
>
> - return sprintf(buf, "%s\n", dev->driver_override);
> + device_lock(_dev);
> + len = sprintf(buf, "%s\n", dev->driver_override);
> + device_unlock(_dev);
> + return len;
> }
>
> static ssize_t driver_override_store(struct device *_dev,
> @@ -78,7 +82,7 @@ static ssize_t driver_override_store(struct device *_dev,
> const char *buf, size_t count)
> {
> struct amba_device *dev = to_amba_device(_dev);
> - char *driver_override, *old = dev->driver_override, *cp;
> + char *driver_override, *old, *cp;
>
> if (count > PATH_MAX)
> return -EINVAL;
> @@ -91,12 +95,15 @@ static ssize_t driver_override_store(struct device *_dev,
> if (cp)
> *cp = '\0';
>
> + device_lock(_dev);
> + old = dev->driver_override;
> if (strlen(driver_override)) {
> dev->driver_override = driver_override;
> } else {
> kfree(driver_override);
> dev->driver_override = NULL;
> }
> + device_unlock(_dev);
>
> kfree(old);
>
> --
> 2.7.4
>
Reviewed-by: Todd Kjos <[email protected]>
On Tue, Apr 10, 2018 at 6:21 AM, Geert Uytterhoeven
<[email protected]> wrote:
> Indentation is one TAB and 7 spaces instead of 2 TABs.
>
> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> drivers/amba/bus.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 4a3ac31c07d0ee49..842314a439fdd4eb 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -101,8 +101,8 @@ static ssize_t driver_override_store(struct device *_dev,
> if (strlen(driver_override)) {
> dev->driver_override = driver_override;
> } else {
> - kfree(driver_override);
> - dev->driver_override = NULL;
> + kfree(driver_override);
> + dev->driver_override = NULL;
> }
> device_unlock(_dev);
>
> --
> 2.7.4
>
On Tue, Apr 10, 2018 at 03:21:44PM +0200, Geert Uytterhoeven wrote:
> The driver_override implementation is susceptible to a race condition
> when different threads are reading vs storing a different driver
> override. Add locking to avoid this race condition.
>
> Cfr. commits 6265539776a0810b ("driver core: platform: fix race
> condition with driver_override") and 9561475db680f714 ("PCI: Fix race
> condition with driver_override").
>
> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> Reviewed-by: Todd Kjos <[email protected]>
> Cc: stable <[email protected]>
> ---
> drivers/amba/bus.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 6ffd778352e6d953..36c5653ced5742b7 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -69,8 +69,12 @@ static ssize_t driver_override_show(struct device *_dev,
> struct device_attribute *attr, char *buf)
> {
> struct amba_device *dev = to_amba_device(_dev);
> + ssize_t len;
>
> - return sprintf(buf, "%s\n", dev->driver_override);
> + device_lock(_dev);
> + len = sprintf(buf, "%s\n", dev->driver_override);
> + device_unlock(_dev);
> + return len;
> }
>
> static ssize_t driver_override_store(struct device *_dev,
> @@ -78,7 +82,7 @@ static ssize_t driver_override_store(struct device *_dev,
> const char *buf, size_t count)
> {
> struct amba_device *dev = to_amba_device(_dev);
> - char *driver_override, *old = dev->driver_override, *cp;
> + char *driver_override, *old, *cp;
>
> if (count > PATH_MAX)
> return -EINVAL;
> @@ -91,12 +95,15 @@ static ssize_t driver_override_store(struct device *_dev,
> if (cp)
> *cp = '\0';
>
> + device_lock(_dev);
> + old = dev->driver_override;
> if (strlen(driver_override)) {
> dev->driver_override = driver_override;
> } else {
> kfree(driver_override);
> dev->driver_override = NULL;
> }
> + device_unlock(_dev);
>
> kfree(old);
>
> --
> 2.7.4
As this should go to stable kernels, I've fixed it up to apply without
patch 1 as that's not a real "fix" that anyone needs...
Please try to remember to put fixes first, and then "trivial" things
later on in a series.
thanks,
greg k-h
On Tue, Apr 10, 2018 at 11:19:02PM +0100, Russell King - ARM Linux wrote:
> On Tue, Apr 10, 2018 at 03:21:42PM +0200, Geert Uytterhoeven wrote:
> > Hi Greg,
> >
> > This patch series contains improvements to driver_override handling in
> > the AMBA bus driver, including two bugfixes that are based on similar
> > fixes for the PCI and platform buses, and which Todd Kjos would like to
> > get into the android common branches to fix possible double free.
> >
> > Changes compared to v1:
> > - RMK stepped down as amba maintainer.
>
> No, in the same way as I haven't stepped down as ARM maintainer.
> I've said that I'm not going to be putting as much effort in as I
> have done, specifically I'm not going to be as active reading the
> mailing lists.
>
> This is the first time I've seen your patches, they look fine to
> me, so please put them in the patch system as normal.
I'll take patches 2 and 3 now as they fix reported issues and would be
good to get merged soon.
thanks,
greg k-h
Hi Russell,
(Woops, seems like I forgot to send the email below, while waiting for
a success report for my submission to your patch queue)
On Wed, Apr 11, 2018 at 12:19 AM, Russell King - ARM Linux
<[email protected]> wrote:
> On Tue, Apr 10, 2018 at 03:21:42PM +0200, Geert Uytterhoeven wrote:
>> This patch series contains improvements to driver_override handling in
>> the AMBA bus driver, including two bugfixes that are based on similar
>> fixes for the PCI and platform buses, and which Todd Kjos would like to
>> get into the android common branches to fix possible double free.
>>
>> Changes compared to v1:
>> - RMK stepped down as amba maintainer.
>
> No, in the same way as I haven't stepped down as ARM maintainer.
> I've said that I'm not going to be putting as much effort in as I
> have done, specifically I'm not going to be as active reading the
> mailing lists.
Good to hear that! Thanks for your (continued) work!
> This is the first time I've seen your patches, they look fine to
> me, so please put them in the patch system as normal.
Thanks, done.
>> Geert Uytterhoeven (4):
>> ARM: amba: Make driver_override output consistent with other buses
>> ARM: amba: Fix race condition with driver_override
>> ARM: amba: Don't read past the end of sysfs "driver_override" buffer
>> ARM: amba: Fix wrong indentation in driver_override_store()
>>
>> drivers/amba/bus.c | 21 +++++++++++++--------
>> 1 file changed, 13 insertions(+), 8 deletions(-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Greg,
On Wed, Apr 25, 2018 at 6:06 PM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Tue, Apr 10, 2018 at 03:21:44PM +0200, Geert Uytterhoeven wrote:
>> The driver_override implementation is susceptible to a race condition
>> when different threads are reading vs storing a different driver
>> override. Add locking to avoid this race condition.
>>
>> Cfr. commits 6265539776a0810b ("driver core: platform: fix race
>> condition with driver_override") and 9561475db680f714 ("PCI: Fix race
>> condition with driver_override").
>>
>> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
>> Signed-off-by: Geert Uytterhoeven <[email protected]>
>> Reviewed-by: Todd Kjos <[email protected]>
>> Cc: stable <[email protected]>
> As this should go to stable kernels, I've fixed it up to apply without
> patch 1 as that's not a real "fix" that anyone needs...
>
> Please try to remember to put fixes first, and then "trivial" things
> later on in a series.
I did it on purpose, as the fix is much more ugly without patch 1 applied.
Can't you just take patch 1, too? More consistency is always nice, even for
stable ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, Apr 25, 2018 at 07:53:06PM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
>
> On Wed, Apr 25, 2018 at 6:06 PM, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Tue, Apr 10, 2018 at 03:21:44PM +0200, Geert Uytterhoeven wrote:
> >> The driver_override implementation is susceptible to a race condition
> >> when different threads are reading vs storing a different driver
> >> override. Add locking to avoid this race condition.
> >>
> >> Cfr. commits 6265539776a0810b ("driver core: platform: fix race
> >> condition with driver_override") and 9561475db680f714 ("PCI: Fix race
> >> condition with driver_override").
> >>
> >> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
> >> Signed-off-by: Geert Uytterhoeven <[email protected]>
> >> Reviewed-by: Todd Kjos <[email protected]>
> >> Cc: stable <[email protected]>
>
> > As this should go to stable kernels, I've fixed it up to apply without
> > patch 1 as that's not a real "fix" that anyone needs...
> >
> > Please try to remember to put fixes first, and then "trivial" things
> > later on in a series.
>
> I did it on purpose, as the fix is much more ugly without patch 1 applied.
> Can't you just take patch 1, too? More consistency is always nice, even for
> stable ;-)
Consistency is nice, but when you have bug fixes that rely on "trivial"
patches, it's usually not nice :(
I already committed patch 2 to my tree without 1, so let's leave it
as-is for now.
thanks,
greg k-h
Hi Greg,
On Thu, Apr 26, 2018 at 9:04 AM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Wed, Apr 25, 2018 at 07:53:06PM +0200, Geert Uytterhoeven wrote:
>> On Wed, Apr 25, 2018 at 6:06 PM, Greg Kroah-Hartman
>> <[email protected]> wrote:
>> > On Tue, Apr 10, 2018 at 03:21:44PM +0200, Geert Uytterhoeven wrote:
>> >> The driver_override implementation is susceptible to a race condition
>> >> when different threads are reading vs storing a different driver
>> >> override. Add locking to avoid this race condition.
>> >>
>> >> Cfr. commits 6265539776a0810b ("driver core: platform: fix race
>> >> condition with driver_override") and 9561475db680f714 ("PCI: Fix race
>> >> condition with driver_override").
>> >>
>> >> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
>> >> Signed-off-by: Geert Uytterhoeven <[email protected]>
>> >> Reviewed-by: Todd Kjos <[email protected]>
>> >> Cc: stable <[email protected]>
>>
>> > As this should go to stable kernels, I've fixed it up to apply without
>> > patch 1 as that's not a real "fix" that anyone needs...
>> >
>> > Please try to remember to put fixes first, and then "trivial" things
>> > later on in a series.
>>
>> I did it on purpose, as the fix is much more ugly without patch 1 applied.
>> Can't you just take patch 1, too? More consistency is always nice, even for
>> stable ;-)
>
> Consistency is nice, but when you have bug fixes that rely on "trivial"
> patches, it's usually not nice :(
>
> I already committed patch 2 to my tree without 1, so let's leave it
> as-is for now.
Unfortunately the version you committed is buggy: the race condition
also covers the NULL check removed by the trivial patch you skipped,
so now you can get inconsistent behavior (no output or "(null)") on the
same running kernel version...
Please revert and apply both. Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Apr 26, 2018 at 09:40:08AM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
>
> On Thu, Apr 26, 2018 at 9:04 AM, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Wed, Apr 25, 2018 at 07:53:06PM +0200, Geert Uytterhoeven wrote:
> >> On Wed, Apr 25, 2018 at 6:06 PM, Greg Kroah-Hartman
> >> <[email protected]> wrote:
> >> > On Tue, Apr 10, 2018 at 03:21:44PM +0200, Geert Uytterhoeven wrote:
> >> >> The driver_override implementation is susceptible to a race condition
> >> >> when different threads are reading vs storing a different driver
> >> >> override. Add locking to avoid this race condition.
> >> >>
> >> >> Cfr. commits 6265539776a0810b ("driver core: platform: fix race
> >> >> condition with driver_override") and 9561475db680f714 ("PCI: Fix race
> >> >> condition with driver_override").
> >> >>
> >> >> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
> >> >> Signed-off-by: Geert Uytterhoeven <[email protected]>
> >> >> Reviewed-by: Todd Kjos <[email protected]>
> >> >> Cc: stable <[email protected]>
> >>
> >> > As this should go to stable kernels, I've fixed it up to apply without
> >> > patch 1 as that's not a real "fix" that anyone needs...
> >> >
> >> > Please try to remember to put fixes first, and then "trivial" things
> >> > later on in a series.
> >>
> >> I did it on purpose, as the fix is much more ugly without patch 1 applied.
> >> Can't you just take patch 1, too? More consistency is always nice, even for
> >> stable ;-)
> >
> > Consistency is nice, but when you have bug fixes that rely on "trivial"
> > patches, it's usually not nice :(
> >
> > I already committed patch 2 to my tree without 1, so let's leave it
> > as-is for now.
>
> Unfortunately the version you committed is buggy: the race condition
> also covers the NULL check removed by the trivial patch you skipped,
> so now you can get inconsistent behavior (no output or "(null)") on the
> same running kernel version...
>
> Please revert and apply both. Thanks!
Ugh, you are right, sorry about that.
I've reverted the offending patch, and added them in the correct order
now, I should have listened to you :)
greg k-h
Hi Greg,
On Thu, Apr 26, 2018 at 10:35 AM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Thu, Apr 26, 2018 at 09:40:08AM +0200, Geert Uytterhoeven wrote:
>> On Thu, Apr 26, 2018 at 9:04 AM, Greg Kroah-Hartman
>> <[email protected]> wrote:
>> > On Wed, Apr 25, 2018 at 07:53:06PM +0200, Geert Uytterhoeven wrote:
>> >> On Wed, Apr 25, 2018 at 6:06 PM, Greg Kroah-Hartman
>> >> <[email protected]> wrote:
>> >> > On Tue, Apr 10, 2018 at 03:21:44PM +0200, Geert Uytterhoeven wrote:
>> >> >> The driver_override implementation is susceptible to a race condition
>> >> >> when different threads are reading vs storing a different driver
>> >> >> override. Add locking to avoid this race condition.
>> >> >>
>> >> >> Cfr. commits 6265539776a0810b ("driver core: platform: fix race
>> >> >> condition with driver_override") and 9561475db680f714 ("PCI: Fix race
>> >> >> condition with driver_override").
>> >> >>
>> >> >> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
>> >> >> Signed-off-by: Geert Uytterhoeven <[email protected]>
>> >> >> Reviewed-by: Todd Kjos <[email protected]>
>> >> >> Cc: stable <[email protected]>
>> >>
>> >> > As this should go to stable kernels, I've fixed it up to apply without
>> >> > patch 1 as that's not a real "fix" that anyone needs...
>> >> >
>> >> > Please try to remember to put fixes first, and then "trivial" things
>> >> > later on in a series.
>> >>
>> >> I did it on purpose, as the fix is much more ugly without patch 1 applied.
>> >> Can't you just take patch 1, too? More consistency is always nice, even for
>> >> stable ;-)
>> >
>> > Consistency is nice, but when you have bug fixes that rely on "trivial"
>> > patches, it's usually not nice :(
>> >
>> > I already committed patch 2 to my tree without 1, so let's leave it
>> > as-is for now.
>>
>> Unfortunately the version you committed is buggy: the race condition
>> also covers the NULL check removed by the trivial patch you skipped,
>> so now you can get inconsistent behavior (no output or "(null)") on the
>> same running kernel version...
>>
>> Please revert and apply both. Thanks!
>
> Ugh, you are right, sorry about that.
>
> I've reverted the offending patch, and added them in the correct order
> now, I should have listened to you :)
Np, issue detected and fixed.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Apr 26, 2018 at 10:45:49AM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
>
> On Thu, Apr 26, 2018 at 10:35 AM, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Thu, Apr 26, 2018 at 09:40:08AM +0200, Geert Uytterhoeven wrote:
> >> On Thu, Apr 26, 2018 at 9:04 AM, Greg Kroah-Hartman
> >> <[email protected]> wrote:
> >> > On Wed, Apr 25, 2018 at 07:53:06PM +0200, Geert Uytterhoeven wrote:
> >> >> On Wed, Apr 25, 2018 at 6:06 PM, Greg Kroah-Hartman
> >> >> <[email protected]> wrote:
> >> >> > On Tue, Apr 10, 2018 at 03:21:44PM +0200, Geert Uytterhoeven wrote:
> >> >> >> The driver_override implementation is susceptible to a race condition
> >> >> >> when different threads are reading vs storing a different driver
> >> >> >> override. Add locking to avoid this race condition.
> >> >> >>
> >> >> >> Cfr. commits 6265539776a0810b ("driver core: platform: fix race
> >> >> >> condition with driver_override") and 9561475db680f714 ("PCI: Fix race
> >> >> >> condition with driver_override").
> >> >> >>
> >> >> >> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
> >> >> >> Signed-off-by: Geert Uytterhoeven <[email protected]>
> >> >> >> Reviewed-by: Todd Kjos <[email protected]>
> >> >> >> Cc: stable <[email protected]>
> >> >>
> >> >> > As this should go to stable kernels, I've fixed it up to apply without
> >> >> > patch 1 as that's not a real "fix" that anyone needs...
> >> >> >
> >> >> > Please try to remember to put fixes first, and then "trivial" things
> >> >> > later on in a series.
> >> >>
> >> >> I did it on purpose, as the fix is much more ugly without patch 1 applied.
> >> >> Can't you just take patch 1, too? More consistency is always nice, even for
> >> >> stable ;-)
> >> >
> >> > Consistency is nice, but when you have bug fixes that rely on "trivial"
> >> > patches, it's usually not nice :(
> >> >
> >> > I already committed patch 2 to my tree without 1, so let's leave it
> >> > as-is for now.
> >>
> >> Unfortunately the version you committed is buggy: the race condition
> >> also covers the NULL check removed by the trivial patch you skipped,
> >> so now you can get inconsistent behavior (no output or "(null)") on the
> >> same running kernel version...
> >>
> >> Please revert and apply both. Thanks!
> >
> > Ugh, you are right, sorry about that.
> >
> > I've reverted the offending patch, and added them in the correct order
> > now, I should have listened to you :)
>
> Np, issue detected and fixed.
> Thanks!
So what about the patches you submitted to the patch system - should
I pick those up or not?
Please don't ask other maintainers to take patches that have been
submitted to the patch system without first changing their status,
they're liable to get applied anyway.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
Hi Russell,
On Wed, May 9, 2018 at 12:39 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Thu, Apr 26, 2018 at 10:45:49AM +0200, Geert Uytterhoeven wrote:
>> On Thu, Apr 26, 2018 at 10:35 AM, Greg Kroah-Hartman
>> <[email protected]> wrote:
>> > On Thu, Apr 26, 2018 at 09:40:08AM +0200, Geert Uytterhoeven wrote:
>> >> On Thu, Apr 26, 2018 at 9:04 AM, Greg Kroah-Hartman
>> >> <[email protected]> wrote:
>> >> > On Wed, Apr 25, 2018 at 07:53:06PM +0200, Geert Uytterhoeven wrote:
>> >> >> On Wed, Apr 25, 2018 at 6:06 PM, Greg Kroah-Hartman
>> >> >> <[email protected]> wrote:
>> >> >> > On Tue, Apr 10, 2018 at 03:21:44PM +0200, Geert Uytterhoeven wrote:
>> >> >> >> The driver_override implementation is susceptible to a race condition
>> >> >> >> when different threads are reading vs storing a different driver
>> >> >> >> override. Add locking to avoid this race condition.
>> >> >> >>
>> >> >> >> Cfr. commits 6265539776a0810b ("driver core: platform: fix race
>> >> >> >> condition with driver_override") and 9561475db680f714 ("PCI: Fix race
>> >> >> >> condition with driver_override").
>> >> >> >>
>> >> >> >> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
>> >> >> >> Signed-off-by: Geert Uytterhoeven <[email protected]>
>> >> >> >> Reviewed-by: Todd Kjos <[email protected]>
>> >> >> >> Cc: stable <[email protected]>
>> >> >>
>> >> >> > As this should go to stable kernels, I've fixed it up to apply without
>> >> >> > patch 1 as that's not a real "fix" that anyone needs...
>> >> >> >
>> >> >> > Please try to remember to put fixes first, and then "trivial" things
>> >> >> > later on in a series.
>> >> >>
>> >> >> I did it on purpose, as the fix is much more ugly without patch 1 applied.
>> >> >> Can't you just take patch 1, too? More consistency is always nice, even for
>> >> >> stable ;-)
>> >> >
>> >> > Consistency is nice, but when you have bug fixes that rely on "trivial"
>> >> > patches, it's usually not nice :(
>> >> >
>> >> > I already committed patch 2 to my tree without 1, so let's leave it
>> >> > as-is for now.
>> >>
>> >> Unfortunately the version you committed is buggy: the race condition
>> >> also covers the NULL check removed by the trivial patch you skipped,
>> >> so now you can get inconsistent behavior (no output or "(null)") on the
>> >> same running kernel version...
>> >>
>> >> Please revert and apply both. Thanks!
>> >
>> > Ugh, you are right, sorry about that.
>> >
>> > I've reverted the offending patch, and added them in the correct order
>> > now, I should have listened to you :)
>>
>> Np, issue detected and fixed.
>> Thanks!
>
> So what about the patches you submitted to the patch system - should
> I pick those up or not?
I think only the 4th patch (#8759) in the series is still applicable.
> Please don't ask other maintainers to take patches that have been
> submitted to the patch system without first changing their status,
> they're liable to get applied anyway.
They got picked up by Greg, on request of a third party who wanted them in
-stable ASAP. Not much I can do to prevent that.
Especially with an "Odd Fixes" maintainership status.
I tried to change the status of the patches Greg applied, but it failed:
Your request to update the patch failed because:
An internal state error was detected.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, May 09, 2018 at 03:32:16PM +0200, Geert Uytterhoeven wrote:
> Hi Russell,
>
> On Wed, May 9, 2018 at 12:39 PM, Russell King - ARM Linux
> <[email protected]> wrote:
> > On Thu, Apr 26, 2018 at 10:45:49AM +0200, Geert Uytterhoeven wrote:
> >> On Thu, Apr 26, 2018 at 10:35 AM, Greg Kroah-Hartman
> >> <[email protected]> wrote:
> >> > On Thu, Apr 26, 2018 at 09:40:08AM +0200, Geert Uytterhoeven wrote:
> >> >> On Thu, Apr 26, 2018 at 9:04 AM, Greg Kroah-Hartman
> >> >> <[email protected]> wrote:
> >> >> > On Wed, Apr 25, 2018 at 07:53:06PM +0200, Geert Uytterhoeven wrote:
> >> >> >> On Wed, Apr 25, 2018 at 6:06 PM, Greg Kroah-Hartman
> >> >> >> <[email protected]> wrote:
> >> >> >> > On Tue, Apr 10, 2018 at 03:21:44PM +0200, Geert Uytterhoeven wrote:
> >> >> >> >> The driver_override implementation is susceptible to a race condition
> >> >> >> >> when different threads are reading vs storing a different driver
> >> >> >> >> override. Add locking to avoid this race condition.
> >> >> >> >>
> >> >> >> >> Cfr. commits 6265539776a0810b ("driver core: platform: fix race
> >> >> >> >> condition with driver_override") and 9561475db680f714 ("PCI: Fix race
> >> >> >> >> condition with driver_override").
> >> >> >> >>
> >> >> >> >> Fixes: 3cf385713460eb2b ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'")
> >> >> >> >> Signed-off-by: Geert Uytterhoeven <[email protected]>
> >> >> >> >> Reviewed-by: Todd Kjos <[email protected]>
> >> >> >> >> Cc: stable <[email protected]>
> >> >> >>
> >> >> >> > As this should go to stable kernels, I've fixed it up to apply without
> >> >> >> > patch 1 as that's not a real "fix" that anyone needs...
> >> >> >> >
> >> >> >> > Please try to remember to put fixes first, and then "trivial" things
> >> >> >> > later on in a series.
> >> >> >>
> >> >> >> I did it on purpose, as the fix is much more ugly without patch 1 applied.
> >> >> >> Can't you just take patch 1, too? More consistency is always nice, even for
> >> >> >> stable ;-)
> >> >> >
> >> >> > Consistency is nice, but when you have bug fixes that rely on "trivial"
> >> >> > patches, it's usually not nice :(
> >> >> >
> >> >> > I already committed patch 2 to my tree without 1, so let's leave it
> >> >> > as-is for now.
> >> >>
> >> >> Unfortunately the version you committed is buggy: the race condition
> >> >> also covers the NULL check removed by the trivial patch you skipped,
> >> >> so now you can get inconsistent behavior (no output or "(null)") on the
> >> >> same running kernel version...
> >> >>
> >> >> Please revert and apply both. Thanks!
> >> >
> >> > Ugh, you are right, sorry about that.
> >> >
> >> > I've reverted the offending patch, and added them in the correct order
> >> > now, I should have listened to you :)
> >>
> >> Np, issue detected and fixed.
> >> Thanks!
> >
> > So what about the patches you submitted to the patch system - should
> > I pick those up or not?
>
> I think only the 4th patch (#8759) in the series is still applicable.
>
> > Please don't ask other maintainers to take patches that have been
> > submitted to the patch system without first changing their status,
> > they're liable to get applied anyway.
>
> They got picked up by Greg, on request of a third party who wanted them in
> -stable ASAP. Not much I can do to prevent that.
> Especially with an "Odd Fixes" maintainership status.
I was going to pick up the last one if needed and put it through my
tree, as it's still in my queue.
thanks,
greg k-h