linux-kernel - Re: [PATCH v15 32/34] fs/resctrl: Disable BMEC event configuration when mbm

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: <e10bfec5-dfbf-4008-aedb-c47f6596e712@intel.com>
Date: Thu, 17 Jul 2025 21:02:34 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....com>, <corbet@....net>, <tony.luck@...el.com>,
	<james.morse@....com>, <tglx@...utronix.de>, <mingo@...hat.com>,
	<bp@...en8.de>, <dave.hansen@...ux.intel.com>
CC: <Dave.Martin@....com>, <x86@...nel.org>, <hpa@...or.com>,
	<akpm@...ux-foundation.org>, <paulmck@...nel.org>, <rostedt@...dmis.org>,
	<Neeraj.Upadhyay@....com>, <david@...hat.com>, <arnd@...db.de>,
	<fvdl@...gle.com>, <seanjc@...gle.com>, <jpoimboe@...nel.org>,
	<pawan.kumar.gupta@...ux.intel.com>, <xin@...or.com>,
	<manali.shukla@....com>, <tao1.su@...ux.intel.com>, <sohil.mehta@...el.com>,
	<kai.huang@...el.com>, <xiaoyao.li@...el.com>, <peterz@...radead.org>,
	<xin3.li@...el.com>, <kan.liang@...ux.intel.com>,
	<mario.limonciello@....com>, <thomas.lendacky@....com>, <perry.yuan@....com>,
	<gautham.shenoy@....com>, <chang.seok.bae@...el.com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<peternewman@...gle.com>, <eranian@...gle.com>
Subject: Re: [PATCH v15 32/34] fs/resctrl: Disable BMEC event configuration
 when mbm_event mode is enabled

Hi Babu,

On 7/8/25 3:17 PM, Babu Moger wrote:
>  
> +/*
> + * resctrl_bmec_files_show() — Controls the visibility of BMEC-related resctrl
> + * files. When @show is true, the files are displayed; when false, the files
> + * are hidden.
> + */
> +static void resctrl_bmec_files_show(struct rdt_resource *r, bool show)

The "void" return is unexpected since many of the calls can fail. It looks to me
that this is indeed intentional since there is no BMEC feature checking, but instead
the existence of the resctrl files are implicitly used as feature check. Doing so enables
this code to be called on any system whether BMEC is supported or not and thus a failure
should not be considered an actual failure. If this is the case, then it is subtle so please
add this information to the function's comments. If this is not the case, could you
please explain the void return?

> +{
> +	struct kernfs_node *kn_config, *l3_mon_kn;
> +	char name[32];
> +
> +	sprintf(name, "%s_MON", r->name);
> +	l3_mon_kn = kernfs_find_and_get(kn_info, name);
> +	if (!l3_mon_kn)
> +		return;
> +
> +	kn_config = kernfs_find_and_get(l3_mon_kn, "mbm_total_bytes_config");
> +	if (kn_config) {
> +		kernfs_show(kn_config, show);
> +		kernfs_put(kn_config);
> +	}
> +
> +	kn_config = kernfs_find_and_get(l3_mon_kn, "mbm_local_bytes_config");
> +	if (kn_config) {
> +		kernfs_show(kn_config, show);
> +		kernfs_put(kn_config);
> +	}
> +
> +	kernfs_put(l3_mon_kn);
> +}
> +
>  static int resctrl_mbm_assign_mode_show(struct kernfs_open_file *of,
>  					struct seq_file *s, void *v)
>  {
> @@ -2659,6 +2690,12 @@ static int rdtgroup_mkdir_info_resdir(void *priv, char *name,
>  			ret = resctrl_mkdir_event_configs(r, kn_subdir);
>  			if (ret)
>  				return ret;
> +			/*
> +			 * Hide BMEC related files if mbm_event mode
> +			 * is enabled.
> +			 */
> +			if (resctrl_arch_mbm_cntr_assign_enabled(r))
> +				resctrl_bmec_files_show(r, false);

Looks like the kernfs_find_and_get(kn_info, name) in resctrl_bmec_files_show()
can be avoided by providing the kn as parameter. I think you may be doing it like
this because of how resctrl_bmec_files_show() is used in resctrl_mbm_assign_mode_write()
in the next patch. I think the kn can still be provided as parameter but 
resctrl_mbm_assign_mode_write() will set it to NULL and only then does 
resctrl_bmec_files_show() need to figure it out itself.

>  		}
>  	}
>  

Reinette