[PATCH 1/1] init: fix race between rootfs mount and firmware loading

[PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Roman Pen]

The thing is that built-in modules are being inited before
rootfs mount.  Some of the modules can request firmware loading
from another thread using async 'request_firmware_nowait' call
on inition, so we can catch this kind of race:
   rootfs does not exist yet, but we are going to open and load
   firmware file requesting it from the kernel thread.

Solution is simple: before any rootfs access firmware loader
must wait for rootfs mount.

There are few questions which remain unanswered, but in this
patch I do not want to complicate things and want to fix exactly
the race.  So here are the questions:

1. can 'request_firmware' (sync variant) be called on module inition
   directly, without any threads? if can, that means firmware will
   never be loaded for the driver at first try in case of built-in
   module, because rootfs will be mounted only at the end of kernel
   init sequence.

2. in case of separated '/lib' mount point we still have the problem:
   firmware will be loaded only on further tries, but the first attempt
   will always fail, because '/lib' mount point does not exist.  Seems
   to me this can't be simply fixed, and firmware_request logic needs
   some refactoring.

Probably, there are other good questions which I do not see because
of shallow understanding of init and firmware loading sequences.

Signed-off-by: Roman Pen <r.peniaev@gmail.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 drivers/base/firmware_class.c |  6 ++++++
 include/linux/init.h          |  1 +
 init/main.c                   | 22 ++++++++++++++++++++--
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index bf42430..450c4ae 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -29,6 +29,7 @@
 #include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 #include <linux/security.h>
+#include <linux/init.h>
 
 #include <generated/utsrelease.h>
 
@@ -324,6 +325,11 @@ static int fw_get_filesystem_firmware(struct device *device,
 	int rc = -ENOENT;
 	char *path = __getname();
 
+	/* Before any file access we have to wait for rootfs.
+	   In case of built-in module we can race with kernel init
+	   thread, which has not mounted rootfs yet */
+	wait_for_rootfs();
+
 	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
 		struct file *file;
 
diff --git a/include/linux/init.h b/include/linux/init.h
index 2df8e8d..83233ae 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -152,6 +152,7 @@ void setup_arch(char **);
 void prepare_namespace(void);
 void __init load_default_modules(void);
 int __init init_rootfs(void);
+void wait_for_rootfs(void);
 
 extern void (*late_time_init)(void);
 
diff --git a/init/main.c b/init/main.c
index bb1aed9..15e1bb1 100644
--- a/init/main.c
+++ b/init/main.c
@@ -973,6 +973,22 @@ static int __ref kernel_init(void *unused)
 	      "See Linux Documentation/init.txt for guidance.");
 }
 
+static DECLARE_COMPLETION(rootfs_mounted);
+
+void wait_for_rootfs(void)
+{
+	/* Avoid waiting for ourselves */
+	if (is_global_init(current))
+		pr_warn("it is not a good idea to wait for rootfs mount from init task\n");
+	else
+		wait_for_completion(&rootfs_mounted);
+}
+
+static inline void wake_up_rootfs_waiters(void)
+{
+	complete(&rootfs_mounted);
+}
+
 static noinline void __init kernel_init_freeable(void)
 {
 	/*
@@ -1025,9 +1041,11 @@ static noinline void __init kernel_init_freeable(void)
 
 	/*
 	 * Ok, we have completed the initial bootup, and
-	 * we're essentially up and running. Get rid of the
-	 * initmem segments and start the user-mode stuff..
+	 * we're essentially up and running.  Wake up rootfs
+	 * mount waiters, get rid of the initmem segments,
+	 * and start the user-mode stuff..
 	 */
+	wake_up_rootfs_waiters();
 
 	/* rootfs is available now, try loading default modules */
 	load_default_modules();
-- 
2.0.0

Re: [PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Oleg Nesterov]

On 09/15, Roman Pen wrote:
>
> +static DECLARE_COMPLETION(rootfs_mounted);
> +
> +void wait_for_rootfs(void)
> +{
> +	/* Avoid waiting for ourselves */
> +	if (is_global_init(current))
> +		pr_warn("it is not a good idea to wait for rootfs mount from init task\n");
> +	else
> +		wait_for_completion(&rootfs_mounted);
> +}
> +
> +static inline void wake_up_rootfs_waiters(void)
> +{
> +	complete(&rootfs_mounted);
> +}

Probably you need complete_all() ?

Otherwise wait_for_rootfs() can return only once, the next call will block
forever. And perhaps it would be better to use another primitive, even if
UINT_MAX/2 should be enough.

Oleg.

Re: [PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Roman Peniaev]

On Tue, Sep 16, 2014 at 1:39 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 09/15, Roman Pen wrote:
>>
>> +static DECLARE_COMPLETION(rootfs_mounted);
>> +
>> +void wait_for_rootfs(void)
>> +{
>> +     /* Avoid waiting for ourselves */
>> +     if (is_global_init(current))
>> +             pr_warn("it is not a good idea to wait for rootfs mount from init task\n");
>> +     else
>> +             wait_for_completion(&rootfs_mounted);
>> +}
>> +
>> +static inline void wake_up_rootfs_waiters(void)
>> +{
>> +     complete(&rootfs_mounted);
>> +}
>
> Probably you need complete_all() ?

Yes, of course. My fault.

>
> Otherwise wait_for_rootfs() can return only once, the next call will block
> forever. And perhaps it would be better to use another primitive, even if
> UINT_MAX/2 should be enough.

And why do you think completion is not good for this?
Seems it is impossible to have so many threads on early init, which wait
for rootfs.

Oleg, I prepared version 2 (will be in reply to this email). What I did there:

1/ wait_for_rootfs should be exported, because framework_class.c is a tristate,
    thus can be built as module.

2/ replaced completion on wait_event and wake_up_all pair.

3/ did some minor tweaks.

--
Roman

[v2 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Roman Pen]

The thing is that built-in modules are being inited before
rootfs mount.  Some of the modules can request firmware loading
from another thread using async 'request_firmware_nowait' call
on inition, so we can catch this kind of race:
   rootfs does not exist yet, but we are going to open and load
   firmware file requesting it from the kernel thread.

Solution is simple: before any rootfs access firmware loader
must wait for rootfs mount.

There are few questions which remain unanswered, but in this
patch I do not want to complicate things and want to fix exactly
the race.  So here are the questions:

1. can 'request_firmware' (sync variant) be called on module inition
   directly, without any threads? if can, that means firmware will
   never be loaded for the driver at first try in case of built-in
   module, because rootfs will be mounted only at the end of kernel
   init sequence.

2. in case of separated '/lib' mount point we still have the problem:
   firmware will be loaded only on further tries, but the first attempt
   will always fail, because '/lib' mount point does not exist.  Seems
   to me this can't be simply fixed, and firmware_request logic needs
   some refactoring.

Probably, there are other good questions which I do not see because
of shallow understanding of init and firmware loading sequences.

Signed-off-by: Roman Pen <r.peniaev@gmail.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 drivers/base/firmware_class.c |  6 ++++++
 include/linux/init.h          |  1 +
 init/main.c                   | 29 +++++++++++++++++++++++++++--
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index bf42430..450c4ae 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -29,6 +29,7 @@
 #include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 #include <linux/security.h>
+#include <linux/init.h>
 
 #include <generated/utsrelease.h>
 
@@ -324,6 +325,11 @@ static int fw_get_filesystem_firmware(struct device *device,
 	int rc = -ENOENT;
 	char *path = __getname();
 
+	/* Before any file access we have to wait for rootfs.
+	   In case of built-in module we can race with kernel init
+	   thread, which has not mounted rootfs yet */
+	wait_for_rootfs();
+
 	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
 		struct file *file;
 
diff --git a/include/linux/init.h b/include/linux/init.h
index 2df8e8d..83233ae 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -152,6 +152,7 @@ void setup_arch(char **);
 void prepare_namespace(void);
 void __init load_default_modules(void);
 int __init init_rootfs(void);
+void wait_for_rootfs(void);
 
 extern void (*late_time_init)(void);
 
diff --git a/init/main.c b/init/main.c
index bb1aed9..65d5568 100644
--- a/init/main.c
+++ b/init/main.c
@@ -973,6 +973,29 @@ static int __ref kernel_init(void *unused)
 	      "See Linux Documentation/init.txt for guidance.");
 }
 
+static DECLARE_WAIT_QUEUE_HEAD(rootfs_waitq);
+static bool rootfs_mounted;
+
+void wait_for_rootfs(void)
+{
+	/* Avoid waiting for ourselves */
+	if (is_global_init(current))
+		pr_warn("init: it is not a good idea to wait for the rootfs mount from the init task\n");
+	else
+		wait_event(rootfs_waitq, rootfs_mounted);
+}
+EXPORT_SYMBOL(wait_for_rootfs);
+
+static inline void wake_up_rootfs_waiters(void)
+{
+	rootfs_mounted = true;
+	/* wake_up guarantees write memory barrier if and only if
+	   there is a task to be woken up, it is not always true
+	   for our case. */
+	smp_wmb();
+	wake_up_all(&rootfs_waitq);
+}
+
 static noinline void __init kernel_init_freeable(void)
 {
 	/*
@@ -1025,9 +1048,11 @@ static noinline void __init kernel_init_freeable(void)
 
 	/*
 	 * Ok, we have completed the initial bootup, and
-	 * we're essentially up and running. Get rid of the
-	 * initmem segments and start the user-mode stuff..
+	 * we're essentially up and running.  Wake up the
+	 * rootfs mount waiters, get rid of the initmem
+	 * segments, and start the user-mode stuff..
 	 */
+	wake_up_rootfs_waiters();
 
 	/* rootfs is available now, try loading default modules */
 	load_default_modules();
-- 
2.0.0

Re: [v2 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Oleg Nesterov]

On 09/17, Roman Pen wrote:
>
> +void wait_for_rootfs(void)
> +{
> +	/* Avoid waiting for ourselves */
> +	if (is_global_init(current))
> +		pr_warn("init: it is not a good idea to wait for the rootfs mount from the init task\n");
> +	else
> +		wait_event(rootfs_waitq, rootfs_mounted);
> +}

Well, this pr_warn() doesn't look very useful, how about

	if (WARN_ON((is_global_init(current))))
		return;

? this will show the caller.

> +static inline void wake_up_rootfs_waiters(void)
> +{
> +	rootfs_mounted = true;
> +	/* wake_up guarantees write memory barrier if and only if
> +	   there is a task to be woken up, it is not always true
> +	   for our case. */

Yes, but this doesn't matter. wait_event() takes care,

> +	smp_wmb();

so please remove this wmb() and the comment.

Oleg.

Re: [PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Oleg Nesterov]

On 09/17, Roman Peniaev wrote:
>
> On Tue, Sep 16, 2014 at 1:39 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 09/15, Roman Pen wrote:
> >>
> >
> > Otherwise wait_for_rootfs() can return only once, the next call will block
> > forever. And perhaps it would be better to use another primitive, even if
> > UINT_MAX/2 should be enough.
>
> And why do you think completion is not good for this?
> Seems it is impossible to have so many threads on early init, which wait
> for rootfs.

Yes, but it is not only called during init? Suppose that a user does
insmod + rmmod in a loop, and the driver calls request_firmware(). This
will hang after UINT_MAX/2 iterations. Sure, this can't happen in practice,
but still this doesn't look good imho.

Oleg.

Re: [v2 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Roman Peniaev]

On Thu, Sep 18, 2014 at 2:46 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 09/17, Roman Pen wrote:
>>
>> +void wait_for_rootfs(void)
>> +{
>> +     /* Avoid waiting for ourselves */
>> +     if (is_global_init(current))
>> +             pr_warn("init: it is not a good idea to wait for the rootfs mount from the init task\n");
>> +     else
>> +             wait_event(rootfs_waitq, rootfs_mounted);
>> +}
>
> Well, this pr_warn() doesn't look very useful, how about
>
>         if (WARN_ON((is_global_init(current))))
>                 return;
>
> ? this will show the caller.

Ok, I will change.

>
>> +static inline void wake_up_rootfs_waiters(void)
>> +{
>> +     rootfs_mounted = true;
>> +     /* wake_up guarantees write memory barrier if and only if
>> +        there is a task to be woken up, it is not always true
>> +        for our case. */
>
> Yes, but this doesn't matter. wait_event() takes care,
>
>> +     smp_wmb();
>
> so please remove this wmb() and the comment.


I was confused by these lines in memory-barriers.txt document:

 (5) LOCK operations.

     This acts as a one-way permeable barrier.  It guarantees that all memory
     operations after the LOCK operation will appear to happen after the LOCK
     operation with respect to the other components of the system.

So it turns out to be, that without explicit barrier the order can be changed
and CONDITION set can be done _after_ list check.

But, it still can't cross UNLOCK border and will be between LOCK-UNLOCK.
And this is the key.

If my final understanding is right (please, correct me if I am still wrong),
following reordering can happen, but we are fine with it:

wake_up_rootfs                 wait_event

LOCK
check the list, but empty
set CONDITION <<< reordered
UNLOCK

                                          LOCK
                                          add to the list
                                          rmb            <<< now we
see CONDITION
                                          UNLOCK

                                          check CONDITION <<< it is
set, we are woken up


--
Roman

[v3 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Roman Pen]

The thing is that built-in modules are being inited before
rootfs mount.  Some of the modules can request firmware loading
from another thread using async 'request_firmware_nowait' call
on inition, so we can catch this kind of race:
   rootfs does not exist yet, but we are going to open and load
   firmware file requesting it from the kernel thread.

Solution is simple: before any rootfs access firmware loader
must wait for rootfs mount.

There are few questions which remain unanswered, but in this
patch I do not want to complicate things and want to fix exactly
the race.  So here are the questions:

1. can 'request_firmware' (sync variant) be called on module inition
   directly, without any threads? if can, that means firmware will
   never be loaded for the driver at first try in case of built-in
   module, because rootfs will be mounted only at the end of kernel
   init sequence.

2. in case of separated '/lib' mount point we still have the problem:
   firmware will be loaded only on further tries, but the first attempt
   will always fail, because '/lib' mount point does not exist.  Seems
   to me this can't be simply fixed, and firmware_request logic needs
   some refactoring.

Probably, there are other good questions which I do not see because
of shallow understanding of init and firmware loading sequences.

Signed-off-by: Roman Pen <r.peniaev@gmail.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 drivers/base/firmware_class.c |  6 ++++++
 include/linux/init.h          |  1 +
 init/main.c                   | 25 +++++++++++++++++++++++--
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index bf42430..450c4ae 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -29,6 +29,7 @@
 #include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 #include <linux/security.h>
+#include <linux/init.h>
 
 #include <generated/utsrelease.h>
 
@@ -324,6 +325,11 @@ static int fw_get_filesystem_firmware(struct device *device,
 	int rc = -ENOENT;
 	char *path = __getname();
 
+	/* Before any file access we have to wait for rootfs.
+	   In case of built-in module we can race with kernel init
+	   thread, which has not mounted rootfs yet */
+	wait_for_rootfs();
+
 	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
 		struct file *file;
 
diff --git a/include/linux/init.h b/include/linux/init.h
index 2df8e8d..83233ae 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -152,6 +152,7 @@ void setup_arch(char **);
 void prepare_namespace(void);
 void __init load_default_modules(void);
 int __init init_rootfs(void);
+void wait_for_rootfs(void);
 
 extern void (*late_time_init)(void);
 
diff --git a/init/main.c b/init/main.c
index bb1aed9..9a29530 100644
--- a/init/main.c
+++ b/init/main.c
@@ -973,6 +973,25 @@ static int __ref kernel_init(void *unused)
 	      "See Linux Documentation/init.txt for guidance.");
 }
 
+static DECLARE_WAIT_QUEUE_HEAD(rootfs_waitq);
+static bool rootfs_mounted;
+
+void wait_for_rootfs(void)
+{
+	/* Avoid waiting for ourselves */
+	if (WARN_ON(is_global_init(current)))
+		return;
+	else
+		wait_event(rootfs_waitq, rootfs_mounted);
+}
+EXPORT_SYMBOL(wait_for_rootfs);
+
+static inline void wake_up_rootfs_waiters(void)
+{
+	rootfs_mounted = true;
+	wake_up_all(&rootfs_waitq);
+}
+
 static noinline void __init kernel_init_freeable(void)
 {
 	/*
@@ -1025,9 +1044,11 @@ static noinline void __init kernel_init_freeable(void)
 
 	/*
 	 * Ok, we have completed the initial bootup, and
-	 * we're essentially up and running. Get rid of the
-	 * initmem segments and start the user-mode stuff..
+	 * we're essentially up and running.  Wake up the
+	 * rootfs mount waiters, get rid of the initmem
+	 * segments, and start the user-mode stuff..
 	 */
+	wake_up_rootfs_waiters();
 
 	/* rootfs is available now, try loading default modules */
 	load_default_modules();
-- 
2.0.0

Re: [v2 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Oleg Nesterov]

On 09/18, Roman Peniaev wrote:
>
> If my final understanding is right (please, correct me if I am still wrong),
> following reordering can happen, but we are fine with it:
>
> wake_up_rootfs                 wait_event
>
> LOCK
> check the list, but empty
> set CONDITION <<< reordered
> UNLOCK

Yes,

>                                           LOCK
>                                           add to the list
>                                           rmb            <<< now we
> see CONDITION

Yes, but note that "now we see CONDITION" is true right after LOCK,
not sure what does this "rmb" in your pseudo mean. If you meant mb()
implied by set_current_state(), please see the comment above
prepare_to_wait().

>                                           UNLOCK
>
>                                           check CONDITION <<< it is
> set, we are woken up

Yes. In short, we rely on wait_queue_head_t->lock. Once a CPU takes
this lock, it should see all changes done by another CPU before it
released the same lock.

Oleg.

Re: [v3 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Oleg Nesterov]

On 09/18, Roman Pen wrote:
>
> +void wait_for_rootfs(void)
> +{
> +	/* Avoid waiting for ourselves */
> +	if (WARN_ON(is_global_init(current)))
> +		return;
> +	else
> +		wait_event(rootfs_waitq, rootfs_mounted);
> +}

Ah, wait, the is_global_init() check doesn't look right in any case.

What if /sbin/init does sys_init_module() and this module does
request_firmware() ?

I think we should only warn if init is going to wait,

	/* Avoid waiting for ourselves */
	if (rootfs_mounted || WARN_ON(is_global_init(current)))
		return;

	__wait_event(rootfs_waitq, rootfs_mounted);


> +EXPORT_SYMBOL(wait_for_rootfs);

Why?

Oleg.

Re: [v3 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Roman Peniaev]

On Fri, Sep 19, 2014 at 2:41 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 09/18, Roman Pen wrote:
>>
>> +void wait_for_rootfs(void)
>> +{
>> +     /* Avoid waiting for ourselves */
>> +     if (WARN_ON(is_global_init(current)))
>> +             return;
>> +     else
>> +             wait_event(rootfs_waitq, rootfs_mounted);
>> +}
>
> Ah, wait, the is_global_init() check doesn't look right in any case.
>
> What if /sbin/init does sys_init_module() and this module does
> request_firmware() ?

Yeah, right. It turned out to be more tricky than I expected.
Will fix.

>
> I think we should only warn if init is going to wait,
>
>         /* Avoid waiting for ourselves */
>         if (rootfs_mounted || WARN_ON(is_global_init(current)))
>                 return;
>
>         __wait_event(rootfs_waitq, rootfs_mounted);
>
>
>> +EXPORT_SYMBOL(wait_for_rootfs);
>
> Why?

CONFIG_FW_LOADER (which includes firmware_class.c to compilation
sequence) is declared as tristate,
thus it can be built as module. Seems I can break  loading of this
module if skip exporting, right?

--
Roman

[v4 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Roman Pen]

The thing is that built-in modules are being inited before
rootfs mount.  Some of the modules can request firmware loading
from another thread using async 'request_firmware_nowait' call
on inition, so we can catch this kind of race:
   rootfs does not exist yet, but we are going to open and load
   firmware file requesting it from the kernel thread.

Solution is simple: before any rootfs access firmware loader
must wait for rootfs mount.

There are few questions which remain unanswered, but in this
patch I do not want to complicate things and want to fix exactly
the race.  So here are the questions:

1. can 'request_firmware' (sync variant) be called on module inition
   directly, without any threads? if can, that means firmware will
   never be loaded for the driver at first try in case of built-in
   module, because rootfs will be mounted only at the end of kernel
   init sequence.

2. in case of separated '/lib' mount point we still have the problem:
   firmware will be loaded only on further tries, but the first attempt
   will always fail, because '/lib' mount point does not exist.  Seems
   to me this can't be simply fixed, and firmware_request logic needs
   some refactoring.

Probably, there are other good questions which I do not see because
of shallow understanding of init and firmware loading sequences.

Signed-off-by: Roman Pen <r.peniaev@gmail.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 drivers/base/firmware_class.c |  6 ++++++
 include/linux/init.h          |  1 +
 init/main.c                   | 33 +++++++++++++++++++++++++++++++--
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index bf42430..450c4ae 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -29,6 +29,7 @@
 #include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 #include <linux/security.h>
+#include <linux/init.h>
 
 #include <generated/utsrelease.h>
 
@@ -324,6 +325,11 @@ static int fw_get_filesystem_firmware(struct device *device,
 	int rc = -ENOENT;
 	char *path = __getname();
 
+	/* Before any file access we have to wait for rootfs.
+	   In case of built-in module we can race with kernel init
+	   thread, which has not mounted rootfs yet */
+	wait_for_rootfs();
+
 	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
 		struct file *file;
 
diff --git a/include/linux/init.h b/include/linux/init.h
index 2df8e8d..83233ae 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -152,6 +152,7 @@ void setup_arch(char **);
 void prepare_namespace(void);
 void __init load_default_modules(void);
 int __init init_rootfs(void);
+void wait_for_rootfs(void);
 
 extern void (*late_time_init)(void);
 
diff --git a/init/main.c b/init/main.c
index bb1aed9..2cfe187 100644
--- a/init/main.c
+++ b/init/main.c
@@ -973,6 +973,33 @@ static int __ref kernel_init(void *unused)
 	      "See Linux Documentation/init.txt for guidance.");
 }
 
+static DECLARE_WAIT_QUEUE_HEAD(rootfs_waitq);
+static bool rootfs_mounted;
+
+void wait_for_rootfs(void)
+{
+	/* Here we try to protect from a few things:
+	 * 1. Avoid waiting for ourselves, when init thread has not
+	 *    mounted rootfs yet.
+	 * 2. Avoid warning if call chain was initiated from userspace
+	 *    /sbin/init. For example when /sbin/init loads the driver,
+	 *    which, in turn, wants to access rootfs (e.g. firmware loading),
+	 *    thus it has to be sure, that rootfs has been successfully
+	 *    mounted.
+	 */
+	if (rootfs_mounted || WARN_ON(is_global_init(current)))
+		return;
+	else
+		wait_event(rootfs_waitq, rootfs_mounted);
+}
+EXPORT_SYMBOL(wait_for_rootfs);
+
+static inline void wake_up_rootfs_waiters(void)
+{
+	rootfs_mounted = true;
+	wake_up_all(&rootfs_waitq);
+}
+
 static noinline void __init kernel_init_freeable(void)
 {
 	/*
@@ -1025,9 +1052,11 @@ static noinline void __init kernel_init_freeable(void)
 
 	/*
 	 * Ok, we have completed the initial bootup, and
-	 * we're essentially up and running. Get rid of the
-	 * initmem segments and start the user-mode stuff..
+	 * we're essentially up and running.  Wake up the
+	 * rootfs mount waiters, get rid of the initmem
+	 * segments, and start the user-mode stuff..
 	 */
+	wake_up_rootfs_waiters();
 
 	/* rootfs is available now, try loading default modules */
 	load_default_modules();
-- 
2.0.0

Re: [v3 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Oleg Nesterov]

On 09/19, Roman Peniaev wrote:
>
> On Fri, Sep 19, 2014 at 2:41 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 09/18, Roman Pen wrote:
> >>
> >> +EXPORT_SYMBOL(wait_for_rootfs);
> >
> > Why?
>
> CONFIG_FW_LOADER (which includes firmware_class.c to compilation
> sequence) is declared as tristate,

Ah, I didn't know it can be compiled as a module. Thanks, you are
right.

Oleg.

Re: [v4 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Oleg Nesterov]

On 09/19, Roman Pen wrote:
>
> +void wait_for_rootfs(void)
> +{
> +	/* Here we try to protect from a few things:
> +	 * 1. Avoid waiting for ourselves, when init thread has not
> +	 *    mounted rootfs yet.
> +	 * 2. Avoid warning if call chain was initiated from userspace
> +	 *    /sbin/init. For example when /sbin/init loads the driver,
> +	 *    which, in turn, wants to access rootfs (e.g. firmware loading),
> +	 *    thus it has to be sure, that rootfs has been successfully
> +	 *    mounted.
> +	 */
> +	if (rootfs_mounted || WARN_ON(is_global_init(current)))
> +		return;
> +	else
> +		wait_event(rootfs_waitq, rootfs_mounted);
> +}

Well, in this case __wait_event() makes more sense, we already checked
rootfs_mounted, but this is minor.

Reviewed-by: Oleg Nesterov <oleg@redhat.com>


But I hope that Greg can take a look too. I do not understand the problem
space enough, so I am not sure this is the "right" fix. Say, perhaps we
should change the drivers which abuse request_firmware(), or do something
else.

Oleg.

Re: [v4 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Greg Kroah-Hartman]

On Fri, Sep 19, 2014 at 09:44:24PM +0900, Roman Pen wrote:
> The thing is that built-in modules are being inited before
> rootfs mount.  Some of the modules can request firmware loading
> from another thread using async 'request_firmware_nowait' call
> on inition, so we can catch this kind of race:
>    rootfs does not exist yet, but we are going to open and load
>    firmware file requesting it from the kernel thread.
> 
> Solution is simple: before any rootfs access firmware loader
> must wait for rootfs mount.

Even simpler solution, don't do that :)

If your hardware needs firmware this early, and you don't put the
firmare into the initrd/initramfs, then it will just be loaded later on,
when the root filesystem is needed and the driver asks for it again
(your driver is failing "nicely" and allows the firmware to be loaded
later, right?  If not, that should be fixed...)

Or, build the firmware into the kernel, I think we still have that
option for some drivers that really wanted/needed this.

What has changed recently that requires this type of patch?  What is
wrong with the above solutions?

thanks,

greg k-h

Re: [v4 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Roman Peniaev]

On Sat, Sep 20, 2014 at 6:42 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Sep 19, 2014 at 09:44:24PM +0900, Roman Pen wrote:
>> The thing is that built-in modules are being inited before
>> rootfs mount.  Some of the modules can request firmware loading
>> from another thread using async 'request_firmware_nowait' call
>> on inition, so we can catch this kind of race:
>>    rootfs does not exist yet, but we are going to open and load
>>    firmware file requesting it from the kernel thread.
>>
>> Solution is simple: before any rootfs access firmware loader
>> must wait for rootfs mount.
>
> Even simpler solution, don't do that :)
>
> If your hardware needs firmware this early, and you don't put the
> firmare into the initrd/initramfs, then it will just be loaded later on,
> when the root filesystem is needed and the driver asks for it again
> (your driver is failing "nicely" and allows the firmware to be loaded
> later, right?  If not, that should be fixed...)
>
> Or, build the firmware into the kernel, I think we still have that
> option for some drivers that really wanted/needed this.
>
> What has changed recently that requires this type of patch?  What is
> wrong with the above solutions?


I definitely agree with that "do not do that", I do agree.
But, I did not find any other solution, because in my case this "to be loaded
later" really matters. The source of the problem is the rtlwifi, which randomly
delays the startup, it randomly warns me about "firmware loading failed" and
repeats loading later. This later costs around one second. For my tiny arm
setup it is important. And it happens because of this race.

So, all I want to say with this patch is:

1. 'mount / firmware loading' race exists, and it can cost time.
2. it affects the path of firmware loading (under the path I mean call
chain, not
    the path on the rootfs): sometimes firmware can be loaded on first
try by the
    request from the kernel, sometimes it can be loaded by push from userspace,
    sometimes it can be loaded from next retry from kernel
3. the firmware loading path is always unpredictable.

It turns out to be that to have predictable loading path and predictable timings
I have to build firmware into the kernel, that means that
request_firmware mechanics
does not work as expected, because it relies on the 'rootfs exists'
statement, but it is
not always true.

Frankly speaking I also do not like this patch, but as I told I did
not find any other _good_
solution . If we can discuss here better way, I can do further work
towards improvements
of request_firmware. Of course if it really makes any sense.

--
Roman

Re: [v4 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Roman Peniaev]

Thanks, Oleg, for the review.

--
Roman

On Sat, Sep 20, 2014 at 4:45 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 09/19, Roman Pen wrote:
>>
>> +void wait_for_rootfs(void)
>> +{
>> +     /* Here we try to protect from a few things:
>> +      * 1. Avoid waiting for ourselves, when init thread has not
>> +      *    mounted rootfs yet.
>> +      * 2. Avoid warning if call chain was initiated from userspace
>> +      *    /sbin/init. For example when /sbin/init loads the driver,
>> +      *    which, in turn, wants to access rootfs (e.g. firmware loading),
>> +      *    thus it has to be sure, that rootfs has been successfully
>> +      *    mounted.
>> +      */
>> +     if (rootfs_mounted || WARN_ON(is_global_init(current)))
>> +             return;
>> +     else
>> +             wait_event(rootfs_waitq, rootfs_mounted);
>> +}
>
> Well, in this case __wait_event() makes more sense, we already checked
> rootfs_mounted, but this is minor.
>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>
>
> But I hope that Greg can take a look too. I do not understand the problem
> space enough, so I am not sure this is the "right" fix. Say, perhaps we
> should change the drivers which abuse request_firmware(), or do something
> else.
>
> Oleg.
>

Re: [v4 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Greg Kroah-Hartman]

On Sat, Sep 20, 2014 at 10:18:39PM +0900, Roman Peniaev wrote:
> On Sat, Sep 20, 2014 at 6:42 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, Sep 19, 2014 at 09:44:24PM +0900, Roman Pen wrote:
> >> The thing is that built-in modules are being inited before
> >> rootfs mount.  Some of the modules can request firmware loading
> >> from another thread using async 'request_firmware_nowait' call
> >> on inition, so we can catch this kind of race:
> >>    rootfs does not exist yet, but we are going to open and load
> >>    firmware file requesting it from the kernel thread.
> >>
> >> Solution is simple: before any rootfs access firmware loader
> >> must wait for rootfs mount.
> >
> > Even simpler solution, don't do that :)
> >
> > If your hardware needs firmware this early, and you don't put the
> > firmare into the initrd/initramfs, then it will just be loaded later on,
> > when the root filesystem is needed and the driver asks for it again
> > (your driver is failing "nicely" and allows the firmware to be loaded
> > later, right?  If not, that should be fixed...)
> >
> > Or, build the firmware into the kernel, I think we still have that
> > option for some drivers that really wanted/needed this.
> >
> > What has changed recently that requires this type of patch?  What is
> > wrong with the above solutions?
> 
> 
> I definitely agree with that "do not do that", I do agree.

Great, no need for a patch, we can move on now.  :)

> But, I did not find any other solution, because in my case this "to be loaded
> later" really matters. The source of the problem is the rtlwifi, which randomly
> delays the startup, it randomly warns me about "firmware loading failed" and
> repeats loading later. This later costs around one second. For my tiny arm
> setup it is important. And it happens because of this race.

What "costs one second"?  I don't understand, if the firmware isn't
there, are you timing out?  Fix the driver to do async firmware loading
and that timeout shouldn't be an issue.

> So, all I want to say with this patch is:
> 
> 1. 'mount / firmware loading' race exists, and it can cost time.

Switch the driver to do async firmware loading and you should be able to
fix this.

thanks,

greg k-h

Re: [v4 PATCH 1/1] init: fix race between rootfs mount and firmware loading

[Roman Peniaev]

On Sat, Sep 20, 2014 at 11:42 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Sep 20, 2014 at 10:18:39PM +0900, Roman Peniaev wrote:
>> On Sat, Sep 20, 2014 at 6:42 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Fri, Sep 19, 2014 at 09:44:24PM +0900, Roman Pen wrote:
>> >> The thing is that built-in modules are being inited before
>> >> rootfs mount.  Some of the modules can request firmware loading
>> >> from another thread using async 'request_firmware_nowait' call
>> >> on inition, so we can catch this kind of race:
>> >>    rootfs does not exist yet, but we are going to open and load
>> >>    firmware file requesting it from the kernel thread.
>> >>
>> >> Solution is simple: before any rootfs access firmware loader
>> >> must wait for rootfs mount.
>> >
>> > Even simpler solution, don't do that :)
>> >
>> > If your hardware needs firmware this early, and you don't put the
>> > firmare into the initrd/initramfs, then it will just be loaded later on,
>> > when the root filesystem is needed and the driver asks for it again
>> > (your driver is failing "nicely" and allows the firmware to be loaded
>> > later, right?  If not, that should be fixed...)
>> >
>> > Or, build the firmware into the kernel, I think we still have that
>> > option for some drivers that really wanted/needed this.
>> >
>> > What has changed recently that requires this type of patch?  What is
>> > wrong with the above solutions?
>>
>>
>> I definitely agree with that "do not do that", I do agree.
>
> Great, no need for a patch, we can move on now.  :)
>
>> But, I did not find any other solution, because in my case this "to be loaded
>> later" really matters. The source of the problem is the rtlwifi, which randomly
>> delays the startup, it randomly warns me about "firmware loading failed" and
>> repeats loading later. This later costs around one second. For my tiny arm
>> setup it is important. And it happens because of this race.
>
> What "costs one second"?  I don't understand, if the firmware isn't
> there, are you timing out?  Fix the driver to do async firmware loading
> and that timeout shouldn't be an issue.

Firmware exists. Driver is built-in (this is the key, because do_initcall_level,
which inits all static stuff including built-in drivers, is started
before rootfs mount).
Driver calls request_firmware_nowait on inition, so firmware_class will attempt
to load this firmware and will randomly fail. After the possible fail
the driver will try to
load alternative firmware and also can randomly fail, because the
rootfs is still not
mounted. Then driver gives up.

>
>> So, all I want to say with this patch is:
>>
>> 1. 'mount / firmware loading' race exists, and it can cost time.
>
> Switch the driver to do async firmware loading and you should be able to
> fix this.

Under async firmware loading you mean request_firmware_nowait ?
Driver uses exactly this call.

--
Roman