Re: [PATCH 6/6] PCI/AER: Support Advisory Non-Fatal Errors

On Fri, Jul 24, 2026 at 04:27:55PM +0000, [email protected] wrote:
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -410,6 +424,15 @@ void pci_aer_init(struct pci_dev *dev)
> >  	n = pcie_cap_has_rtctl(dev) ? 5 : 4;
> >  	pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR, sizeof(u32) * n);
> >  
> > +	/*
> > +	 * Advisory Non-Fatal Errors are masked by default (PCIe r7.0, sec
> > +	 * 7.8.4.6).
> > +	 */
> > +	if (dev->devcap & PCI_EXP_DEVCAP_RBER)
> > +		pci_clear_and_set_config_dword(dev,
> > +					       dev->aer_cap + PCI_ERR_COR_MASK,
> > +					       PCI_ERR_COR_ADV_NFAT, 0);
> 
> [Severity: High]
> Does this code modify the AER Correctable Error Mask register unconditionally
> without checking pcie_aer_is_native(dev)?

Yes, that's a valid point.

> > @@ -916,8 +997,20 @@ void aer_print_error(struct aer_err_info *info, int i)
> >  
> >  	__aer_print_error(dev, info);
> >  
> > -	if (info->tlp_header_valid)
> > +	if (info->tlp_header_valid && info->severity != AER_CORRECTABLE)
> >  		pcie_print_tlp_log(dev, &info->tlp, level, dev_fmt("  "));
> > +
> > +anfe:
> > +	/* Recursive invocation for Advisory Non-Fatal Errors */
> > +	if (info->anfe_status && info->severity == AER_CORRECTABLE) {
> > +		info->severity = AER_NONFATAL;
> > +		info->status = info->anfe_status;
> > +		info->mask = 0;
> > +
> > +		aer_print_error(info, i);
> 
> [Severity: Medium]
> Will this recursive call operate with the stale info->level and
> info->ratelimit_print[i] state?

That's intentional.  I want the (Advisory) Uncorrectable Errors to be
reported with the same loglevel and ratelimiting as the accompanying
Correctable Error.

However while native AER handling does this the way I want it to,
I missed that Firmware First does not.  It uses the loglevel and
ratelimiting of (non-Advisory) Uncorrectable Errors.  So that's
something I'll have to fix.

> [Severity: High]
> This is a pre-existing issue, but can this cause a PCI device reference leak
> if the hardware error status changes between loops?
[...]
> If the hardware or firmware clears the error status after the first loop but
> before the second loop reads it, aer_get_device_error_info() returns 0. The
> second loop will skip the handler, and the pci_dev reference is permanently
> leaked.

Hallucination.  If hardware or firmware clears error bits behind our back,
it would be broken.  

> [Severity: High]
> This is a pre-existing issue, but does this create a TOCTOU race window for
> newly arriving hardware errors?
> 
> drivers/pci/pcie/aer.c:aer_process_err_devices() {
>     ...
> 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
> 		if (aer_get_device_error_info(e_info, i))
> 			aer_print_error(e_info, i);
> 	}
> 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
> 		if (aer_get_device_error_info(e_info, i))
> 			handle_error_source(e_info->dev[i], e_info);
> 	}
>     ...
> }

It's a valid point and already on my todo list, but not something
I want to fix as part of this series.  The duplicate invocation of
aer_get_device_error_info() is not only susceptible to a TOCTOU race,
but also very inefficient.

> [Severity: Medium]
> This is a pre-existing issue, but does this code cause a memory leak of the
> dynamically allocated aer_capability_regs when the recovery queue overflows?
> 
> drivers/pci/pcie/aer.c:aer_recover_queue() {
>     ...
> 	else
> 		pr_err("buffer overflow in recovery for %04x:%02x:%02x.%x\n",
> 		       domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>     ...
> }
> 
> In the GHES subsystem, memory for struct aer_capability_regs is allocated from
> ghes_estatus_pool and passed as a pointer to aer_recover_queue(). If the
> bounded aer_recover_ring kfifo is full, it prints an overflow error and returns
> without freeing the memory.
> 
> The aer_regs pointer is abandoned and the memory is permanently leaked, as it
> is only freed on the success path inside aer_recover_work_func().

It's a valid point, I've added that to my todo list.

> [Severity: High]
> This is a pre-existing issue, but can this lead to data corruption when
> decoding concurrent Machine Check Exceptions due to an unprotected global
> buffer?
> 
> drivers/acpi/acpi_extlog.c:extlog_print() {
>     ...
> 	memcpy(elog_buf, (void *)estatus, ELOG_ENTRY_LEN);
> 	/* clear record status to enable BIOS to update it again */
> 	estatus->block_status = 0;
> 
> 	tmp = (struct acpi_hest_generic_status *)elog_buf;
>     ...
> }
> 
> The ACPI extended log driver allocates a single global buffer elog_buf. The MCE
> decoder callback unconditionally copies the new MCE event payload into this
> global buffer without any locks. 
> 
> Since Machine Check Exceptions can trigger concurrently across different CPUs,
> concurrent executions of the MCE handlers will simultaneously overwrite
> elog_buf, corrupting the data structures being parsed.

I'm not familiar with MCE handling, I'll have to ask internally whether
that's a valid finding.

Thanks,

Lukas