| 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
| ||
|
Message-ID: <1e07785a-fbc0-400e-8b82-68ed110d7a93@intel.com>
Date: Fri, 25 Jul 2025 16:51:47 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghuay@...dia.com>, "Maciej
Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Peter Newman
<peternewman@...gle.com>, James Morse <james.morse@....com>, Babu Moger
<babu.moger@....com>, Drew Fustini <dfustini@...libre.com>, Dave Martin
<Dave.Martin@....com>, Chen Yu <yu.c.chen@...el.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
<patches@...ts.linux.dev>
Subject: Re: [PATCH v7 26/31] fs/resctrl: Fix life-cycle of
closid_num_dirty_rmid
Hi Tony,
"Fix" is a loaded word to use in a patch subject and its use cannot be
justified for a non-functional change such as this.
Do not make false claims of code being broken as patch motivation.
On 7/11/25 4:53 PM, Tony Luck wrote:
> closid_num_dirty_rmid is specific to the L3 resource, but it
> is allocated/freed in the more generic dom_data_{init,exit}().
Quite bold make this argument when "the more generic
dom_data_{init,exit}()" is only called with L3 resource as argument.
This is a very straight forward change but the description so far
totally obfuscates this.
This patch does two things:
a) Rename resctrl_mon_resource_init()/resctrl_mon_resource_exit() to
resctrl_mon_l3_resource_init()/resctrl_mon_l3_resource_exit()
respectively. As mentioned earlier, this can be done as part of
earlier patch that does the renaming.
b) Separate closid_num_dirty_rmid and rmid_ptrs[] allocation done in
dom_data_init() in preparation for rmid_ptrs[] to be allocated on
resctrl mount in support of the new telemetry events.
>
> Add helpers to allocate/free closid_num_dirty_rmid.
>
> Rename resctrl_mon_resource_init() to resctrl_mon_l3_resource_init()
> and call the closid_num_dirty_rmid_init() here, instead of
> allocating in dom_data_init().
>
> Making matching changes to the exit path by renaming
> resctrl_mon_resource_exit() to resctrl_mon_l3_resource_exit()
> and free closid_num_dirty_rmid here instead of in dom_data_exit().
>
> Suggested-by: Reinette Chatre <reinette.chatre@...el.com>
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
> fs/resctrl/internal.h | 6 ++--
> fs/resctrl/monitor.c | 69 ++++++++++++++++++++++++-------------------
> fs/resctrl/rdtgroup.c | 12 ++++----
> 3 files changed, 48 insertions(+), 39 deletions(-)
>
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 56fdccb39375..28d505efdb7c 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -358,7 +358,9 @@ int alloc_rmid(u32 closid);
>
> void free_rmid(u32 closid, u32 rmid);
>
> -void resctrl_mon_resource_exit(void);
> +int resctrl_mon_l3_resource_init(void);
> +
> +void resctrl_mon_l3_resource_exit(void);
>
> void mon_event_count(void *info);
>
> @@ -368,8 +370,6 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> struct rdt_domain_hdr *hdr, struct rdtgroup *rdtgrp,
> cpumask_t *cpumask, struct mon_evt *evt, int first);
>
> -int resctrl_mon_resource_init(void);
> -
> void mbm_setup_overflow_handler(struct rdt_l3_mon_domain *dom,
> unsigned long delay_ms,
> int exclude_cpu);
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 92798e1fb5b0..e3eceba70713 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -86,6 +86,37 @@ unsigned int resctrl_rmid_realloc_threshold;
> */
> unsigned int resctrl_rmid_realloc_limit;
>
> +static int closid_num_dirty_rmid_init(struct rdt_resource *r)
It is not clear to me that these new helpers are needed. To me it seems
easier to follow if they are just open coded.
> +{
> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
> + !closid_num_dirty_rmid) {
> + u32 num_closid = resctrl_arch_get_num_closid(r);
> + u32 *tmp;
> +
> + /*
> + * If the architecture hasn't provided a sanitised value here,
> + * this may result in larger arrays than necessary. Resctrl will
> + * use a smaller system wide value based on the resources in
> + * use.
> + */
> + tmp = kcalloc(num_closid, sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + closid_num_dirty_rmid = tmp;
> + }
> +
> + return 0;
> +}
> +
> +static void closid_num_dirty_rmid_exit(void)
> +{
> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> + kfree(closid_num_dirty_rmid);
> + closid_num_dirty_rmid = NULL;
> + }
> +}
> +
> /*
> * x86 and arm64 differ in their handling of monitoring.
> * x86's RMID are independent numbers, there is only one source of traffic
...
> @@ -938,7 +942,7 @@ bool resctrl_is_mon_event_enabled(enum resctrl_event_id eventid)
> }
>
> /**
> - * resctrl_mon_resource_init() - Initialise global monitoring structures.
> + * resctrl_mon_l3_resource_init() - Initialise global monitoring structures.
> *
> * Allocate and initialise global monitor resources that do not belong to a
> * specific domain. i.e. the rmid_ptrs[] used for the limbo and free lists.
> @@ -949,7 +953,7 @@ bool resctrl_is_mon_event_enabled(enum resctrl_event_id eventid)
> *
> * Returns 0 for success, or -ENOMEM.
> */
> -int resctrl_mon_resource_init(void)
> +int resctrl_mon_l3_resource_init(void)
> {
> struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> int ret;
> @@ -957,6 +961,10 @@ int resctrl_mon_resource_init(void)
> if (!r->mon_capable)
> return 0;
>
> + ret = closid_num_dirty_rmid_init(r);
> + if (ret)
> + return ret;
> +
> ret = dom_data_init(r);
> if (ret)
Leaking closid_num_dirty_rmid here?
> return ret;
Reinette
Powered by blists - more mailing lists