linux-kernel - Re: [patch v8 03/10] task isolation: sync vmstats on return to userspace

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] [day] [month] [year] [list]

Message-ID: <20220127180151.GA308637@lothringen>
Date:   Thu, 27 Jan 2022 19:01:51 +0100
From:   Frederic Weisbecker <frederic@...nel.org>
To:     Marcelo Tosatti <mtosatti@...hat.com>
Cc:     linux-kernel@...r.kernel.org, Nitesh Lal <nilal@...hat.com>,
        Nicolas Saenz Julienne <nsaenzju@...hat.com>,
        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>,
        Thomas Gleixner <tglx@...utronix.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>
Subject: Re: [patch v8 03/10] task isolation: sync vmstats on return to
 userspace

On Thu, Jan 27, 2022 at 01:47:07PM -0300, Marcelo Tosatti wrote:
> > > @@ -177,6 +179,9 @@ static unsigned long exit_to_user_mode_l
> > >  		/* Architecture specific TIF work */
> > >  		arch_exit_to_user_mode_work(regs, ti_work);
> > >  
> > > +		if (tsk_isol_work)
> > > +			isolation_exit_to_user_mode();
> > > +
> > >  		/*
> > >  		 * Disable interrupts and reevaluate the work flags as they
> > >  		 * might have changed while interrupts and preemption was
> > > @@ -188,6 +193,7 @@ static unsigned long exit_to_user_mode_l
> > >  		tick_nohz_user_enter_prepare();
> > >  
> > >  		ti_work = READ_ONCE(current_thread_info()->flags);
> > > +		tsk_isol_work = task_isol_has_work();
> > 
> > Shouldn't it be a TIF_FLAG part of EXIT_TO_USER_MODE_WORK instead?
> > 
> > Thanks.
> 
> static inline int task_isol_has_work(void)
> {
>        int cpu, ret;
>        struct isol_info *i;
> 
>        if (likely(current->task_isol_info == NULL))
>                return 0;
> 
>        i = current->task_isol_info;
>        if (i->active_mask != ISOL_F_QUIESCE)
>                return 0;
> 
>        if (!(i->quiesce_mask & ISOL_F_QUIESCE_VMSTATS))
>                return 0;
> 
>        cpu = get_cpu();
>        ret = per_cpu(vmstat_dirty, cpu);
>        put_cpu();
> 
>        return ret;
> }
> 
> Well, whether its necessary to call task_isol_exit_to_user_mode depends
> on the state of the enabled/disabled masks _and_ vmstat dirty bit
> information.
> 
> It seems awkward, to me, to condense all that information in a single bit.
> 
> Addressed all other comments, thanks.

You're unconditionally adding overhead to the syscall fastpath when it would
be so easy to set a TIF_FLAG as long as (current->task_isol_info->quiesce_mask
!= 0). vmstat_dirty can be checked afterward.

I suspect your patchset will sell much better if you join the common slowpath
single condition in EXIT_TO_USER_MODE_WORK.

Thanks.