linux-kernel - Re: [PATCH 09/25] kcpustat: Track running task following vtime sequences

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20181120135849.GS2131@hirez.programming.kicks-ass.net>
Date:   Tue, 20 Nov 2018 14:58:49 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Frederic Weisbecker <frederic@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Wanpeng Li <wanpengli@...cent.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Yauheni Kaliuta <yauheni.kaliuta@...hat.com>,
        Ingo Molnar <mingo@...nel.org>, Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH 09/25] kcpustat: Track running task following vtime
 sequences

On Wed, Nov 14, 2018 at 03:45:53AM +0100, Frederic Weisbecker wrote:
> In order to make kcpustat vtime aware (ie: work on nohz_full without
> freezing), we need to track the task running on the CPU in order to
> fetch its vtime delta and add it to the relevant kcpustat field.
> 
> The most efficient way to track this task is to use RCU. The task is
> assigned on context switch right after we flush the vtime of the previous
> task and the next task has been set on vtime.
> 
> Things are then prepared to be ordered that way:
> 
>              WRITER (ctx switch)                READER
>              ------------------            -----------------------
>         vtime_seqcount_write_lock(prev)     rcu_read_lock()
>         //flush prev vtime                  curr = rcu_dereference(kcpustat->curr)
>         vtime_seqcount_write_unlock(prev)   vtime_seqcount_read_start(curr)
>                                             //fetch curr vtime
>         vtime_seqcount_lock(next)           vtime_seqcount_read_end(curr)
>         //Init vtime                        rcu_read_unlock()
>         vtime_seqcount_unlock(next)
> 
>         rcu_assign_pointer(kcpustat->curr, next)
> 
> With this ordering layout, we are sure that we get a sequence with a
> coherent couple (task cputime, kcpustat).

I'm confused; earlier you added a ->cpu member; but I don't see that
used.

Also, I'm confuddled on the purpose of rcu_assign_pointer(), what does
the store_release therein ensure?

Also, I'm pretty sure the below is terminally broken; task_struct is not
rcu-freed, and therefore the above scenario is just broken. Nothing
stops the task from going away right after rcu_dereference().