linux-kernel - Re: [PATCH] fs/resctrl: Restore the missing rdt_last_cmd

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: <44a4f211-6723-4fde-822c-d739fa2d603d@intel.com>
Date: Thu, 29 May 2025 15:01:40 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Zeng Heng <zengheng4@...wei.com>, <Dave.Martin@....com>, <bp@...e.de>,
	<tony.luck@...el.com>, <james.morse@....com>, <xiaochen.shen@...el.com>,
	<fenghua.yu@...el.com>
CC: <bobo.shaobowang@...wei.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] fs/resctrl: Restore the missing rdt_last_cmd_clear()

Hi Zeng Heng,

Thank you very much for catching this and providing a fix.

On 5/29/25 4:33 AM, Zeng Heng wrote:
> The fixes tag patch resolves the lockdep warning. However, directly
> removing rdt_last_cmd_clear() would leave the last_cmd_status interface
> with stale logs, which does not conform to the functional definition before
> the fix. Therefore, the rdt_last_cmd_clear() operation is performed after
> successfully acquiring the rdtgroup_mutex.

I would like to suggest some rework to changelog to meet requirements from
Documentation/process/maintainer-tip.rst. Specifically the rules about
imperative tone and structure of the changelog. Below attempts to address
those requirements but please feel free to rework after you considered the
rules yourself:

	A lockdep fix removed two rdt_last_cmd_clear() calls that were used
	to clear the last_cmd_status buffer but called without holding the 
	required rdtgroup_mutex. The impacted resctrl commands are:
	writing to the cpus or cpus_list files and creating a new monitor
	or control group. With stale data in the last_cmd_status buffer the
	impacted resctrl commands report the stale error on success, or append
	its own failure message to the stale error on failure.

	Restore the rdt_last_cmd_clear() calls after acquiring rdtgroup_mutex.

> 
> Fixes: c8eafe149530 ("x86/resctrl: Fix potential lockdep warning")
> Signed-off-by: Zeng Heng <zengheng4@...wei.com>
> ---
>  fs/resctrl/rdtgroup.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index cc37f58b47dd..4aae9eb74215 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -536,6 +536,8 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
>  		goto unlock;
>  	}
>  
> +	rdt_last_cmd_clear();
> +
>  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
>  	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>  		ret = -EINVAL;
> @@ -3481,6 +3483,8 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>  		goto out_unlock;
>  	}
>  
> +	rdt_last_cmd_clear();
> +

Could you please move this to be right after acquiring the mutex? I think clearing
last_cmd_status at beginning of a resctrl command's work is a good pattern to follow.
Thus a change like:

---8<---
diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
index 7410321d01ff..77d08229d855 100644
--- a/fs/resctrl/rdtgroup.c
+++ b/fs/resctrl/rdtgroup.c
@@ -3474,6 +3474,8 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 		goto out_unlock;
 	}
 
+	rdt_last_cmd_clear();
+
 	/*
 	 * Check that the parent directory for a monitor group is a "mon_groups"
 	 * directory.
@@ -3483,8 +3485,6 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 		goto out_unlock;
 	}
 
-	rdt_last_cmd_clear();
-
 	if (rtype == RDTMON_GROUP &&
 	    (prdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP ||
 	     prdtgrp->mode == RDT_MODE_PSEUDO_LOCKED)) {
---8<---

>  	if (rtype == RDTMON_GROUP &&
>  	    (prdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP ||
>  	     prdtgrp->mode == RDT_MODE_PSEUDO_LOCKED)) {

Thank you very much.

Reinette