Received-SPF: pass (google.com: domain of linux-kernel+bounces-215533-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <1e3bba2e-5cf6-4a77-b92d-5c7ab1661d17@intel.com>
Date: Fri, 14 Jun 2024 15:47:58 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] x86/resctrl: Don't try to free nonexistent RMIDs
To: Dave Martin <Dave.Martin@arm.com>, <x86@kernel.org>,
	<linux-kernel@vger.kernel.org>
CC: James Morse <james.morse@arm.com>, Fenghua Yu <fenghua.yu@intel.com>,
	Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>, H Peter Anvin <hpa@zytor.com>, Babu Moger
	<Babu.Moger@amd.com>, <shameerali.kolothum.thodi@huawei.com>, "D Scott
 Phillips OS" <scott@os.amperecomputing.com>, <carl@os.amperecomputing.com>,
	<lcherian@marvell.com>, <bobo.shaobowang@huawei.com>, "Shaopeng Tan
 (Fujitsu)" <tan.shaopeng@fujitsu.com>, <baolin.wang@linux.alibaba.com>,
	"Jamie Iles" <quic_jiles@quicinc.com>, Xin Hao <xhao@linux.alibaba.com>,
	Peter Newman <peternewman@google.com>, <dfustini@baylibre.com>,
	<amitsinght@marvell.com>, David Hildenbrand <david@redhat.com>, Rex Nie
	<rex.nie@jaguarmicro.com>
References: <20240614160843.11006-1-Dave.Martin@arm.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240614160843.11006-1-Dave.Martin@arm.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?REsyckwrM250WDFhV3ZPQml3MzBXcXMzNkJ3bWFjRkVMSmp1U2pSc09YT2Iy?=
 =?utf-8?B?OU9Rd0tQR1h2a0duOXRVeXI1a0UrbjdqcGUvRW9vemtpUnJtZ2RGSFJIblEx?=
 =?utf-8?B?VnI1cEpGYTRJY2k3OHdJYzJVbVRGcnhvRStHZWgwWDM4VEpXaVhPaDhha0FO?=
 =?utf-8?B?aS9zWFcxYzRpejMxN1hYZis0MDlVdHBJOEJvMHQ3T2FqZ3NYYTV4UEdlZ2kr?=
 =?utf-8?B?RlpwbmFaenVDYTg3UDdUQUZ3ZmZXSTUvWWg5ekRDTEJhRzB4Z0h0RXlJakxO?=
 =?utf-8?B?L2JVbHhaY0t2d3hTdmF4K3J3Q01NaXQxbmJoZ2c0YUwxZlBidkJGRWZZTmEr?=
 =?utf-8?B?L0p5VnpUNzJyWHBNVk9wbW5rSTQzYUVZMkNIbGpqUDdzTStud2hpaUVhT08r?=
 =?utf-8?B?eENCQ3hOaHo3eTFCb3FWd3R6dDhMVzJrOS9iU2I0c2xFNE1MRmVHQWF3UlVn?=
 =?utf-8?B?clRDSzVoUTdicnNqZGlxNWNOMlJ6YWFqUWZIVC93bXNyQldrSkNnZmlHbVpI?=
 =?utf-8?B?MDVrQnQyOStkNW5hVmlrK1R2R0FmaGg1UHBESHo4TzFkVkkySWkzREFLQ0lY?=
 =?utf-8?B?ZEpIbnpjRTJ2RDhBUEZZcjZnZWUyZ3FDMjQrTmhRVGJMeERrWG1VWEovd3pX?=
 =?utf-8?B?VGdKdkFkZ0tUNjZPVHYvWlFkUmtjRDJGdlg2SmNnbFc4M0tSaTd3VnVESDlh?=
 =?utf-8?B?aGM0VVpSKzBSVTFBM0xKVHh0OHVhWnNOWUVzZXVNTlpiNGtDZkpDcUJCZ3lK?=
 =?utf-8?B?N25qSWw0Wm9vOTVRaGh6RlhtUW5xUmJrVHZJSDd5RGw2RnZtZ2djR0pCWkpD?=
 =?utf-8?B?dThPdmtSVERoRHZiQXRKR2ZEWVNheEVKVXJzby9CTm9xVHZVT0xZNWFpRVVE?=
 =?utf-8?B?eGNPRkY2ZDNxcStBQVNDVXB4V2tocS9uVVpHY0RjNnd3Z01rZXVHazdreEk2?=
 =?utf-8?B?bWVHSHkzbHRIRVc0VzlpZktrUUg2ODZ5MDAyQ2dKZTI5Vm5aWVJUaG1ickMz?=
 =?utf-8?B?cTU3ZUgyT3FIK3hCNzJxWHdnQ1IwK0pkcDVLU3JPUGk2YU9mdUZDS21Xekps?=
 =?utf-8?B?RHNiVWdUK1llY0xwZWp1bk44QWN3cGErR3U2eVgybnR3a1lVYlk2ZXRKb0tv?=
 =?utf-8?B?U1dQM3o0eFlkaXBacTE1UUlGanRZZnYxMEhqaUtOT0w1YjJGeEhaY1IrTVhs?=
 =?utf-8?B?cUVmZktBd1NrWDNBb0huZ3MvZU1qUHg3L1ByMkQ1NVR1T0ozbnJKQUNRTTVn?=
 =?utf-8?B?dHF1V283YThmempsMk41QThKdGN0QmNJU1I4YTVDcCtXM2djVXhqZ3E1aFI5?=
 =?utf-8?B?djlpM1QxTlY5NWV4L0RUWHN5YjJCdEs2T0hGemxsMVBXdm1rSmZSQ044blQy?=
 =?utf-8?B?WW1aSWV3N0czUldRam5CbDJTYXMyUUthSXdlMEVRK05neEUrcTVMblBGSndz?=
 =?utf-8?B?VEN5K0J2VHFSVlVxc1ZYSFQ5bmJGWVlZTisrblFlM2FJWnR0QzJDVlJpYTRU?=
 =?utf-8?B?elNxWVRqb2dOSTN1MFNkOHFaTkxJdVRUZnBPWWtsdmNkU0dGM1JqemkvL0Nt?=
 =?utf-8?B?bksyNlMwQXRjeUlZTmFINERsZGc2aDdJQlV4VEd1UGVYWjR4NSsrWGMxWVJ2?=
 =?utf-8?B?K0lLdHFPVHhhcGo0TjZ4Sm5MYmpmV08vQnhUK1UzZGJ0M2NhRkE0am44U0J3?=
 =?utf-8?B?ZS9EM042NFpkUmJBUzBaalk1eTkvdTZSNmIzcnlvNDRucHNlT0VzZU9lcHFW?=
 =?utf-8?B?alhqMUEvRHQwZERRWmlVYWdaRjNYUTJqdkZkNUw0aVRvSWEwUGNVTDU0c2lx?=
 =?utf-8?B?RXU5eDd3Q1pTdkJjTk9GS3piUk91d0w4d1FKWlo4d1Z2RjJuT1hsWGJ2NjNp?=
 =?utf-8?B?RU10UytMZGY1QXpEL0NLdGQreU83MTNaeG9nVnZqVEVEYTdZbGRsQlJKbkM4?=
 =?utf-8?B?MjBQbXlJaVhBMjJvQUE4Z1dFSVZjWWZETmJIMzNacHVaZVZsTzhnUGRIWWdU?=
 =?utf-8?B?amlGUmRLVTdGM2pyaU5OdWpkS2kzVzE3cUFGeG04OGUwWVY1NW84SnNKZHI3?=
 =?utf-8?B?WUZNTWxYeHNtMDBDVGVSRVRyMU53K2t4MXphWWRNemN6YVU4MzZMMkxRUWN5?=
 =?utf-8?B?ajJTaUtsbUVlMVdCOFRpM1hHTUpnSHBFbGxtTTZRMFdQdHZjQUNIMFJ3bUpm?=
 =?utf-8?B?MEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 34f6cc7a-871a-456d-a5e7-08dc8cc40b3c
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 22:48:01.0886
 (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: MtbQu4UxcOAL28JFDuYUE5Pb5lWDS7dmm69pMAsGYeQ7L0FxQ5/xcxYwCP/TnGZUUqUBbFBBWvqxzk9GYYmy8G8XdlP+Z46eglR5eXKgG+8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB6339
X-OriginatorOrg: intel.com

Hi Dave,

On 6/14/24 9:08 AM, Dave Martin wrote:
> Commit 6791e0ea3071 ("x86/resctrl: Access per-rmid structures by
> index") adds logic to map individual monitoring groups into a
> global index space used for tracking allocated RMIDs.
> 
> That patch keept the logic to ignore requests to free the default

keept -> kept

nitpick: I actually do not know if "that patch" gets same hate as
"this patch" so to avoid any potential feedback about this I'd like
to suggest that this is rewritten without this term. Perhaps
something like: "Requests to free the default RMID in free_rmid()
are ignored, and this works fine on x86."

> RMID in free_rmid(), and this works fine on x86.
> 
> With arm64 MPAM, there is a latent bug here: on platforms with no
> monitors exposed through resctrl, each control group still gets a
> different monitoring group ID as seen by the hardware, since the
> CLOSID always forms part of the monitoring group ID.  This means
> that when removing a control group, the code may try to free this
> group's default monitoring group RMID for real.  If there are no
> monitors however, the RMID tracking table rmid_ptrs[] would be a
> waste of memory and is never allocated, leading to a splat when a
> free_rmid() tries to dereference the table.
> 
> One option would be to treat RMID 0 as special for every CLOSID,
> but this would be ugly since we still want to do bookkeeping for
> these monitoring group IDs when there are monitors present in the
> hardware.
> 
> Instead, add a gating check of resctrl_arch_mon_capable() in
> free_rmid(), and just do nothing if the hardware doesn't have
> monitors.
> 
> This fix mirrors the gating checks already present in
> mkdir_rdt_prepare_rmid_alloc() and elsewhere.
> 
> No functional change on x86.
> 
> Fixes: 6791e0ea3071 ("x86/resctrl: Access per-rmid structures by index")
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> 
> ---
> 
> Based on v6.10-rc3.
> 
> Tested on x86 (But so far for the monitors-present case.

Tested by booting with "rdt=!cmt,!mbmtotal,!mbmlocal".

> Testing on Atom would be appreciated.)
> 
> Tested on arm64 for the no-monitors case.
> ---
>   arch/x86/kernel/cpu/resctrl/monitor.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 2345e6836593..366f496ca3ce 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -519,7 +519,8 @@ void free_rmid(u32 closid, u32 rmid)
>   	 * allows architectures that ignore the closid parameter to avoid an
>   	 * unnecessary check.
>   	 */
> -	if (idx == resctrl_arch_rmid_idx_encode(RESCTRL_RESERVED_CLOSID,
> +	if (!resctrl_arch_mon_capable() ||
> +	    idx == resctrl_arch_rmid_idx_encode(RESCTRL_RESERVED_CLOSID,
>   						RESCTRL_RESERVED_RMID))
>   		return;
>   
> 

Thank you for catching this. This reveals how subtle resctrl is by
calling free_rmid() in paths for convenience and then relying on an
uninitialized variable for correct behavior.

This replaces this subtle assumption with an actual check.

With changelog comments addressed:
| Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette