linux-kernel - Re: [v3 PATCH 1/1] x86/resctrl: Add "*" shorthand to set io

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] [day] [month] [year] [list]

Message-ID: <jcsimwz3gs347dplzdxfcjft7a2azigomhgnz6uule3tqyjryg@fftgowubg2pg>
Date: Sun, 25 Jan 2026 22:30:57 -0500
From: Aaron Tomlin <atomlin@...mlin.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: tony.luck@...el.com, Dave.Martin@....com, james.morse@....com, 
	babu.moger@....com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, 
	dave.hansen@...ux.intel.com, sean@...e.io, linux-kernel@...r.kernel.org
Subject: Re: [v3 PATCH 1/1] x86/resctrl: Add "*" shorthand to set io_alloc
 CBM for all domains

On Thu, Jan 08, 2026 at 10:45:11AM -0800, Reinette Chatre wrote:
> nit: the value is not "required". Compare with, for example, "An ID of
> '*' configures all domains with provided CBM"

Hi Reinette,

Agreed. Your phrasing is much clearer; I shall incorporate this change into
the documentation.

> Why is it necessary to change to a while loop? From what I can tell this
> is not required to support this particular feature and thus makes this
> patch consist of two logical changes instead of one (see "Separate your
> changes" in Documentation/process/submitting-patches.rst). Since the
> focus of this patch is to support "*" the switch to a while loop
> distracts from the core change due to generating so many line changes.
> 
> I have two concerns with a switch to a while loop:
> 
> The switch to the while loop has a subtle but significant consequence
> that changes the behavior of this flow to return "success" when the user
> provides an invalid domain ID. The caller, resctrl_io_alloc_cbm_write(),
> expects "success" to mean that the configuration has been staged and can
> be configured on hardware. Now it may be possible that hardware
> configuration attempted without a configuration staged?
> 
> Apart from the user interface change the switch to the while loop changes
> the common pattern in resctrl used for parsing similar content from user
> space. See fs/resctrl/ctrlmondata.c:parse_line(),
> fs/resctrl/monitor.c:resctrl_parse_mbm_assignment(),
> fs/resctrl/rdtgroup.c:mon_config_write(). When working with resctrl I
> prefer to use common patterns. If there is an issue with a particular
> pattern then all instances should be changed.
> 
> Please just change the code needed to support the new feature.

You are quite right. In hindsight, the refactoring distracted from the
primary objective of the patch and, as you noted, inadvertently introduced
a regression regarding invalid domain IDs. I also appreciate the point
regarding consistency with the existing resctrl parsing patterns.

I will revert to the original goto style in the next iteration to ensure
the patch remains minimal and consistent with the subsystem's conventions.

> Have you tried this feature with some edge cases? When considering, for
> example, a user just providing "*" then from this point on "dom" is
> assumed to point to a valid CBM but is actually NULL. Since this parses
> user input it is required to be robust against any input.

That is a fair point. I will ensure the parser is robust against malformed
input, such as a bare "*" without a corresponding value, to prevent any
potential null pointer dereferences.

I provided a v4 [1].

[1]: https://lore.kernel.org/lkml/20260125171752.3374930-1-atomlin@atomlin.com/


Kind regards,
-- 
Aaron Tomlin