[BUG] OpenRISC exec init fails, bisected to 0886551 ("initramfs: finish fput() before accessing any binary from initramfs")

[BUG] OpenRISC exec init fails, bisected to 0886551 ("initramfs: finish fput() before accessing any binary from initramfs")

[Stafford Horne]

Hello,

While booting the v4.11 kernel I found the below issue.

The summary of the issue mentions

    Commit 4a9d4b024a31 ("switch fput to task_work_add") implements a
    schedule_work() for completing fput(), but did not guarantee calling
    __fput() after unpacking initramfs.  Because of this, there is a
    possibility that during boot a driver can see ETXTBSY when it tries to
    load a binary from initramfs as fput() is still pending on that binary.

It seems this patch (0886551) introduces that issue though?

I am looking into it, but any suggestions would be helpful.

BOOT LOG

  Compiled-in FDT at c03b9100
  Linux version 4.10.0-10351-g0886551 (shorne@lianli.shorne-pla.net) (gcc version 5.4.0 (GCC) ) #223 Thu May 4 16:01:02 JST 2017
  CPU: OpenRISC-0 (revision 0) @20 MHz
  ...
  Freeing unused kernel memory: 2856K
  This architecture does not have kernel memory protection.
  Failed to execute /init (error -26)
  Starting init: /sbin/init exists but couldn't execute it (error -26)
  Starting init: /bin/sh exists but couldn't execute it (error -26)
  Kernel panic - not syncing: No working init found.  Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.
  ---[ end Kernel panic - not syncing: No working init found.  Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.


TO REPRODUCE

The failure is intermittent.

Using some tools:
  - initramfs: https://github.com/stffrdhrn/or1k-utils
  - compiler: https://github.com/openrisc/or1k-gcc/releases (or1k-linux-5.4.0-20170218.tar.xz)
  - qemu: version 2.9

Make and run using:
  make ARCH=openrisc defconfig
  make -j5 ARCH=openrisc CROSS_COMPILE=or1k-linux- \
  CONFIG_INITRAMFS_SOURCE="../openrisc/or1k-utils/initramfs ../openrisc/or1k-utils/initramfs.devnodes"

  qemu-system-or1k -cpu or1200 -M or1k-sim -kernel vmlinux -serial stdio \
  -nographic -monitor none

-Stafford

Re: [BUG] OpenRISC exec init fails, bisected to 0886551 ("initramfs: finish fput() before accessing any binary from initramfs")

[Lokesh Vutla]

On Thursday 04 May 2017 12:41 PM, Stafford Horne wrote:
> Hello,
> 
> While booting the v4.11 kernel I found the below issue.
> 
> The summary of the issue mentions
> 
>     Commit 4a9d4b024a31 ("switch fput to task_work_add") implements a
>     schedule_work() for completing fput(), but did not guarantee calling
>     __fput() after unpacking initramfs.  Because of this, there is a
>     possibility that during boot a driver can see ETXTBSY when it tries to
>     load a binary from initramfs as fput() is still pending on that binary.
> 
> It seems this patch (0886551) introduces that issue though?
> 
> I am looking into it, but any suggestions would be helpful.

Can you check if flush_delayed_fput() is being called? Do you have
CONFIG_INITRAMFS_FORCE enabled?

Can I see complete boot log?

Thanks and regards,
Lokesh

> 
> BOOT LOG
> 
>   Compiled-in FDT at c03b9100
>   Linux version 4.10.0-10351-g0886551 (shorne@lianli.shorne-pla.net) (gcc version 5.4.0 (GCC) ) #223 Thu May 4 16:01:02 JST 2017
>   CPU: OpenRISC-0 (revision 0) @20 MHz
>   ...
>   Freeing unused kernel memory: 2856K
>   This architecture does not have kernel memory protection.
>   Failed to execute /init (error -26)
>   Starting init: /sbin/init exists but couldn't execute it (error -26)
>   Starting init: /bin/sh exists but couldn't execute it (error -26)
>   Kernel panic - not syncing: No working init found.  Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.
>   ---[ end Kernel panic - not syncing: No working init found.  Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.
> 
> 
> TO REPRODUCE
> 
> The failure is intermittent.
> 
> Using some tools:
>   - initramfs: https://github.com/stffrdhrn/or1k-utils
>   - compiler: https://github.com/openrisc/or1k-gcc/releases (or1k-linux-5.4.0-20170218.tar.xz)
>   - qemu: version 2.9
> 
> Make and run using:
>   make ARCH=openrisc defconfig
>   make -j5 ARCH=openrisc CROSS_COMPILE=or1k-linux- \
>   CONFIG_INITRAMFS_SOURCE="../openrisc/or1k-utils/initramfs ../openrisc/or1k-utils/initramfs.devnodes"
> 
>   qemu-system-or1k -cpu or1200 -M or1k-sim -kernel vmlinux -serial stdio \
>   -nographic -monitor none
> 
> -Stafford
> 

Re: [BUG] OpenRISC exec init fails, bisected to 0886551 ("initramfs: finish fput() before accessing any binary from initramfs")

[Stafford Horne]

On Thu, May 04, 2017 at 01:15:23PM +0530, Lokesh Vutla wrote:
> 
> 
> On Thursday 04 May 2017 12:41 PM, Stafford Horne wrote:
> > Hello,
> > 
> > While booting the v4.11 kernel I found the below issue.
> > 
> > The summary of the issue mentions
> > 
> >     Commit 4a9d4b024a31 ("switch fput to task_work_add") implements a
> >     schedule_work() for completing fput(), but did not guarantee calling
> >     __fput() after unpacking initramfs.  Because of this, there is a
> >     possibility that during boot a driver can see ETXTBSY when it tries to
> >     load a binary from initramfs as fput() is still pending on that binary.
> > 
> > It seems this patch (0886551) introduces that issue though?
> > 
> > I am looking into it, but any suggestions would be helpful.
> 
> Can you check if flush_delayed_fput() is being called? Do you have
> CONFIG_INITRAMFS_FORCE enabled?

This is not enabled.  I debugged it and I cant see it getting called.  I
see populate_rootfs getting called but initrd_start is 0, I think there is
something different happening to unpack in initramfs since ours is compiled
in.

I am trying to look into it, but I need to relearn how the initramfs gets
initted for OpenRISC.  Perhaps the fix it going to keep the
flush_delayed_fput() call in init/main.c.

> Can I see complete boot log?

I expect to see a line "Unpacking initramfs..." but I don't:

See below:

Compiled-in FDT at c0351200
Linux version 4.10.0-10351-g0886551 (shorne@lianli.shorne-pla.net) (gcc version 5.4.0 (GCC) ) #225 Thu May 4 17:21:27 JST 2017
CPU: OpenRISC-0 (revision 0) @20 MHz
-- dcache disabled
-- icache disabled
-- dmmu:   64 entries, 1 way(s)
-- immu:   64 entries, 1 way(s)
-- additional features:
-- power management
-- PIC
-- timer
setup_memory: Memory: 0x0-0x2000000
Setting up paging and PTEs.
map_ram: Memory: 0x0-0x2000000
itlb_miss_handler c0002160
dtlb_miss_handler c0002000
OpenRISC Linux -- http://openrisc.io
Built 1 zonelists in Zone order, mobility grouping off.  Total pages: 4080
Kernel command line: console=uart,mmio,0x90000000,115200
earlycon: uart0 at MMIO 0x90000000 (options '115200')
bootconsole [uart0] enabled
PID hash table entries: 128 (order: -4, 512 bytes)
Dentry cache hash table entries: 4096 (order: 1, 16384 bytes)
Inode-cache hash table entries: 2048 (order: 0, 8192 bytes)
Sorting __ex_table...
Memory: 26312K/32768K available (2846K kernel code, 112K rwdata, 312K rodata, 2856K init, 94K bss, 6456K reserved, 0K cma-reserved)
mem_init_done ...........................................
NR_IRQS:32 nr_irqs:32 0
clocksource: openrisc_timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 95563022313 ns
40.00 BogoMIPS (lpj=200000)
pid_max: default: 32768 minimum: 301
Mount-cache hash table entries: 2048 (order: 0, 8192 bytes)
Mountpoint-cache hash table entries: 2048 (order: 0, 8192 bytes)
devtmpfs: initialized
clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
futex hash table entries: 256 (order: -2, 3072 bytes)
NET: Registered protocol family 16
clocksource: Switched to clocksource openrisc_timer
NET: Registered protocol family 2
TCP established hash table entries: 2048 (order: 0, 8192 bytes)
TCP bind hash table entries: 2048 (order: 0, 8192 bytes)
TCP: Hash tables configured (established 2048 bind 2048)
UDP hash table entries: 512 (order: 0, 8192 bytes)
UDP-Lite hash table entries: 512 (order: 0, 8192 bytes)
NET: Registered protocol family 1
RPC: Registered named UNIX socket transport module.
RPC: Registered udp transport module.
RPC: Registered tcp transport module.
RPC: Registered tcp NFSv4.1 backchannel transport module.
workingset: timestamp_bits=30 max_order=12 bucket_order=0
Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
90000000.serial: ttyS0 at MMIO 0x90000000 (irq = 2, base_baud = 1250000) is a 16550A
console [ttyS0] enabled
console [ttyS0] enabled
bootconsole [uart0] disabled
bootconsole [uart0] disabled
libphy: Fixed MDIO Bus: probed
NET: Registered protocol family 17
Freeing unused kernel memory: 2856K
This architecture does not have kernel memory protection.
Failed to execute /init (error -26)
Starting init: /sbin/init exists but couldn't execute it (error -26)
Starting init: /bin/sh exists but couldn't execute it (error -26)
Kernel panic - not syncing: No working init found.  Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.
---[ end Kernel panic - not syncing: No working init found.  Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.

Re: [BUG] OpenRISC exec init fails, bisected to 0886551 ("initramfs: finish fput() before accessing any binary from initramfs")

[Lokesh Vutla]

On Thursday 04 May 2017 02:05 PM, Stafford Horne wrote:
> On Thu, May 04, 2017 at 01:15:23PM +0530, Lokesh Vutla wrote:
>>
>>
>> On Thursday 04 May 2017 12:41 PM, Stafford Horne wrote:
>>> Hello,
>>>
>>> While booting the v4.11 kernel I found the below issue.
>>>
>>> The summary of the issue mentions
>>>
>>>     Commit 4a9d4b024a31 ("switch fput to task_work_add") implements a
>>>     schedule_work() for completing fput(), but did not guarantee calling
>>>     __fput() after unpacking initramfs.  Because of this, there is a
>>>     possibility that during boot a driver can see ETXTBSY when it tries to
>>>     load a binary from initramfs as fput() is still pending on that binary.
>>>
>>> It seems this patch (0886551) introduces that issue though?
>>>
>>> I am looking into it, but any suggestions would be helpful.
>>
>> Can you check if flush_delayed_fput() is being called? Do you have
>> CONFIG_INITRAMFS_FORCE enabled?
> 
> This is not enabled.  I debugged it and I cant see it getting called.  I
> see populate_rootfs getting called but initrd_start is 0, I think there is
> something different happening to unpack in initramfs since ours is compiled
> in.

What about __initramfs_start, __initramfs_size?

> 
> I am trying to look into it, but I need to relearn how the initramfs gets
> initted for OpenRISC.  Perhaps the fix it going to keep the
> flush_delayed_fput() call in init/main.c.

My initial version of the patch did not remove the call to
flush_delayed_fput() from init/main.c but Al Viro asked to drop it as it
is called in populate_rootfs().

May be Al Viro can give more data on what is happening here.

Thanks and regards,
Lokesh


> 
>> Can I see complete boot log?
> 
> I expect to see a line "Unpacking initramfs..." but I don't:
> 
> See below:
> 
> Compiled-in FDT at c0351200
> Linux version 4.10.0-10351-g0886551 (shorne@lianli.shorne-pla.net) (gcc version 5.4.0 (GCC) ) #225 Thu May 4 17:21:27 JST 2017
> CPU: OpenRISC-0 (revision 0) @20 MHz
> -- dcache disabled
> -- icache disabled
> -- dmmu:   64 entries, 1 way(s)
> -- immu:   64 entries, 1 way(s)
> -- additional features:
> -- power management
> -- PIC
> -- timer
> setup_memory: Memory: 0x0-0x2000000
> Setting up paging and PTEs.
> map_ram: Memory: 0x0-0x2000000
> itlb_miss_handler c0002160
> dtlb_miss_handler c0002000
> OpenRISC Linux -- http://openrisc.io
> Built 1 zonelists in Zone order, mobility grouping off.  Total pages: 4080
> Kernel command line: console=uart,mmio,0x90000000,115200
> earlycon: uart0 at MMIO 0x90000000 (options '115200')
> bootconsole [uart0] enabled
> PID hash table entries: 128 (order: -4, 512 bytes)
> Dentry cache hash table entries: 4096 (order: 1, 16384 bytes)
> Inode-cache hash table entries: 2048 (order: 0, 8192 bytes)
> Sorting __ex_table...
> Memory: 26312K/32768K available (2846K kernel code, 112K rwdata, 312K rodata, 2856K init, 94K bss, 6456K reserved, 0K cma-reserved)
> mem_init_done ...........................................
> NR_IRQS:32 nr_irqs:32 0
> clocksource: openrisc_timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 95563022313 ns
> 40.00 BogoMIPS (lpj=200000)
> pid_max: default: 32768 minimum: 301
> Mount-cache hash table entries: 2048 (order: 0, 8192 bytes)
> Mountpoint-cache hash table entries: 2048 (order: 0, 8192 bytes)
> devtmpfs: initialized
> clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
> futex hash table entries: 256 (order: -2, 3072 bytes)
> NET: Registered protocol family 16
> clocksource: Switched to clocksource openrisc_timer
> NET: Registered protocol family 2
> TCP established hash table entries: 2048 (order: 0, 8192 bytes)
> TCP bind hash table entries: 2048 (order: 0, 8192 bytes)
> TCP: Hash tables configured (established 2048 bind 2048)
> UDP hash table entries: 512 (order: 0, 8192 bytes)
> UDP-Lite hash table entries: 512 (order: 0, 8192 bytes)
> NET: Registered protocol family 1
> RPC: Registered named UNIX socket transport module.
> RPC: Registered udp transport module.
> RPC: Registered tcp transport module.
> RPC: Registered tcp NFSv4.1 backchannel transport module.
> workingset: timestamp_bits=30 max_order=12 bucket_order=0
> Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> 90000000.serial: ttyS0 at MMIO 0x90000000 (irq = 2, base_baud = 1250000) is a 16550A
> console [ttyS0] enabled
> console [ttyS0] enabled
> bootconsole [uart0] disabled
> bootconsole [uart0] disabled
> libphy: Fixed MDIO Bus: probed
> NET: Registered protocol family 17
> Freeing unused kernel memory: 2856K
> This architecture does not have kernel memory protection.
> Failed to execute /init (error -26)
> Starting init: /sbin/init exists but couldn't execute it (error -26)
> Starting init: /bin/sh exists but couldn't execute it (error -26)
> Kernel panic - not syncing: No working init found.  Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.
> ---[ end Kernel panic - not syncing: No working init found.  Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.
> 

Re: [BUG] OpenRISC exec init fails, bisected to 0886551 ("initramfs: finish fput() before accessing any binary from initramfs")

[Stafford Horne]

On Thu, May 04, 2017 at 02:39:38PM +0530, Lokesh Vutla wrote:
> 
> 
> On Thursday 04 May 2017 02:05 PM, Stafford Horne wrote:
> > On Thu, May 04, 2017 at 01:15:23PM +0530, Lokesh Vutla wrote:
> >>
> >>
> >> On Thursday 04 May 2017 12:41 PM, Stafford Horne wrote:
> >>> Hello,
> >>>
> >>> While booting the v4.11 kernel I found the below issue.
> >>>
> >>> The summary of the issue mentions
> >>>
> >>>     Commit 4a9d4b024a31 ("switch fput to task_work_add") implements a
> >>>     schedule_work() for completing fput(), but did not guarantee calling
> >>>     __fput() after unpacking initramfs.  Because of this, there is a
> >>>     possibility that during boot a driver can see ETXTBSY when it tries to
> >>>     load a binary from initramfs as fput() is still pending on that binary.
> >>>
> >>> It seems this patch (0886551) introduces that issue though?
> >>>
> >>> I am looking into it, but any suggestions would be helpful.
> >>
> >> Can you check if flush_delayed_fput() is being called? Do you have
> >> CONFIG_INITRAMFS_FORCE enabled?
> > 
> > This is not enabled.  I debugged it and I cant see it getting called.  I
> > see populate_rootfs getting called but initrd_start is 0, I think there is
> > something different happening to unpack in initramfs since ours is compiled
> > in.
> 
> What about __initramfs_start, __initramfs_size?

Those have the correct values.

I think the problem is with openrisc's linker file.

In there we have:

	__initrd_start = .;
	*(.initrd)
	__initrd_end = .;

In setup_arch() we have:

    #ifdef CONFIG_BLK_DEV_INITRD
        initrd_start = (unsigned long)&__initrd_start;
        initrd_end = (unsigned long)&__initrd_end;
        if (initrd_start == initrd_end) {
                initrd_start = 0;
                initrd_end = 0;
        }
        initrd_below_start_ok = 1;
    #endif

That doesnt work because there is nothing in .initrd and __initrd_start is
the same as __initrd_end.

The initramfs gets linked to the location of  __initramfs_start, not
__initrd_start.  I am guessing the above is something very old.

I think I just need to remove all of that __initrd_start/__initrd_end
stuff.

I'll try it out and send a patch later today or tomorrow.  Not much time
now.

> > 
> > I am trying to look into it, but I need to relearn how the initramfs gets
> > initted for OpenRISC.  Perhaps the fix it going to keep the
> > flush_delayed_fput() call in init/main.c.
> 
> My initial version of the patch did not remove the call to
> flush_delayed_fput() from init/main.c but Al Viro asked to drop it as it
> is called in populate_rootfs().
> 
> May be Al Viro can give more data on what is happening here.

Thanks,  any comments or history lessons would be good here.

-Stafford

[PATCH] initramfs: Always do fput() and load modules after rootfs populate

[Stafford Horne]

In OpenRISC we do not have a bootloader passed initrd, but the built in
initramfs does contain the /init and other binaries, including modules.
The previous commit 08865514805d2 ("initramfs: finish fput() before
accessing any binary from initramfs") made a change to only call fput()
if the bootloader initrd was available, this caused intermittent crashes
for OpenRISC.

This patch changes the fput() to happen unconditionally if any rootfs is
loaded. Also, I added some comments to make it a bit more clear why we
call unpack_to_rootfs() multiple times.

Fixes: 08865514805d2 ("initramfs: finish fput() before accessing any binary from initramfs")
Cc: Lokesh Vutla <lokeshvutla@ti.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 init/initramfs.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 981f286..3a68715 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -608,9 +608,11 @@ static void __init clean_rootfs(void)
 
 static int __init populate_rootfs(void)
 {
+	/* Load the built in initramfs */
 	char *err = unpack_to_rootfs(__initramfs_start, __initramfs_size);
 	if (err)
 		panic("%s", err); /* Failed to decompress INTERNAL initramfs */
+	/* If available load the bootloader supplied initrd */
 	if (initrd_start) {
 #ifdef CONFIG_BLK_DEV_RAM
 		int fd;
@@ -648,13 +650,14 @@ static int __init populate_rootfs(void)
 			printk(KERN_EMERG "Initramfs unpacking failed: %s\n", err);
 		free_initrd();
 #endif
-		flush_delayed_fput();
-		/*
-		 * Try loading default modules from initramfs.  This gives
-		 * us a chance to load before device_initcalls.
-		 */
-		load_default_modules();
 	}
+	flush_delayed_fput();
+	/*
+	 * Try loading default modules from initramfs.  This gives
+	 * us a chance to load before device_initcalls.
+	 */
+	load_default_modules();
+
 	return 0;
 }
 rootfs_initcall(populate_rootfs);
-- 
2.9.3

Re: [PATCH] initramfs: Always do fput() and load modules after rootfs populate

[Al Viro]

On Thu, May 04, 2017 at 09:47:00PM +0900, Stafford Horne wrote:
> In OpenRISC we do not have a bootloader passed initrd, but the built in
> initramfs does contain the /init and other binaries, including modules.
> The previous commit 08865514805d2 ("initramfs: finish fput() before
> accessing any binary from initramfs") made a change to only call fput()
> if the bootloader initrd was available, this caused intermittent crashes
> for OpenRISC.
> 
> This patch changes the fput() to happen unconditionally if any rootfs is
> loaded. Also, I added some comments to make it a bit more clear why we
> call unpack_to_rootfs() multiple times.

Hmm...  Looks like there are two bugs: the older one (from "init, block: try
to load default elevator module early during boot") that missed the case of
said module being on built-in initramfs and then flush_delayed_fput() patch
apparently using that as a marker...

Frankly, the life would be a lot more convenient if we had the whole
populate_rootfs() run in userland.  It's using the normal syscalls anyway;
the only place where it needs more is the access to initramfs images and
that could be done e.g. by offering one or two file descriptors with
splice() from those picking those images.  We could build a static binary,
link _that_ into the kernel and turn the entire thing into "fork a thread,
write that sucker to rootfs, synchronously close it and do kernel_execve",
letting everything else (decompression, unpacking, etc.) done in there.

That was the original plan, actually, and that was the original motivation
for klibc.  Unfortunately, klibc would have to live in the kernel tree for
that to work, and that hadn't happened.  Pity, that...

Anyway, feel free to slap
Acked-by: Al Viro <viro@zeniv.linux.org.uk>
on that patch.  I can take it through vfs.git, or either of you could send
it directly to Linus - up to you.

Re: [PATCH] initramfs: Always do fput() and load modules after rootfs populate

[Stafford Horne]

On Thu, May 04, 2017 at 07:41:48PM +0100, Al Viro wrote:
> On Thu, May 04, 2017 at 09:47:00PM +0900, Stafford Horne wrote:
> > In OpenRISC we do not have a bootloader passed initrd, but the built in
> > initramfs does contain the /init and other binaries, including modules.
> > The previous commit 08865514805d2 ("initramfs: finish fput() before
> > accessing any binary from initramfs") made a change to only call fput()
> > if the bootloader initrd was available, this caused intermittent crashes
> > for OpenRISC.
> > 
> > This patch changes the fput() to happen unconditionally if any rootfs is
> > loaded. Also, I added some comments to make it a bit more clear why we
> > call unpack_to_rootfs() multiple times.
> 
> Hmm...  Looks like there are two bugs: the older one (from "init, block: try
> to load default elevator module early during boot") that missed the case of
> said module being on built-in initramfs and then flush_delayed_fput() patch
> apparently using that as a marker...
> 
> Frankly, the life would be a lot more convenient if we had the whole
> populate_rootfs() run in userland.  It's using the normal syscalls anyway;
> the only place where it needs more is the access to initramfs images and
> that could be done e.g. by offering one or two file descriptors with
> splice() from those picking those images.  We could build a static binary,
> link _that_ into the kernel and turn the entire thing into "fork a thread,
> write that sucker to rootfs, synchronously close it and do kernel_execve",
> letting everything else (decompression, unpacking, etc.) done in there.
> 
> That was the original plan, actually, and that was the original motivation
> for klibc.  Unfortunately, klibc would have to live in the kernel tree for
> that to work, and that hadn't happened.  Pity, that...
> 
> Anyway, feel free to slap
> Acked-by: Al Viro <viro@zeniv.linux.org.uk>
> on that patch.  I can take it through vfs.git, or either of you could send
> it directly to Linus - up to you.

Hi Al,

Thanks for the review and the background on this.  I could see this would
make it nicer/cleaner to have populate_rootfs() in userland.  But, I guess
if that means adding klibc and keeping this static binary all in tree the
benefits don't outweight the downside of having the extra code and changes.
Do you think it is work revisiting someday?

I have pushed to linus, lets see what he says, if anything.

-Stafford

Re: initramfs: Always do fput() and load modules after rootfs populate

[Andrei Vagin]

Hi,

I see the following error with this patch:

init/initramfs.c: In function 'populate_rootfs':
init/initramfs.c:644:2: error: label at end of compound statement
  done:
  ^
scripts/Makefile.build:294: recipe for target 'init/initramfs.o' failed
make[1]: *** [init/initramfs.o] Error 1

probably we need something like this:
diff --git a/init/initramfs.c b/init/initramfs.c
index 3a68715..9966c0f 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -641,7 +641,6 @@ static int __init populate_rootfs(void)
                        sys_close(fd);
                        free_initrd();
                }
-       done:
 #else
                printk(KERN_INFO "Unpacking initramfs...\n");
                err = unpack_to_rootfs((char *)initrd_start,
@@ -651,6 +650,7 @@ static int __init populate_rootfs(void)
                free_initrd();
 #endif
        }
+done:
        flush_delayed_fput();
        /*
         * Try loading default modules from initramfs.  This gives

Thanks,
Andrei

On Thu, May 04, 2017 at 09:47:00PM +0900, Stafford Horne wrote:
> In OpenRISC we do not have a bootloader passed initrd, but the built in
> initramfs does contain the /init and other binaries, including modules.
> The previous commit 08865514805d2 ("initramfs: finish fput() before
> accessing any binary from initramfs") made a change to only call fput()
> if the bootloader initrd was available, this caused intermittent crashes
> for OpenRISC.
> 
> This patch changes the fput() to happen unconditionally if any rootfs is
> loaded. Also, I added some comments to make it a bit more clear why we
> call unpack_to_rootfs() multiple times.
> 
> Fixes: 08865514805d2 ("initramfs: finish fput() before accessing any binary from initramfs")
> Cc: Lokesh Vutla <lokeshvutla@ti.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> Acked-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  init/initramfs.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/init/initramfs.c b/init/initramfs.c
> index 981f286..3a68715 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> @@ -608,9 +608,11 @@ static void __init clean_rootfs(void)
>  
>  static int __init populate_rootfs(void)
>  {
> +	/* Load the built in initramfs */
>  	char *err = unpack_to_rootfs(__initramfs_start, __initramfs_size);
>  	if (err)
>  		panic("%s", err); /* Failed to decompress INTERNAL initramfs */
> +	/* If available load the bootloader supplied initrd */
>  	if (initrd_start) {
>  #ifdef CONFIG_BLK_DEV_RAM
>  		int fd;
> @@ -648,13 +650,14 @@ static int __init populate_rootfs(void)
>  			printk(KERN_EMERG "Initramfs unpacking failed: %s\n", err);
>  		free_initrd();
>  #endif
> -		flush_delayed_fput();
> -		/*
> -		 * Try loading default modules from initramfs.  This gives
> -		 * us a chance to load before device_initcalls.
> -		 */
> -		load_default_modules();
>  	}
> +	flush_delayed_fput();
> +	/*
> +	 * Try loading default modules from initramfs.  This gives
> +	 * us a chance to load before device_initcalls.
> +	 */
> +	load_default_modules();
> +
>  	return 0;
>  }
>  rootfs_initcall(populate_rootfs);

Re: initramfs: Always do fput() and load modules after rootfs populate

[Stafford Horne]

On Sat, May 06, 2017 at 12:02:32AM -0700, Andrei Vagin wrote:
> Hi,
> 
> I see the following error with this patch:
> 
> init/initramfs.c: In function 'populate_rootfs':
> init/initramfs.c:644:2: error: label at end of compound statement
>   done:
>   ^
> scripts/Makefile.build:294: recipe for target 'init/initramfs.o' failed
> make[1]: *** [init/initramfs.o] Error 1

Ahh, I didnt notice this and none of the build robots reported to us.
Sorry about it, the above patch is already merged, so this is not good, Ill
get this fixed.

Could you give me some info on your compiler?

> probably we need something like this:
> diff --git a/init/initramfs.c b/init/initramfs.c
> index 3a68715..9966c0f 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> @@ -641,7 +641,6 @@ static int __init populate_rootfs(void)
>                         sys_close(fd);
>                         free_initrd();
>                 }
> -       done:
>  #else
>                 printk(KERN_INFO "Unpacking initramfs...\n");
>                 err = unpack_to_rootfs((char *)initrd_start,
> @@ -651,6 +650,7 @@ static int __init populate_rootfs(void)
>                 free_initrd();
>  #endif
>         }
> +done:
>         flush_delayed_fput();
>         /*
>          * Try loading default modules from initramfs.  This gives
> 

Right, this looks correct for the above error.

-Stafford

Re: initramfs: Always do fput() and load modules after rootfs populate

[Stafford Horne]

On Sat, May 06, 2017 at 05:17:04PM +0900, Stafford Horne wrote:
> On Sat, May 06, 2017 at 12:02:32AM -0700, Andrei Vagin wrote:
> > Hi,
> > 
> > I see the following error with this patch:
> > 
> > init/initramfs.c: In function 'populate_rootfs':
> > init/initramfs.c:644:2: error: label at end of compound statement
> >   done:
> >   ^
> > scripts/Makefile.build:294: recipe for target 'init/initramfs.o' failed
> > make[1]: *** [init/initramfs.o] Error 1
> 
> Ahh, I didnt notice this and none of the build robots reported to us.
> Sorry about it, the above patch is already merged, so this is not good, Ill
> get this fixed.
> 
> Could you give me some info on your compiler?

Nvermind, I was able to reproduce enabling:

  CONFIG_BLK_DEV_RAM=y

I should have run my allyesconfig testing, instead of just the defconfig!

-Stafford

> > probably we need something like this:
> > diff --git a/init/initramfs.c b/init/initramfs.c
> > index 3a68715..9966c0f 100644
> > --- a/init/initramfs.c
> > +++ b/init/initramfs.c
> > @@ -641,7 +641,6 @@ static int __init populate_rootfs(void)
> >                         sys_close(fd);
> >                         free_initrd();
> >                 }
> > -       done:
> >  #else
> >                 printk(KERN_INFO "Unpacking initramfs...\n");
> >                 err = unpack_to_rootfs((char *)initrd_start,
> > @@ -651,6 +650,7 @@ static int __init populate_rootfs(void)
> >                 free_initrd();
> >  #endif
> >         }
> > +done:
> >         flush_delayed_fput();
> >         /*
> >          * Try loading default modules from initramfs.  This gives
> > 
> 
> Right, this looks correct for the above error.
> 
> -Stafford