Re: [PATCH v2 2/4] ARM: amba: Fix race condition with driver_override

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>,
	Russell King <linux@armlinux.org.uk>,
	Adrian Salido <salidoa@google.com>,
	Nicolai Stange <nstange@suse.de>,
	Sasha Levin <Alexander.Levin@microsoft.com>,
	Todd Kjos <tkjos@android.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/4] ARM: amba: Fix race condition with driver_override
Date: Thu, 26 Apr 2018 09:40:08 +0200	[thread overview]
Message-ID: <CAMuHMdWLCQoHzuW1UZNP9gdRV3EE6_yPcdyX1NDy65q7Nt4tZw@mail.gmail.com> (raw)
In-Reply-To: <20180426070410.GM14025@kroah.com>

Hi Greg,

On Thu, Apr 26, 2018 at 9:04 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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
>> <gregkh@linuxfoundation.org> 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 <geert+renesas@glider.be>
>> >> Reviewed-by: Todd Kjos <tkjos@google.com>
>> >> Cc: stable <stable@vger.kernel.org>
>>
>> > 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 -- geert@linux-m68k.org

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