Re: [PATCH 5/5] PCI/FLIT: Ratelimit flit error logging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Sysfs Group Directory Leak for devices without FLIT capability.
- [Medium] Missing input validation allows negative burst values, permanently suppressing flit error logs.

Pre-existing issues:
- [High] Surprise hot-unplug or malicious endpoint causes an infinite loop in `flit_isr`, leading to a hard CPU lockup.
--

--- Patch [5]: [PATCH 5/5] PCI/FLIT: Ratelimit flit error logging ---
commit 3a5e2834e7b9f5641abd9dbcf3215773fe5a6904
Author: Yazen Ghannam <yazen.ghannam@amd.com>

PCI/FLIT: Ratelimit flit error logging

This commit introduces a per-device ratelimit state for flit error logging
to prevent console log flooding. It exposes tuning parameters via per-device
sysfs attributes and applies the ratelimiter inside the interrupt handler.

> diff --git a/drivers/pci/pcie/flit.c b/drivers/pci/pcie/flit.c
> index 83cf53f392913..7cb5aff7c552c 100644
> --- a/drivers/pci/pcie/flit.c
> +++ b/drivers/pci/pcie/flit.c
[ ... ]
> +static ssize_t flit_ratelimit_burst_store(struct device *dev,
> +					  struct device_attribute *attr,
> +					  const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int burst;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (kstrtoint(buf, 0, &burst) < 0)
> +		return -EINVAL;
> +
> +	pdev->flit_ratelimit.burst = burst;

[Severity: Medium]
Does this allow negative values to be assigned to the burst limit?

If a user writes a negative string (like "-5"), kstrtoint() will parse it
successfully into a negative integer. If burst is negative, the core
___ratelimit() check (rs->burst <= rs->printed) will permanently evaluate to
true, which would silently suppress all future flit error logs.

> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(flit_ratelimit_burst);
> +
> +static struct attribute *flit_attrs[] = {
> +	&dev_attr_flit_ratelimit_interval_ms.attr,
> +	&dev_attr_flit_ratelimit_burst.attr,
> +	NULL
> +};
> +
> +static umode_t flit_attrs_are_visible(struct kobject *kobj,
> +				      struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (!pdev->flit_cap)
> +		return 0;

[Severity: Medium]
Because this attribute group has a name (".name = "flit""), does returning 0
here leak an empty sysfs directory for devices without the FLIT capability?

Returning 0 suppresses the individual attributes, but the sysfs core will
still create an empty "/sys/bus/pci/devices/.../flit/" directory. Should
this return SYSFS_GROUP_INVISIBLE instead to suppress the entire directory?

> +
> +	return a->mode;
>  }
>  
> +const struct attribute_group flit_attr_group = {
> +	.name = "flit",
> +	.attrs = flit_attrs,
> +	.is_visible = flit_attrs_are_visible,
> +};
[ ... ]
>  	do {
>  		pci_read_config_dword(pdev, flit + PCI_FLIT_ERR_LOG1, &err_log1);
>  
>  		if (!(err_log1 & PCI_FLIT_ERR_LOG_VALID))
>  			break;
>  
>  		pci_read_config_dword(pdev, flit + PCI_FLIT_ERR_LOG2, &err_log2);
> -		pci_info(pdev, HW_ERR "  Error Log1: 0x%08x Error Log2: 0x%08x\n", err_log1, err_log2);
> +		if (print)
> +			pci_info(pdev, HW_ERR "  Error Log1: 0x%08x Error Log2: 0x%08x\n",
> +				 err_log1, err_log2);
>  		trace_flit_event(pci_name(pdev), cntr_ctrl, cntr_sta, err_log1, err_log2);
>  
>  		pci_write_config_dword(pdev, flit + PCI_FLIT_ERR_LOG1, err_log1);
> -	} while (err_log1 & PCI_FLIT_ERR_LOG_MORE);
> +	} while (err_log1 & PCI_FLIT_ERR_LOG_MORE);

[Severity: High]
This is a pre-existing issue, but is this loop safe against surprise device
hot-unplugs?

If the device is disconnected, pci_read_config_dword() will fail and assign
~0 (0xFFFFFFFF) to err_log1. Because ~0 has all bits set, the initial check
for PCI_FLIT_ERR_LOG_VALID will pass.

The loop condition checking for PCI_FLIT_ERR_LOG_MORE will also pass, creating
an unconditional infinite loop that could cause a hard CPU lockup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714183950.1620016-1-yazen.ghannam@amd.com?part=5