linux-kernel - Re: [patch v12 09/13] task isolation: add preempt notifier to sync per-CPU vmstat dirty info to thread info

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]

Date:   Wed, 04 May 2022 19:39:17 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Marcelo Tosatti <mtosatti@...hat.com>
Cc:     linux-kernel@...r.kernel.org, Nitesh Lal <nilal@...hat.com>,
        Nicolas Saenz Julienne <nsaenzju@...hat.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Christoph Lameter <cl@...ux.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Alex Belits <abelits@...its.com>, Peter Xu <peterx@...hat.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Oscar Shiang <oscar0225@...email.tw>
Subject: Re: [patch v12 09/13] task isolation: add preempt notifier to sync
 per-CPU vmstat dirty info to thread info

On Wed, May 04 2022 at 13:32, Marcelo Tosatti wrote:
> On Wed, Apr 27, 2022 at 02:09:16PM +0200, Thomas Gleixner wrote:
>> Aside of that, the existance of this preempt notifier alone tells me
>> that this is either a design fail or has no design in the first place.
>> 
>> The state of vmstat does not matter at all at the point where a task is
>> scheduled in. It matters when an isolated task goes out to user space or
>> enters a VM.
>
> If the following happens, with two threads with names that mean whether
> a thread has task isolation enabled or not:
>
> Thread-no-task-isol, Thread-task-isol.
>
> Events:
>
> not-runnable  		Thread-task-isol
> runnable      		Thread-task-no-isol
> marks vmstat dirty	Thread-task-no-isol (writes to some per-CPU vmstat
> counter)
> not-runnable		Thread-task-no-isol
> runnable		Thread-task-isol
>
> Then we have to transfer the "vmstat dirty" information from per-CPU 
> bool to per-thread TIF_TASK_ISOL bit (so that the
> task_isolation_process_work thing executes on return to userspace).

That's absolute nonsense.

sched_out()      isolated task
vmstat_dirty()
  this_cpu_or(isolwork, VMSTAT);
sched_in()       isolated task

return_to_user()
  local_irq_disable();
  exit_to_user_update_work()
    task_isol_exit_to_user_prepare()
      if (!isolated_task())
          return;
      if (this_cpu_read(isolwork) & current->isol_work_mask)
      	  set_thread_flag(TIF_ISOL);

  exit_to_user_mode_loop()
     do {
        local_irq_enable();
        handle_TIF_bits();
        local_irq_disable();
        exit_to_user_update_work();
        work = read_thread_flags();
     } while (work & EXIT_WORK);
          
Solves the problem nicely with a minimal overhead for non-isolated
tasks.

Plus some of these isolwork bits could even be handled _after_ returning
from exit_do_user_mode_loop() if they are good to be done in irq
diasbled context.

> Sure, but who sets SYSCALL_TASK_ISOL_EXIT or SYSCALL_TASK_ISOL_EXIT ?

It's set once by the prctl() when an isolation feature is enabled for a
task and it's cleared by the prctl() when the last isolation feature is
disabled for the task.

That's then used in:

static inline bool isolated_task()
{
       return current->XXXX_work & TASK_ISOL_EXIT;
}

IOW, the return to user path has

     - _ONE_ extra cache hot conditional for non-isolated tasks.

     - _ONE_ central place to transform the per cpu isolation muck into
       the TIF flag.

See? No sprinkling of TIF bits, no preempt notifiers, nothing.

> Use TIF_TASK_ISOL for "task isolation configured and activated,
> quiesce vmstat work on return to userspace" only, and then have
> the "is vmstat per-CPU data dirty?" information held on 
> task->syscall_work or task->isol_work ? (that will be probably be two
> cachelines).

See above.

> You'd still need the preempt notifier, though (unless i am missing
> something).

Yes, see above.

Using a preempt notifier isa design fail because it tags information at
a place where this information is absolutely irrelevant and subject to
change.

Aside of that this information is not a task property. vmmstat_is_dirty
is a per CPU property. The only point where this per CPU property is
relevant for a task is when the task is isolated and goes out to user
space or enters a VM.

Trying to carry this information in a task flag is fundamentaly wrong
for obvious reasons and causes pointless overhead and complexity for
absolutely no value.

Thanks,

        tglx