linux-kernel - Re: [PATCH v3 2/2] x86/resctrl: update task closid/rmid with task_call

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:   Tue, 22 Nov 2022 13:13:28 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Peter Newman <peternewman@...gle.com>
CC:     <fenghua.yu@...el.com>, <bp@...en8.de>, <derkling@...gle.com>,
        <eranian@...gle.com>, <hpa@...or.com>, <james.morse@....com>,
        <jannh@...gle.com>, <kpsingh@...gle.com>,
        <linux-kernel@...r.kernel.org>, <mingo@...hat.com>,
        <tglx@...utronix.de>, <x86@...nel.org>
Subject: Re: [PATCH v3 2/2] x86/resctrl: update task closid/rmid with
 task_call_func()

Hi Peter,

On 11/22/2022 7:17 AM, Peter Newman wrote:
> Hi Reinette,
> 
> On Mon, Nov 21, 2022 at 10:59 PM Reinette Chatre
> <reinette.chatre@...el.com> wrote:
>> Patch description in subject should start with upper case.
>>
>> On 11/15/2022 6:19 AM, Peter Newman wrote:
>>> When determining whether running tasks need to be interrupted due to a
>>> closid/rmid change, it was possible for the task in question to migrate
>>> or wake up concurrently without observing the updated values.
>>
>> Mixing tenses can quickly become confusing. Please stick to imperative tone.
> 
> Sorry about this. Looking at how other bug fix commit messages are
> worded, they describe the bug as the present behavior of the kernel
> rather than how the kernel used to behave before the patch was applied.
> It seems I've been consistently wording problem statements as past
> behavior.

No problem. There is a good section in the tip handbook about changelogs. 

>>> +static void update_task_closid_rmid(struct task_struct *t,
>>> +                                 struct rdtgroup *rdtgrp)
>>> +{
>>> +     /*
>>> +      * Serialize the closid and rmid update with context switch. If this
>>> +      * function indicates that the task was running, then it needs to be
>>
>> What does "this function" refer to? Please replace with function name to be
>> specific since there are a few functions below.
> 
> Ok.
> 
>>
>> /was running/is running/?
> 
> The task is no longer locked here, so it doesn't seem correct to say "is
> running" when it could have already stopped running.

ok, how about "was running during its closid and rmid change" or similar?

> 
> Also, maybe related, I was considering moving the task_curr() call out
> of update_locked_task_closid_rmid() to avoid giving the misconception
> that the result is current, because the value is used after releasing
> the task's pi/rq lock.

I think task_curr() should stay within update_locked_task_closid_rmid(). It
is an appropriate usage per description of task_call_func() and from what I
understand it is the most accurate way to answer the question "was task
running while its closid/rmid changed?". It cannot be guaranteed that the
task is still running when the MSR changing code gets its change to run but
_update_task_closid_rmid() as well as resctrl_sched_in() have that covered.

Reinette