linux-kernel - Re: [PATCH] PCI/AER: Iterate over error counters instead of error

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

Message-Id: <20220604203021.10663-1-mkhalfella@purestorage.com>
Date:   Sat,  4 Jun 2022 20:30:21 +0000
From:   Mohamed Khalfella <mkhalfella@...estorage.com>
To:     helgaas@...nel.org
Cc:     bhelgaas@...gle.com, ebadger@...estorage.com,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org, mkhalfella@...estorage.com,
        msaggi@...estorage.com, oohall@...il.com, rajatja@...gle.com,
        stable@...r.kernel.org
Subject: Re: [PATCH] PCI/AER: Iterate over error counters instead of error

On 6/3/22 16:58, Bjorn Helgaas wrote:
> On Fri, Jun 03, 2022 at 10:12:47PM +0000, Mohamed Khalfella wrote:
>> Is there any chance for this to land in 5.19?
>
> Too late for v5.19, since the merge window will end in a couple days.
> Remind me again if you don't see it in -next by v5.20-rc5 or so.
>

Thank you. I will keep an eye on -next.

>> On 5/10/22 14:17, Mohamed Khalfella wrote:
>>>> Thanks for catching this; it definitely looks like a real issue!  I
>>>> guess you're probably seeing junk in the sysfs files?
>>>
>>> That is correct. The initial report was seeing junk when reading sysfs
>>> files. As descibed, this is happening because we reading data past the
>>> end of the stats counters array.
>>>
>>>
>>>> I think maybe we should populate the currently NULL entries in the
>>>> string[] arrays and simplify the code here, e.g.,
>>>>
>>>> static const char *aer_correctable_error_string[] = {
>>>>        "RxErr",                        /* Bit Position 0       */
>>>>        "dev_cor_errs_bit[1]",
>>>> 	...
>>>>
>>>>  if (stats[i])
>>>>    len += sysfs_emit_at(buf, len, "%s %llu\n", strings_array[i], stats[i]);
>>>
>>> Doing it this way will change the output format. In this case we will show
>>> stats only if their value is greater than zero. The current code shows all the
>>> stats those have names (regardless of their value) plus those have non-zero
>>> values.
>>>
>>>>> @@ -1342,6 +1342,11 @@ static int aer_probe(struct pcie_device *dev)
>>>>>  	struct device *device = &dev->device;
>>>>>  	struct pci_dev *port = dev->port;
>>>>>
>>>>> +	BUILD_BUG_ON(ARRAY_SIZE(aer_correctable_error_string) <
>>>>> +		     AER_MAX_TYPEOF_COR_ERRS);
>>>>> +	BUILD_BUG_ON(ARRAY_SIZE(aer_uncorrectable_error_string) <
>>>>> +		     AER_MAX_TYPEOF_UNCOR_ERRS);
>>>>
>>>> And make these check for "!=" instead of "<".
>>
>> I am happy to remove these BUILD_BUG_ON() if you think it is a good
>> idea to do so.
>
> I think it's good to enforce correctness there somehow, so let's leave
> them there unless somebody has a better idea.
>
>>> This will require unnecessarily extending stats arrays to have 32 entries
>>> in order to match names arrays. If you don't feel strogly about changing
>>> "<" to "!=", I prefer to keep the code as it is.