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

Groups > linux.kernel > #1180354 > unrolled thread

Re: Linux 4.2-rc1

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Linux 4.2-rc1 Ming Lei <ming.lei@canonical.com> - 2015-07-09 03:00 +0200
    Re: Linux 4.2-rc1 Shuah Khan <shuahkh@osg.samsung.com> - 2015-07-09 15:20 +0200
      Re: Linux 4.2-rc1 Shuah Khan <shuahkh@osg.samsung.com> - 2015-07-10 19:00 +0200
        Re: Linux 4.2-rc1 Linus Torvalds <torvalds@linux-foundation.org> - 2015-07-10 19:20 +0200
          Re: Linux 4.2-rc1 Ming Lei <ming.lei@canonical.com> - 2015-07-11 00:40 +0200

#1180354 — Re: Linux 4.2-rc1

On Thu, Jul 9, 2015 at 1:29 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Jul 8, 2015 at 10:17 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Decoding the "Code:" line shows that this is the "->fw_id" dereference in
>>
>>         if (add_uevent_var(env, "FIRMWARE=%s", fw_priv->buf->fw_id))
>>                 return -ENOMEM;
>>
>> and that "fw_priv->buf" pointer is NULL.
>>
>> However, I don't see anything that looks like it should have changed
>> any of this since 4.1.
>
> Looking at the otehr uses of "fw_priv->buf", they all check that
> pointer for NULL. I see code like
>
>         fw_buf = fw_priv->buf;
>         if (!fw_buf)
>                 goto out;
>
> etc.
>
> Also, it looks like you need to hold the "fw_lock" to even look at
> that pointer, since the buffer can get reallocated etc.


Yes, the above code with holding 'fw_lock' is right fix for the issue since
sysfs read can happen anytime, and there is one race between firmware
request abort and reading uevent of sysfs.


> So that uevent code really looks buggy. It just doesn't look like a
> *new* bug to me. That code looks old, going back to 2012 and commit
> 1244691c73b2.

Exactly.

Thanks,
Ming
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1180743

On 07/08/2015 09:17 PM, Linus Torvalds wrote:
> On Wed, Jul 8, 2015 at 5:58 PM, Ming Lei <ming.lei@canonical.com> wrote:
>> On Thu, Jul 9, 2015 at 1:29 AM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>> Also, it looks like you need to hold the "fw_lock" to even look at
>>> that pointer, since the buffer can get reallocated etc.
>>
>> Yes, the above code with holding 'fw_lock' is right fix for the issue since
>> sysfs read can happen anytime, and there is one race between firmware
>> request abort and reading uevent of sysfs.
> 
> So if fw_priv->buf is NULL, what should we do?
> 
> Should we skip the TIMEOUT= and ASYNC= fields too?
> 
> Something like the attached, perhaps?
> 
> Shuah, how reproducible is this? Does this (completely untested) patch
> make any difference?
> 

Happened both times I booted 4.2-rc1 up, so I would say 100% so far.
I will test with your patch and report results.

-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1181747

On 07/09/2015 11:40 AM, Shuah Khan wrote:
> On 07/09/2015 08:45 AM, Greg Kroah-Hartman wrote:
>> On Thu, Jul 09, 2015 at 07:44:04AM -0600, Shuah Khan wrote:
>>> On 07/09/2015 07:10 AM, Shuah Khan wrote:
>>>> On 07/08/2015 09:17 PM, Linus Torvalds wrote:
>>>>> On Wed, Jul 8, 2015 at 5:58 PM, Ming Lei <ming.lei@canonical.com> wrote:
>>>>>> On Thu, Jul 9, 2015 at 1:29 AM, Linus Torvalds
>>>>>> <torvalds@linux-foundation.org> wrote:
>>>>>>> Also, it looks like you need to hold the "fw_lock" to even look at
>>>>>>> that pointer, since the buffer can get reallocated etc.
>>>>>>
>>>>>> Yes, the above code with holding 'fw_lock' is right fix for the issue since
>>>>>> sysfs read can happen anytime, and there is one race between firmware
>>>>>> request abort and reading uevent of sysfs.
>>>>>
>>>>> So if fw_priv->buf is NULL, what should we do?
>>>>>
>>>>> Should we skip the TIMEOUT= and ASYNC= fields too?
>>>>>
>>>>> Something like the attached, perhaps?
>>>>>
>>>>> Shuah, how reproducible is this? Does this (completely untested) patch
>>>>> make any difference?
>>>>>
>>>>
>>>> Happened both times I booted 4.2-rc1 up, so I would say 100% so far.
>>>> I will test with your patch and report results.
>>>>
>>>
>>> Yes. This patch fixed the problem.
>>
>> That's great, but what changed recently to cause this problem to happen?
>> Any chance you can bisect to the problem commit?
>>
> 
> :) Starting bisect now. Thankfully I built 4.1.0 in this tree
> which is good place to start. Let you know in a bit
> 

Ok here is the bisect result. Did I mention "I really love bisect
on the very first rc after the merge window" :)

I am not sure why this patch would cause the problem I am seeing.
This patch itself looks like a cleanup type patch and doesn't
really fix a bug. I am building with this patch reverted at the
moment to confirm. In the meantime:


eaa5cd926345f86e9df1eb6b0490da539f5ce7d0 is the first bad commit
commit eaa5cd926345f86e9df1eb6b0490da539f5ce7d0
Author: Vladimir Zapolskiy <vz@mleia.com>
Date:   Fri May 22 00:21:16 2015 +0300

    fs: sysfs: don't pass count == 0 to bin file readers

    If count == 0 bytes are requested by a reader, sysfs_kf_bin_read()
    deliberately returns 0 without passing a potentially harmful value to
    some externally defined underlying battr->read() function.

    However in case of (pos == size && count) the next clause always sets
    count to 0 and this value is handed over to battr->read().

    The change intends to make obsolete (and remove later) a redundant
    sanity check in battr->read(), if it is present, or add more
    protection to struct bin_attribute users, who does not care about
    input arguments.

    Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
    Acked-by: Tejun Heo <tj@kernel.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

:040000 040000 aca6ce463db111e3b3ef3fe4ed799a670618b1c4
2aad70773e76e899561ef4a8851174ae87793fed M	fs


Complete bisect log:


git bisect start
# bad: [d6ac4ffc61ace6ed6f183e9fd7f207c0ddafb897] Merge branch
'for-linus' of git://ftp.arm.linux.org.uk/~rmk/linux-arm
git bisect bad d6ac4ffc61ace6ed6f183e9fd7f207c0ddafb897
# good: [b953c0d234bc72e8489d3bf51a276c5c4ec85345] Linux 4.1
git bisect good b953c0d234bc72e8489d3bf51a276c5c4ec85345
# good: [4570a37169d4b44d316f40b2ccc681dc93fedc7b] Merge tag
'sound-4.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
git bisect good 4570a37169d4b44d316f40b2ccc681dc93fedc7b
# bad: [8d7804a2f03dbd34940fcb426450c730adf29dae] Merge tag
'driver-core-4.2-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core
git bisect bad 8d7804a2f03dbd34940fcb426450c730adf29dae
# good: [3d9f96d850e4bbfae24dc9aee03033dd77c81596] Merge tag 'armsoc-dt'
of git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc
git bisect good 3d9f96d850e4bbfae24dc9aee03033dd77c81596
# good: [692a59e696afe1a4e777d0e4359325336ab0ad89] drm/amdgpu: remove
AMDGPU_CTX_OP_STATE_RUNNING
git bisect good 692a59e696afe1a4e777d0e4359325336ab0ad89
# good: [099bfbfc7fbbe22356c02f0caf709ac32e1126ea] Merge branch
'drm-next' of git://people.freedesktop.org/~airlied/linux
git bisect good 099bfbfc7fbbe22356c02f0caf709ac32e1126ea
# good: [19a4fb21f804dbd5a327eba7a1569b6b8e941a54] i2c-parport: define
ports to connect
git bisect good 19a4fb21f804dbd5a327eba7a1569b6b8e941a54
# good: [3dc196eae1db548f05e53e5875ff87b8ff79f249] mei: me: wait for
power gating exit confirmation
git bisect good 3dc196eae1db548f05e53e5875ff87b8ff79f249
# good: [e382608254e06c8109f40044f5e693f2e04f3899] Merge tag
'trace-v4.2' of
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace
git bisect good e382608254e06c8109f40044f5e693f2e04f3899
# good: [f5727b05d221796baf69667ed5c891d4bd53711e] firmware: fix
__getname() missing failure check
git bisect good f5727b05d221796baf69667ed5c891d4bd53711e
# good: [9ba8af66432cb8e82553f2e273eb11db0cec7d2d] base:dd - Fix for
typo in comment to function driver_deferred_probe_trigger().
git bisect good 9ba8af66432cb8e82553f2e273eb11db0cec7d2d
# bad: [8b2dcebae330fb6dffc7717b740aa4b2c4d00451] Revert "base/platform:
Remove code duplication"
git bisect bad 8b2dcebae330fb6dffc7717b740aa4b2c4d00451
# bad: [303cda0ea7c1c33701812ccb80d37083a4093c7c] firmware: add missing
kfree for work on async call
git bisect bad 303cda0ea7c1c33701812ccb80d37083a4093c7c
# bad: [eaa5cd926345f86e9df1eb6b0490da539f5ce7d0] fs: sysfs: don't pass
count == 0 to bin file readers
git bisect bad eaa5cd926345f86e9df1eb6b0490da539f5ce7d0
# first bad commit: [eaa5cd926345f86e9df1eb6b0490da539f5ce7d0] fs:
sysfs: don't pass count == 0 to bin file readers

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1181772

On Fri, Jul 10, 2015 at 9:58 AM, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>
> I am not sure why this patch would cause the problem I am seeing.
> This patch itself looks like a cleanup type patch and doesn't
> really fix a bug. I am building with this patch reverted at the
> moment to confirm.

Smells to me like it's just a timing issue, and that mayeb the bisect
failed because it's not 100% repeatable. Or maybe it *was* 100%
repeatable, but simply because that commit changed the timing of the
bootup scripts etc.

But yes, trying it with the revert in place is a good idea just to
make sure. And perhaps checking that kernel more than a few times to
verify just how repeatable it is.

           Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1181972

On Sat, Jul 11, 2015 at 5:47 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> But my patch (which is committed now) solves it all for you?
>
> I'm going to just assume it's timing, and there is no major real reason why
> it started triggering just now...

Now I see it, the issue is triggered when firmware request is
timed out, and Shuah's reported should be caused by the following
commit 0cb64249(firmware_loader: abort request if wait_for_completion
is interrupted).

But your patch is correct for this issue too.

Thanks,
Ming

>
>      Linus
>
> On Jul 10, 2015 2:33 PM, "Shuah Khan" <shuahkh@osg.samsung.com> wrote:
>>
>> On 07/10/2015 11:11 AM, Linus Torvalds wrote:
>> > On Fri, Jul 10, 2015 at 9:58 AM, Shuah Khan <shuahkh@osg.samsung.com>
>> > wrote:
>> >>
>> >> I am not sure why this patch would cause the problem I am seeing.
>> >> This patch itself looks like a cleanup type patch and doesn't
>> >> really fix a bug. I am building with this patch reverted at the
>> >> moment to confirm.
>> >
>> > Smells to me like it's just a timing issue, and that mayeb the bisect
>> > failed because it's not 100% repeatable. Or maybe it *was* 100%
>> > repeatable, but simply because that commit changed the timing of the
>> > bootup scripts etc.
>> >
>> > But yes, trying it with the revert in place is a good idea just to
>> > make sure. And perhaps checking that kernel more than a few times to
>> > verify just how repeatable it is.
>> >
>>
>> Quick update. Reverting didn't help. I think I mentioned I am seeing
>> hangs during poweroff and reboot. I am seeing hangs during boot as well.
>> I think there is a timing problem that manifests into the following
>> 3 variations:
>>
>> 1. NULL pointer dereference alert, boots fine and runs fine - hangs
>>    during poweroff and reboot
>> 2. Hangs during boot. When booted in recovery, it runs into repeated
>>    errors which looks very much like the same call trace I see in the
>>    alert.
>>
>> Please see attached images. These two are rolling failures repeated
>> during udev initialization. It is related to firmware loading it looks
>> like.
>>
>> thanks,
>> -- Shuah
>>
>> --
>> Shuah Khan
>> Sr. Linux Kernel Developer
>> Open Source Innovation Group
>> Samsung Research America (Silicon Valley)
>> shuahkh@osg.samsung.com | (970) 217-8978
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web