2017-09-10 23:43:28

by Rob Landley

[permalink] [raw]
Subject: Re: Patch 0727d35de ("Make initramfs honor CONFIG_DEVTMPFS_MOUNT") breaks boot

Taking another stab at this old issue from last merge window...

> Rob Landley <[email protected]> writes:
>> On 05/23/2017 03:01 AM, Yury Norov wrote:
>>> On Mon, May 22, 2017 at 09:07:54PM -0500, Rob Landley wrote:
>>>> Your userspace mounted a tmpfs over /dev when it couldn't mount a second
>>>> identical instance of devtmpfs over itself. If you had a static /dev in
>>>> initramfs but didn't configure _in_ devtmpfs to your kernel, your broken
>>>> error path would have taken that out too with a pointless tmpfs mount.
>>>
>>> CONFIG_DEVTMPFS_MOUNT is enabled on my machine, so I think your
>>> suggestion is correct. But I didn't do that specifically - I run
>>> almost default kernel based on Ubuntu 14.04 config and environment.
>>
>> I.E. ubuntu has a bug: they enabled CONFIG_DEVTMPFS_MOUNT and then
>> launchd an initramfs instead (which didn't do the automount they
>> requested so why request it), but if CONFIG_DEVTMPFS_MOUNT actually
>> starts working in initramfs they have an insane error path that breaks
>> the system, and does nothing _except_ break the system.

...

On 05/25/2017 01:13 AM, Michael Ellerman wrote:
> Hi Rob,
>
> This is breaking a bunch of my powerpc boxes, for the exact same
> reason, they use a config that has DEVTMPFS_MOUNT=y and that trips
> up the initramfs.

I've continued to use this locally but should probably make another
stab at submitting upstream. The obvious workaround until debian fixes
its 100% obvious bug seems to be:

diff --git a/fs/namespace.c b/fs/namespace.c
index f8893dc..f57d5df 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2417,7 +2417,17 @@ static int do_add_mount(struct mount *newmnt, struct path *path, int mnt_flags)
err = -EBUSY;
if (path->mnt->mnt_sb == newmnt->mnt.mnt_sb &&
path->mnt->mnt_root == path->dentry)
+ {
+ if (IS_ENABLED(CONFIG_DEVTMPFS_MOUNT) &&
+ !strcmp(path->mnt->mnt_sb->s_type->name, "devtmpfs"))
+ {
+ printk(KERN_WARNING "Debian bug workaround for devtmpfs overmount.");
+ printk(KERN_WARNING "This line doesn't output for some reason.");
+
+ err = 0;
+ }
goto unlock;
+ }

err = -EINVAL;
if (d_is_symlink(newmnt->mnt.mnt_root))

Except for the second printk line: If you boot with rdinit=/bin/hush
then the first time you mount -t devtmpfs /dev /dev after boot (with
CONFIG_DEVTMPFS_MOUNT already having mounted it), you get the 0 return
value but the last printk() doesn't output? The second and later times
you repeat it, both printk() lines are output.

What's up with printk?

(I added the second printk because the _first_ one wasn't outputting
that first time. Something is happening to flush the printk() queue
instead of writing it out? Built for x86-64, miniconfig attached for
reference. I tested commit 4dfc2788033d from yesterday.)

Rob


Attachments:
x86_64.miniconf (977.00 B)

2017-09-11 11:45:39

by Petr Mladek

[permalink] [raw]
Subject: Re: Patch 0727d35de ("Make initramfs honor CONFIG_DEVTMPFS_MOUNT") breaks boot

On Sun 2017-09-10 18:43:24, Rob Landley wrote:
> On 05/25/2017 01:13 AM, Michael Ellerman wrote:
> > Hi Rob,
> >
> > This is breaking a bunch of my powerpc boxes, for the exact same
> > reason, they use a config that has DEVTMPFS_MOUNT=y and that trips
> > up the initramfs.
>
> I've continued to use this locally but should probably make another
> stab at submitting upstream. The obvious workaround until debian fixes
> its 100% obvious bug seems to be:
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index f8893dc..f57d5df 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2417,7 +2417,17 @@ static int do_add_mount(struct mount *newmnt, struct path *path, int mnt_flags)
> err = -EBUSY;
> if (path->mnt->mnt_sb == newmnt->mnt.mnt_sb &&
> path->mnt->mnt_root == path->dentry)
> + {
> + if (IS_ENABLED(CONFIG_DEVTMPFS_MOUNT) &&
> + !strcmp(path->mnt->mnt_sb->s_type->name, "devtmpfs"))
> + {
> + printk(KERN_WARNING "Debian bug workaround for devtmpfs overmount.");
> + printk(KERN_WARNING "This line doesn't output for some reason.");
> +
> + err = 0;
> + }
> goto unlock;
> + }
>
> err = -EINVAL;
> if (d_is_symlink(newmnt->mnt.mnt_root))
>
> Except for the second printk line: If you boot with rdinit=/bin/hush
> then the first time you mount -t devtmpfs /dev /dev after boot (with
> CONFIG_DEVTMPFS_MOUNT already having mounted it), you get the 0 return
> value but the last printk() doesn't output? The second and later times
> you repeat it, both printk() lines are output.
>
> What's up with printk?
>
> (I added the second printk because the _first_ one wasn't outputting
> that first time. Something is happening to flush the printk() queue
> instead of writing it out?

You need to add "\n" at the end of the line. Otherwise, it expects
that the message would continue and puts it into a cont buffer.
The buffer is flushed only when another non-continuous message
is added.

This problem is more visible since the commit 5c2992ee7fd8a29d0412
("printk: remove console flushing special cases for partial buffered
lines").

Hmm, Linus wanted to add a timer that would always flush
the cont buffer in a reasonable time frame. But there was a risk
of deadlocks because of circular timer<->printk dependency,
so it never happened.

Maybe, we could setup the timer via an irq_work. We already use
this trick for flushing deferred printk and waking klogd.
It is not nice but it would be easier than the previous mess.

Best Regards,
Petr

2017-09-12 00:42:49

by Sergey Senozhatsky

[permalink] [raw]
Subject: Re: Patch 0727d35de ("Make initramfs honor CONFIG_DEVTMPFS_MOUNT") breaks boot

On (09/11/17 13:45), Petr Mladek wrote:
[..]
> > Except for the second printk line: If you boot with rdinit=/bin/hush
> > then the first time you mount -t devtmpfs /dev /dev after boot (with
> > CONFIG_DEVTMPFS_MOUNT already having mounted it), you get the 0 return
> > value but the last printk() doesn't output? The second and later times
> > you repeat it, both printk() lines are output.
> >
> > What's up with printk?
> >
> > (I added the second printk because the _first_ one wasn't outputting
> > that first time. Something is happening to flush the printk() queue
> > instead of writing it out?
> Maybe, we could setup the timer via an irq_work. We already use
> this trick for flushing deferred printk and waking klogd.
> It is not nice but it would be easier than the previous mess.

printk() and printf() have similar behaviour here. both flush on \n.
so let's keep it the way it is?

-ss

2017-09-13 02:46:14

by Rob Landley

[permalink] [raw]
Subject: Re: Patch 0727d35de ("Make initramfs honor CONFIG_DEVTMPFS_MOUNT") breaks boot

On 09/11/2017 06:45 AM, Petr Mladek wrote:
>> Except for the second printk line: If you boot with rdinit=/bin/hush
>> then the first time you mount -t devtmpfs /dev /dev after boot (with
>> CONFIG_DEVTMPFS_MOUNT already having mounted it), you get the 0 return
>> value but the last printk() doesn't output? The second and later times
>> you repeat it, both printk() lines are output.
>>
>> What's up with printk?
>>
>> (I added the second printk because the _first_ one wasn't outputting
>> that first time. Something is happening to flush the printk() queue
>> instead of writing it out?
>
> You need to add "\n" at the end of the line. Otherwise, it expects
> that the message would continue and puts it into a cont buffer.
> The buffer is flushed only when another non-continuous message
> is added.

Ah. The next one flushes the previous one, meaning when I repeat the
command I get the output I expected the second time but I'm seeing the
_previous_ instance of it, not the current one.

> This problem is more visible since the commit 5c2992ee7fd8a29d0412
> ("printk: remove console flushing special cases for partial buffered
> lines").

Gotcha. My bad.

Thanks,

Rob