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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 18 Sep 2014 22:31:45 +0900
From:	Roman Peniaev <r.peniaev@...il.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ming Lei <ming.lei@...onical.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [v2 PATCH 1/1] init: fix race between rootfs mount and firmware loading

On Thu, Sep 18, 2014 at 2:46 AM, Oleg Nesterov <oleg@...hat.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/