linux-kernel - Re: [RFD] resctrl: reassigning a running container's CTRL

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: <317b4a96-f28d-aab5-57cc-f0222b7e4901@intel.com>
Date:   Wed, 26 Oct 2022 14:12:28 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Peter Newman <peternewman@...gle.com>,
        James Morse <james.morse@....com>
CC:     Tony Luck <tony.luck@...el.com>,
        "Yu, Fenghua" <fenghua.yu@...el.com>,
        "Eranian, Stephane" <eranian@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Babu Moger" <Babu.Moger@....com>,
        Gaurang Upasani <gupasani@...gle.com>
Subject: Re: [RFD] resctrl: reassigning a running container's CTRL_MON group

Hi Peter,

On 10/26/2022 1:52 AM, Peter Newman wrote:
> On Tue, Oct 25, 2022 at 5:55 PM James Morse <james.morse@....com> wrote:
>> On 21/10/2022 13:42, Peter Newman wrote:
>>> Even on x86, without an smp_mb(), the stores to t->closid and t->rmid could be
>>> reordered with the task_curr(t) and task_cpu(t) reads which follow. The original
>>> description of this scenario seemed to assume that accesses below would happen
>>> in program order:
>>>
>>>     WRITE_ONCE(t->closid, to->closid);
>>>     WRITE_ONCE(t->rmid, to->mon.rmid);
>>>
>>>     /*
>>>      * If the task is on a CPU, set the CPU in the mask.
>>>      * The detection is inaccurate as tasks might move or
>>>      * schedule before the smp function call takes place.
>>>      * In such a case the function call is pointless, but
>>>      * there is no other side effect.
>>>      */
>>>     if (IS_ENABLED(CONFIG_SMP) && mask && task_curr(t))
>>>          cpumask_set_cpu(task_cpu(t), mask);
>>>
>>> If the task concurrently switches in on another CPU, the code above may not
>>> observed that it's running, and the CPU running the task may not have observed
>>> the updated rmid and closid yet, so it could continue with the old rmid/closid
>>> and not get interrupted.
>>
>> Makes sense to me - do you want to send a patch to fix it?
> 
> Sure, when I think of a solution. For an smp_mb() to be effective above,
> we would need to execute another smp_mb() unconditionally before reading
> the closid/rmid fields when switching a task in.
> 
> The only quick fix I know will work without badly hurting context switch
> time would be to go back to pinging all CPUs following a mass
> task-movement operation.
> 
> I'll see if I can come up with anything better, though.
> 

The original concern is "the stores to t->closid and t->rmid could be
reordered with the task_curr(t) and task_cpu(t) reads which follow". I can see
that issue. Have you considered using the compiler barrier, barrier(), instead?
>From what I understand it will prevent the compiler from moving the memory accesses.
This is what is currently done in __rdtgroup_move_task() and could be done here also?

Reinette