Hi,
I noticed that nios2 images crash in mainline. Bisect points to commit
33d72f3822d7 ("init/main.c: extract early boot entropy from the passed
cmdline"). Bisect log is attached.
As far as I can see, the problem is seen because add_device_randomness()
calls random_get_entropy(). However, the underlying timer function
used by the nios2 architecture (nios2_timer_read) is not yet initialized,
causing a NULL pointer access and crash. A sample crash log is at
http://kerneltests.org/builders/qemu-nios2-master/builds/175/steps/qemubuildcommand/logs/stdio
Guenter
---
# bad: [4dfc2788033d30dfccfd4268e06dd73ce2c654ed] Merge tag 'iommu-updates-v4.14' of git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu
# good: [5969d1bb3082b41eba8fd2c826559abe38ccb6df] Merge branch 'gperf-removal'
git bisect start 'HEAD' '5969d1bb3082'
# bad: [fbd01410e89a66f346ba1b3c0161e1198449b746] Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
git bisect bad fbd01410e89a66f346ba1b3c0161e1198449b746
# good: [0d519f2d1ed1f11e49abc88cfcf6cf13b83ba14c] Merge tag 'pci-v4.14-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci
git bisect good 0d519f2d1ed1f11e49abc88cfcf6cf13b83ba14c
# bad: [229cf16d3c8ac2e9b082c223fd0e619dc8f62cc1] sh: defconfig: cleanup from old Kconfig options
git bisect bad 229cf16d3c8ac2e9b082c223fd0e619dc8f62cc1
# good: [1caffba9db4aa27c3e7ebc05668afca1f991ab8d] drivers/scsi/sym53c8xx_2/sym_hipd.c: convert to use memset32
git bisect good 1caffba9db4aa27c3e7ebc05668afca1f991ab8d
# good: [7c61bd6983b185272315722787cceacf5f5d2e7d] lib/cmdline.c: remove meaningless comment
git bisect good 7c61bd6983b185272315722787cceacf5f5d2e7d
# bad: [718b303b49893d8f9dd469f710d76f77673bd35d] autofs: use AUTOFS_DEV_IOCTL_SIZE
git bisect bad 718b303b49893d8f9dd469f710d76f77673bd35d
# good: [9367bb730e4d9d85a8911a08a3542ec2aa873d37] binfmt_flat: delete two error messages for a failed memory allocation in decompress_exec()
git bisect good 9367bb730e4d9d85a8911a08a3542ec2aa873d37
# bad: [e54c7bcbf14a25dc3a913b4c808b52121c522e9b] autofs: make disc device user accessible
git bisect bad e54c7bcbf14a25dc3a913b4c808b52121c522e9b
# bad: [33d72f3822d7ff8a9e45bd7413c811085cb87aa5] init/main.c: extract early boot entropy from the passed cmdline
git bisect bad 33d72f3822d7ff8a9e45bd7413c811085cb87aa5
# good: [121388a31362b0d3176dc1190ac8064b98a61b20] init: move stack canary initialization after setup_arch
git bisect good 121388a31362b0d3176dc1190ac8064b98a61b20
# first bad commit: [33d72f3822d7ff8a9e45bd7413c811085cb87aa5] init/main.c: extract early boot entropy from the passed cmdline
On Sat, Sep 9, 2017 at 8:58 PM, Guenter Roeck <[email protected]> wrote:
> Hi,
>
> I noticed that nios2 images crash in mainline. Bisect points to commit
> 33d72f3822d7 ("init/main.c: extract early boot entropy from the passed
> cmdline"). Bisect log is attached.
>
> As far as I can see, the problem is seen because add_device_randomness()
> calls random_get_entropy(). However, the underlying timer function
> used by the nios2 architecture (nios2_timer_read) is not yet initialized,
> causing a NULL pointer access and crash. A sample crash log is at
> http://kerneltests.org/builders/qemu-nios2-master/builds/175/steps/qemubuildcommand/logs/stdio
Oh, yikes. Do you have a full call trace? (Does this come through
get_cycles() or via the It seems like we could either initialize the
timer earlier or allow it to fall back when not initialized...
-Kees
>
> Guenter
>
> ---
> # bad: [4dfc2788033d30dfccfd4268e06dd73ce2c654ed] Merge tag 'iommu-updates-v4.14' of git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu
> # good: [5969d1bb3082b41eba8fd2c826559abe38ccb6df] Merge branch 'gperf-removal'
> git bisect start 'HEAD' '5969d1bb3082'
> # bad: [fbd01410e89a66f346ba1b3c0161e1198449b746] Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
> git bisect bad fbd01410e89a66f346ba1b3c0161e1198449b746
> # good: [0d519f2d1ed1f11e49abc88cfcf6cf13b83ba14c] Merge tag 'pci-v4.14-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci
> git bisect good 0d519f2d1ed1f11e49abc88cfcf6cf13b83ba14c
> # bad: [229cf16d3c8ac2e9b082c223fd0e619dc8f62cc1] sh: defconfig: cleanup from old Kconfig options
> git bisect bad 229cf16d3c8ac2e9b082c223fd0e619dc8f62cc1
> # good: [1caffba9db4aa27c3e7ebc05668afca1f991ab8d] drivers/scsi/sym53c8xx_2/sym_hipd.c: convert to use memset32
> git bisect good 1caffba9db4aa27c3e7ebc05668afca1f991ab8d
> # good: [7c61bd6983b185272315722787cceacf5f5d2e7d] lib/cmdline.c: remove meaningless comment
> git bisect good 7c61bd6983b185272315722787cceacf5f5d2e7d
> # bad: [718b303b49893d8f9dd469f710d76f77673bd35d] autofs: use AUTOFS_DEV_IOCTL_SIZE
> git bisect bad 718b303b49893d8f9dd469f710d76f77673bd35d
> # good: [9367bb730e4d9d85a8911a08a3542ec2aa873d37] binfmt_flat: delete two error messages for a failed memory allocation in decompress_exec()
> git bisect good 9367bb730e4d9d85a8911a08a3542ec2aa873d37
> # bad: [e54c7bcbf14a25dc3a913b4c808b52121c522e9b] autofs: make disc device user accessible
> git bisect bad e54c7bcbf14a25dc3a913b4c808b52121c522e9b
> # bad: [33d72f3822d7ff8a9e45bd7413c811085cb87aa5] init/main.c: extract early boot entropy from the passed cmdline
> git bisect bad 33d72f3822d7ff8a9e45bd7413c811085cb87aa5
> # good: [121388a31362b0d3176dc1190ac8064b98a61b20] init: move stack canary initialization after setup_arch
> git bisect good 121388a31362b0d3176dc1190ac8064b98a61b20
> # first bad commit: [33d72f3822d7ff8a9e45bd7413c811085cb87aa5] init/main.c: extract early boot entropy from the passed cmdline
--
Kees Cook
Pixel Security
On Mon, Sep 11, 2017 at 09:36:00AM -0700, Kees Cook wrote:
> On Sat, Sep 9, 2017 at 8:58 PM, Guenter Roeck <[email protected]> wrote:
> > Hi,
> >
> > I noticed that nios2 images crash in mainline. Bisect points to commit
> > 33d72f3822d7 ("init/main.c: extract early boot entropy from the passed
> > cmdline"). Bisect log is attached.
> >
> > As far as I can see, the problem is seen because add_device_randomness()
> > calls random_get_entropy(). However, the underlying timer function
> > used by the nios2 architecture (nios2_timer_read) is not yet initialized,
> > causing a NULL pointer access and crash. A sample crash log is at
> > http://kerneltests.org/builders/qemu-nios2-master/builds/175/steps/qemubuildcommand/logs/stdio
>
> Oh, yikes. Do you have a full call trace? (Does this come through
> get_cycles() or via the It seems like we could either initialize the
> timer earlier or allow it to fall back when not initialized...
>
nios2 doesn't give me a traceback. I followed it by adding debug messages.
The code path is through get_cycles().
On nios2:
static u64 nios2_timer_read(struct clocksource *cs)
{
struct nios2_clocksource *nios2_cs = to_nios2_clksource(cs);
unsigned long flags;
u32 count;
local_irq_save(flags);
count = read_timersnapshot(&nios2_cs->timer); // <- not initialized
local_irq_restore(flags);
/* Counter is counting down */
return ~count;
}
cycles_t get_cycles(void)
{
return nios2_timer_read(&nios2_cs.cs);
}
EXPORT_SYMBOL(get_cycles);
Guenter
> -Kees
>
> >
> > Guenter
> >
> > ---
> > # bad: [4dfc2788033d30dfccfd4268e06dd73ce2c654ed] Merge tag 'iommu-updates-v4.14' of git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu
> > # good: [5969d1bb3082b41eba8fd2c826559abe38ccb6df] Merge branch 'gperf-removal'
> > git bisect start 'HEAD' '5969d1bb3082'
> > # bad: [fbd01410e89a66f346ba1b3c0161e1198449b746] Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
> > git bisect bad fbd01410e89a66f346ba1b3c0161e1198449b746
> > # good: [0d519f2d1ed1f11e49abc88cfcf6cf13b83ba14c] Merge tag 'pci-v4.14-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci
> > git bisect good 0d519f2d1ed1f11e49abc88cfcf6cf13b83ba14c
> > # bad: [229cf16d3c8ac2e9b082c223fd0e619dc8f62cc1] sh: defconfig: cleanup from old Kconfig options
> > git bisect bad 229cf16d3c8ac2e9b082c223fd0e619dc8f62cc1
> > # good: [1caffba9db4aa27c3e7ebc05668afca1f991ab8d] drivers/scsi/sym53c8xx_2/sym_hipd.c: convert to use memset32
> > git bisect good 1caffba9db4aa27c3e7ebc05668afca1f991ab8d
> > # good: [7c61bd6983b185272315722787cceacf5f5d2e7d] lib/cmdline.c: remove meaningless comment
> > git bisect good 7c61bd6983b185272315722787cceacf5f5d2e7d
> > # bad: [718b303b49893d8f9dd469f710d76f77673bd35d] autofs: use AUTOFS_DEV_IOCTL_SIZE
> > git bisect bad 718b303b49893d8f9dd469f710d76f77673bd35d
> > # good: [9367bb730e4d9d85a8911a08a3542ec2aa873d37] binfmt_flat: delete two error messages for a failed memory allocation in decompress_exec()
> > git bisect good 9367bb730e4d9d85a8911a08a3542ec2aa873d37
> > # bad: [e54c7bcbf14a25dc3a913b4c808b52121c522e9b] autofs: make disc device user accessible
> > git bisect bad e54c7bcbf14a25dc3a913b4c808b52121c522e9b
> > # bad: [33d72f3822d7ff8a9e45bd7413c811085cb87aa5] init/main.c: extract early boot entropy from the passed cmdline
> > git bisect bad 33d72f3822d7ff8a9e45bd7413c811085cb87aa5
> > # good: [121388a31362b0d3176dc1190ac8064b98a61b20] init: move stack canary initialization after setup_arch
> > git bisect good 121388a31362b0d3176dc1190ac8064b98a61b20
> > # first bad commit: [33d72f3822d7ff8a9e45bd7413c811085cb87aa5] init/main.c: extract early boot entropy from the passed cmdline
>
>
>
> --
> Kees Cook
> Pixel Security
On Mon, 2017-09-11 at 10:35 -0700, Guenter Roeck wrote:
> On Mon, Sep 11, 2017 at 09:36:00AM -0700, Kees Cook wrote:
> > On Sat, Sep 9, 2017 at 8:58 PM, Guenter Roeck <[email protected]>
> > wrote:
> > > Hi,
> > >
> > > I noticed that nios2 images crash in mainline. Bisect points to
> > > commit
> > > 33d72f3822d7 ("init/main.c: extract early boot entropy from the
> > > passed
> > > cmdline"). Bisect log is attached.
> > >
> > > As far as I can see, the problem is seen because
> > > add_device_randomness()
> > > calls random_get_entropy(). However, the underlying timer function
> > > used by the nios2 architecture (nios2_timer_read) is not yet
> > > initialized,
> > > causing a NULL pointer access and crash. A sample crash log is at
> > > http://kerneltests.org/builders/qemu-nios2-master/builds/1
> > > 75/steps/qemubuildcommand/logs/stdio
> >
> > Oh, yikes. Do you have a full call trace? (Does this come through
> > get_cycles() or via the It seems like we could either initialize the
> > timer earlier or allow it to fall back when not initialized...
> >
>
> nios2 doesn't give me a traceback. I followed it by adding debug
> messages.
> The code path is through get_cycles().
>
> On nios2:
>
> static u64 nios2_timer_read(struct clocksource *cs)
> {
> struct nios2_clocksource *nios2_cs = to_nios2_clksource(cs);
> unsigned long flags;
> u32 count;
>
> local_irq_save(flags);
> count = read_timersnapshot(&nios2_cs->timer); // <- not
> initialized
> local_irq_restore(flags);
>
> /* Counter is counting down */
> return ~count;
> }
>
> cycles_t get_cycles(void)
> {
> return nios2_timer_read(&nios2_cs.cs);
> }
> EXPORT_SYMBOL(get_cycles);
>
> Guenter
Maybe it should WARN and return 0 for now if that's NULL?
On Mon, Sep 11, 2017 at 11:25 AM, Daniel Micay <[email protected]> wrote:
> On Mon, 2017-09-11 at 10:35 -0700, Guenter Roeck wrote:
>> On Mon, Sep 11, 2017 at 09:36:00AM -0700, Kees Cook wrote:
>> > On Sat, Sep 9, 2017 at 8:58 PM, Guenter Roeck <[email protected]>
>> > wrote:
>> > > Hi,
>> > >
>> > > I noticed that nios2 images crash in mainline. Bisect points to
>> > > commit
>> > > 33d72f3822d7 ("init/main.c: extract early boot entropy from the
>> > > passed
>> > > cmdline"). Bisect log is attached.
>> > >
>> > > As far as I can see, the problem is seen because
>> > > add_device_randomness()
>> > > calls random_get_entropy(). However, the underlying timer function
>> > > used by the nios2 architecture (nios2_timer_read) is not yet
>> > > initialized,
>> > > causing a NULL pointer access and crash. A sample crash log is at
>> > > http://kerneltests.org/builders/qemu-nios2-master/builds/1
>> > > 75/steps/qemubuildcommand/logs/stdio
>> >
>> > Oh, yikes. Do you have a full call trace? (Does this come through
>> > get_cycles() or via the It seems like we could either initialize the
>> > timer earlier or allow it to fall back when not initialized...
>> >
>>
>> nios2 doesn't give me a traceback. I followed it by adding debug
>> messages.
>> The code path is through get_cycles().
>>
>> On nios2:
>>
>> static u64 nios2_timer_read(struct clocksource *cs)
>> {
>> struct nios2_clocksource *nios2_cs = to_nios2_clksource(cs);
>> unsigned long flags;
>> u32 count;
>>
>> local_irq_save(flags);
>> count = read_timersnapshot(&nios2_cs->timer); // <- not
>> initialized
>> local_irq_restore(flags);
>>
>> /* Counter is counting down */
>> return ~count;
>> }
>>
>> cycles_t get_cycles(void)
>> {
>> return nios2_timer_read(&nios2_cs.cs);
>> }
>> EXPORT_SYMBOL(get_cycles);
>>
>> Guenter
>
> Maybe it should WARN and return 0 for now if that's NULL?
In this case, we'd always WARN. :P But yeah, 0 return on NULL timer
seems okay to me here. I am curious if it's possible to start the
timer earlier, though. It's not clear to me where nios2_cs->timer gets
set.
-Kees
--
Kees Cook
Pixel Security
On 09/11/2017 11:41 AM, Kees Cook wrote:
> On Mon, Sep 11, 2017 at 11:25 AM, Daniel Micay <[email protected]> wrote:
>> On Mon, 2017-09-11 at 10:35 -0700, Guenter Roeck wrote:
>>> On Mon, Sep 11, 2017 at 09:36:00AM -0700, Kees Cook wrote:
>>>> On Sat, Sep 9, 2017 at 8:58 PM, Guenter Roeck <[email protected]>
>>>> wrote:
>>>>> Hi,
>>>>>
>>>>> I noticed that nios2 images crash in mainline. Bisect points to
>>>>> commit
>>>>> 33d72f3822d7 ("init/main.c: extract early boot entropy from the
>>>>> passed
>>>>> cmdline"). Bisect log is attached.
>>>>>
>>>>> As far as I can see, the problem is seen because
>>>>> add_device_randomness()
>>>>> calls random_get_entropy(). However, the underlying timer function
>>>>> used by the nios2 architecture (nios2_timer_read) is not yet
>>>>> initialized,
>>>>> causing a NULL pointer access and crash. A sample crash log is at
>>>>> http://kerneltests.org/builders/qemu-nios2-master/builds/1
>>>>> 75/steps/qemubuildcommand/logs/stdio
>>>>
>>>> Oh, yikes. Do you have a full call trace? (Does this come through
>>>> get_cycles() or via the It seems like we could either initialize the
>>>> timer earlier or allow it to fall back when not initialized...
>>>>
>>>
>>> nios2 doesn't give me a traceback. I followed it by adding debug
>>> messages.
>>> The code path is through get_cycles().
>>>
>>> On nios2:
>>>
>>> static u64 nios2_timer_read(struct clocksource *cs)
>>> {
>>> struct nios2_clocksource *nios2_cs = to_nios2_clksource(cs);
>>> unsigned long flags;
>>> u32 count;
>>>
>>> local_irq_save(flags);
>>> count = read_timersnapshot(&nios2_cs->timer); // <- not
>>> initialized
>>> local_irq_restore(flags);
>>>
>>> /* Counter is counting down */
>>> return ~count;
>>> }
>>>
>>> cycles_t get_cycles(void)
>>> {
>>> return nios2_timer_read(&nios2_cs.cs);
>>> }
>>> EXPORT_SYMBOL(get_cycles);
>>>
>>> Guenter
>>
>> Maybe it should WARN and return 0 for now if that's NULL?
>
> In this case, we'd always WARN. :P But yeah, 0 return on NULL timer
> seems okay to me here. I am curious if it's possible to start the
> timer earlier, though. It's not clear to me where nios2_cs->timer gets
> set.
>
> -Kees
>
At the bottom of the file is
TIMER_OF_DECLARE(nios2_timer, ALTR_TIMER_COMPATIBLE, nios2_time_init);
so I don't think initialization can happen any earlier if this is
tied to devicetree.
Thanks,
Laura