Message-ID: <8f2b606c-945b-1c26-9bc5-29f5b89227f7@intel.com>
Date:   Fri, 3 Mar 2023 11:05:45 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.7.2
Subject: Re: [PATCH v3 3/3] x86/resctrl: Implement rename op for mon groups
Content-Language: en-US
To:     Peter Newman <peternewman@google.com>
CC:     <fenghua.yu@intel.com>, <Babu.Moger@amd.com>, <bp@alien8.de>,
        <dave.hansen@linux.intel.com>, <eranian@google.com>,
        <gupasani@google.com>, <hpa@zytor.com>, <james.morse@arm.com>,
        <linux-kernel@vger.kernel.org>, <mingo@redhat.com>,
        <skodak@google.com>, <tony.luck@intel.com>, <tglx@linutronix.de>,
        <x86@kernel.org>
References: <20230125101334.1069060-1-peternewman@google.com>
 <20230125101334.1069060-4-peternewman@google.com>
 <b14598a8-2ab1-eda2-e1fb-a5f568e869d5@intel.com>
 <CALPaoCgnP0zU4fkDxst9pyY1FrxW2zT-VfMTnJbj_iytQxknjQ@mail.gmail.com>
 <2f0abf41-87ba-0eee-03d9-ec3e5062d595@intel.com>
 <CALPaoCgZuHxfH-OEiXsCn0cxmUQygwCH5HOT0ka5OmxMX96Y8g@mail.gmail.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <CALPaoCgZuHxfH-OEiXsCn0cxmUQygwCH5HOT0ka5OmxMX96Y8g@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WktFQTRCa1NINHhpQ2NJaFZzWGkvby9ZbUVHN0luaS8wMzhoUStCOUUyUU1L?=
 =?utf-8?B?bHp4ZHF3QXpycG1ERUtTb0MrTDh6YTVrbXV3MmtWNnZZRkdtb3JoSDEwNGJp?=
 =?utf-8?B?QUIrYzd4bjZTWDRXekdzTUJQbTlndFEvV2VHS2ovWnZVTDV5cGwzOXZkdVEx?=
 =?utf-8?B?Wkt2UTdqdThLOENnVDIyaTlDVTRFNE00VnJyaTIya0o0aWNPUkFwbzdwU0lL?=
 =?utf-8?B?amtXQW9wVjNueTNwT2l0YjRzSlpxVXVjV2xzYTFLNGVQUUtWWG1mYVJFM2Yy?=
 =?utf-8?B?TS8wcDhtNERpZDVXUE0vUXYzQysxUkxMeVQrdXQrK1BNMEJRTEtVUHRrUm93?=
 =?utf-8?B?bTl1dEYrRW01dzRPRFkwU3M0TXYyVVhHd3JCSDhlVHBiR0RhRm1LWjhYSGlW?=
 =?utf-8?B?OWxaM05HSVkyNHVWb3ZqUGgxdFNYUWxiM1hCOXp4NE4ydVRRUXRqZ2F0TUZM?=
 =?utf-8?B?L2lZUGZMVGxkMVkwU2dPR2J6TUpmMkx1cUszYVF3bFZoMUxkREFZYnFYMktT?=
 =?utf-8?B?NGcyMncrd1Q1KzJlNm1hWTlNM3FMcXA3UUwzOEZhVndlNW1xbnVGbE9EelpE?=
 =?utf-8?B?dW56Wko4Zk9OTmxob2ZvYlBJU3J0cVVGeU5uYVFBbURkNGkzempoTkpoaURx?=
 =?utf-8?B?TXhmZTNLRFkwalNNUUcwMG5ZaFJGdDZsOUpwdElDWGxwdXlRUE1mMlNsbVh4?=
 =?utf-8?B?TllZdUJTNFBKUmxRNnI0clpOR3JUV3ZNaEVuWmUyZEcyeWpkV2t6ZnZkOHZa?=
 =?utf-8?B?NjYxN1h2MHJZL2dHZzZYMldtOXViZWVTS09EWjAzSWY1T21MOFpCK1VzYWsr?=
 =?utf-8?B?ekJRV2J2TDEvZ25nM3gveDhWd05hcUF0RXUyQm9UMGg5NFhNQmI1SDF1alpl?=
 =?utf-8?B?UTVhRGRkRHAxbXpyMmJqcVBTWFdSV0IrOGFRdlpjZlEzVU1WRW05Vno2Y1pB?=
 =?utf-8?B?R1AvN05MTFNwSEI3OWFPSW9HK2ZFeHBGc1pPZnZpM05vVkVWbjM2Z2hWQ29v?=
 =?utf-8?B?Nlpmc3NVaHJoUXdLeUlHWkFPYUtJdFJYeUx4eGVzOHc2OFBMTmswNDR0bkVo?=
 =?utf-8?B?cVBNTWhjZ3FFczFwbGxDemZmK3ozaWVNQlFoSG8wcE1KZCtsRGV3TEM3MG00?=
 =?utf-8?B?US80NTdvZWk1SFRIZ0tYcEJLVEVYZThVRmd0RExwVHZRY2trRVVIOVNkb0hE?=
 =?utf-8?B?WTAvdzBsWmpMNXVLMEZqOVZEV1lNZGs5cWFvd1ErMUo4V2ZZYXM5Y3JtOUJL?=
 =?utf-8?B?Vy93ckVxUml1U2wrS2MxK3J6SUhkMGJhYi8wSDk3Rkg3cm04cGs3RzN4MjRL?=
 =?utf-8?B?VFd4ZHVXWUtVL3F5TEpXcmVLMndtdUVVbE5mdTdsWDVqNGlFZ0tMM1pKZVdq?=
 =?utf-8?B?Y3RtcjhvYVFnK0FWRHAySlZ3LzlYaFY4S3ZnMytVUExrdWlZMEJOUTJHNFFn?=
 =?utf-8?B?SkN4c05NR3dMTjVyL2ZhK1NCNVpHTVJhMHlONVl5emNVdjl1bkxtbHZhTytl?=
 =?utf-8?B?ejY3VCtKakRrQ2FCaGRGdjJLb3dCYzNQZ2NQT2xoendPcmsyd2JwaWV4WERa?=
 =?utf-8?B?OTQ0T0hSV0ZNaEpTNFdDdXdJcVF4anNsUnNhbUpHczBmM0Y0aERRRGJvb1NE?=
 =?utf-8?B?NDUxTVpzOStuOFFldEM0djlTREYvMjl1K1E1M1crL1poK1JFVi9acTJuaEsy?=
 =?utf-8?B?eFRHS1c1dnNtZFJMYkcrSnE5S2lWV1poc3NUSEVpRSs1VHJoM3JwVCtUM3Vp?=
 =?utf-8?B?ZjJ3QTZOSHc5akZQbCsreklvOVVhcHU1a0F5bGF1azdxVktHMjY4STlKWjVE?=
 =?utf-8?B?OHVjZUJYVlljdjlGYzYzMFp5eXBaU2hnd3VjOG1zcXZ2SEhwR2JlYU1rbnF2?=
 =?utf-8?B?QkFrU3Y4blJBV2lEOGQ1ZUxyUWQ3VnZpdllUSUp6RkwwcTBZWjRLWTZ5by9P?=
 =?utf-8?B?ZVBpQWNIMlcwS01pM1RNZ1czaFlYK2dOdE9FTWVxTEVxdHhjZm0zUm9RTW14?=
 =?utf-8?B?V25QUktRZkdnN01vaVpnSzRKZ1pxOFhkMk16VXFscVJUNlZhZlRrNmU3R3lY?=
 =?utf-8?B?TzNQV003S21Gc1pDRXh2T2xkWGZCcElmaGxmcm9sejdsWXF6VG11ZzQ0aWRW?=
 =?utf-8?B?dWFhWjdFTWZIbDVmalN6S29wOWxYeVZacVRDazRRSTZ3aExmbnYvQ2F4Zlov?=
 =?utf-8?B?c2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5f7aa069-28f7-42d7-df51-08db1c1a4ce1
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Mar 2023 19:05:49.0311
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8QJ61vAGJIGkjEWMQw0UetiqF7pUApck3MEUuZaG3H9d7IrXs4gxl4r9Y2JornhZKq0UP7Y8MbhgKqiy3BMDvCVA6kyGfmwQNHpKbYkhF00=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7538
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Peter,

On 3/3/2023 7:10 AM, Peter Newman wrote:
> On Thu, Mar 2, 2023 at 11:27 PM Reinette Chatre
> <reinette.chatre@intel.com> wrote:
>> On 3/2/2023 6:26 AM, Peter Newman wrote:
>>> On Sat, Feb 11, 2023 at 12:21 AM Reinette Chatre
>>> <reinette.chatre@intel.com> wrote:
>>>
>>>> On 1/25/2023 2:13 AM, Peter Newman wrote:
>>>>> +     for_each_process_thread(p, t) {
>>>>> +             if (is_closid_match(t, prdtgrp) && is_rmid_match(t, rdtgrp))
>>>>> +                     rdt_move_one_task(t, new_prdtgrp->closid, t->rmid,
>>>>> +                                       cpus);
>>>>> +     }
>>>>> +     read_unlock(&tasklist_lock);
>>>>
>>>> Can rdt_move_group_tasks() be used here?
>>>
>>> As it is today, rdt_move_group_tasks() would move too many tasks.
>>> mongrp_move() needs both the CLOSID and RMID to match, while
>>> rdt_move_group_tasks() only needs 0-1 of the two to match.
>>>
>>> I tried adding more parameters to rdt_move_group_tasks() to change the
>>> move condition, but I couldn't make the resulting code not look gross
>>> and after factoring the tricky stuff into rdt_move_one_task(),
>>> rdt_move_group_tasks() didn't look interesting enough to reuse.
>>
>> Could it be made readable by adding a compare function as parameter to
>> rdt_move_group_tasks() that is used to determine if a task should be moved?
> 
> Yes, I think that would be ok in this case. That shouldn't have any
> cost if these are all static functions.
> 
> As long as we have an rdt_move_group_tasks() function, it's a liability
> to have a separate task-moving loop for someone to miss in the future.

Agreed.
 
> Should I still bother with factoring out rdt_move_one_task() in the
> parent patch? It was motivated by my creating a new task-moving loop
> in this patch.

I do not think that refactoring is necessary if you go this route.
 
>>>>> +
>>>>> +     rdtgrp = kernfs_to_rdtgroup(kn);
>>>>> +     new_prdtgrp = kernfs_to_rdtgroup(new_parent);
>>>>> +     if (!rdtgrp || !new_prdtgrp) {
>>>>> +             free_cpumask_var(tmpmask);
>>>>> +             return -EPERM;
>>>>> +     }
>>>>> +
>>>>
>>>> How robust is this against user space attempting to move files?
>>>
>>> I'm not sure I understand the question. Can you be more specific?
>>
>> This commit adds support for rename to resctrl. I thus expect this
>> function to be called when user space attempts to move _any_ of
>> the files and/or directories within resctrl. This could be out of
>> curiosity, buggy, or maliciousness. I would like to understand how
>> robust this code would be against such attempts because I do not see
>> much checking before the preparation to do the move is started.
> 
> Now I see, thanks.
> 
> kernfs_to_rdtgroup() will return the parent rdtgroup when
> kn or new_parent is a file, which will lead to kernfs_rename() moving
> a file out of a group or clobbering another file node. I'll need to
> enforce that kn and new_parent are rdtgroup directories and not file
> nodes.

Perhaps additionally that the source directories have mon_groups as parent,
similar to the destination check you already have.

Reinette