Re: [PATCH v9 01/12] platform/x86/intel/{pmt,vsec}: Prevent unbind via sysfs

["Luck, Tony" <tony.luck@intel.com>]

On Wed, Jul 08, 2026 at 03:45:03PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 7/1/26 2:35 PM, Tony Luck wrote:
> > The resctrl AET code uses intel_pmt_get_regions_by_feature() (exported by the
> > pmt_telemetry module) to obtain pmt_feature_group instances that are populated
> > using data from the pmt_discovery module. The returned telemetry_region::addr
> > values are MMIO mappings created in the telemetry driver's probe path,
> > and resctrl reads from them while a mount is active.
> > 
> > However, a user can unbind the intel_pmt driver or its parent intel_vsec
> > from their devices via sysfs "unbind" attributes. Unbinding either of these
> > drivers would tear down the MMIO mappings, leaving resctrl with stale pointers
> > and causing a fault on the next access.
> > 
> > It is safe for the discovery driver to be unbound. It does all its work at
> > initial probe time and saves a copy of the features and RMID count.
> > 
> > Set device_driver::suppress_bind_attrs in each driver so that the "bind"
> > and "unbind" sysfs attributes are not exposed and the user cannot detach
> > the driver from its devices while resctrl (or any other in-kernel consumer)
> > may be using the resources.
> 
> Unbinding via sysfs does not seem to be the only way in which resources that
> resctrl depend on could disappear.  The "/sys/bus/pci/devices/.../remove"
> interface looks to be another way? I am not aware of a way to prevent this since
> this represents how devices could just be yanked from a system. I wonder if
> resctrl should instead protect itself against telemetry resources disappearing
> after mount with another helper that telemetry driver can call when the device
> is being removed or the module is being unloaded. When telemetry driver informs AET
> that resources are going away after resctrl is mounted then resctrl/AET can return
> EIO on event read attempts? resctrl could possibly recover when the device/module
> re-appears but if that is complicated then user space could just be forced to remount
> resctrl.
> 
> With this there is no need to prevent unbind here since it creates a false
> guarantee to resctrl.

This sounds like a complexity road that I'd prefer not to travel. The AET
enumeration is handled with a contract between resctrl and the pmt_telemetry
module. But the mapping of MMIO space is handled by the parent intel_vsec
driver. So to get notifications of impending unmap of MMIO space the intel_vsec
would somehow need to be aware that pmt_telemetry had handed out some pointers.

The "remove" interface might push this trail up additional levels (to the
PCIe bus driver level?).

All this complexity to avoid crashing when system operators poke at sysfs
files without understanding dependencies.

I propose a simpler solution. Just have the AET code defensively read from
the MMIO registers with a new function that turns any page fault due to the
MMIO space being unmapped into an error return.

I'm terrible as inline ASM code. So I had Gemini write this for me. I tested it
by removing the code that disables unbind, and then unbinding the intel_vsec
device for socket 0 on my system. All the AET files for socket 0 reported
"unavailable", the socket 1 files continued to work normally.

------------------------------
/**
 * readq_safe - Safely read a 64-bit MMIO register, catching page faults.
 * @ptr: The MMIO address to read from.
 * @val: Pointer to where the fetched value should be stored.
 *
 * Returns 0 on success, or -EFAULT if a page fault occurred.
 */
static int readq_safe(const volatile u64 __iomem *ptr, u64 *val)
{
        int err = 0;
        u64 ret_val;

        asm volatile (
                "1: movq %[addr], %[val]\n"
                _ASM_EXTABLE(1b, 2f)
                "   jmp 3f\n"
                "2: mov %[efault], %[err]\n"
                "   xorq %[val], %[val]\n"
                "3:\n"
                : [val] "=r" (ret_val), [err] "+r" (err)
                : [addr] "m" (*ptr), [efault] "i" (-EFAULT)
                : "memory"
        );

        *val = err ? 0 : ret_val;

        return err;
}
------------------------------
> 
> > 
> > Fixes: 1fb2daa60de6 ("x86/resctrl: Discover hardware telemetry events")
> > Assisted-by: Claude:Opus-4.7
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > --
> 
> Please note the separator typo ("--" -> "---") above that exists in every patch in
> this series that has a changelog. Please check all patches for this, I only highlight this
> instance.

Oops. I was using those so keep my change list intact when updating,
shuffling, re-basing patches. I forgot to tweak the "--" back to "---"
when posting.
> 
> Reinette
> 
-Tony