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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SJ0PR11MB67440741E9625433755FFEED92C22@SJ0PR11MB6744.namprd11.prod.outlook.com>
Date: Fri, 14 Jun 2024 03:13:55 +0000
From: "Duan, Zhenzhong" <zhenzhong.duan@...el.com>
To: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
CC: "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"rafael@...nel.org" <rafael@...nel.org>, "lenb@...nel.org" <lenb@...nel.org>,
	"james.morse@....com" <james.morse@....com>, "Luck, Tony"
	<tony.luck@...el.com>, "bp@...en8.de" <bp@...en8.de>, "dave@...olabs.net"
	<dave@...olabs.net>, "jonathan.cameron@...wei.com"
	<jonathan.cameron@...wei.com>, "Jiang, Dave" <dave.jiang@...el.com>,
	"Schofield, Alison" <alison.schofield@...el.com>, "Verma, Vishal L"
	<vishal.l.verma@...el.com>, "Weiny, Ira" <ira.weiny@...el.com>,
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>, "helgaas@...nel.org"
	<helgaas@...nel.org>, "mahesh@...ux.ibm.com" <mahesh@...ux.ibm.com>,
	"oohall@...il.com" <oohall@...il.com>, "linmiaohe@...wei.com"
	<linmiaohe@...wei.com>, "shiju.jose@...wei.com" <shiju.jose@...wei.com>,
	"Preble, Adam C" <adam.c.preble@...el.com>, "lukas@...ner.de"
	<lukas@...ner.de>, "Smita.KoralahalliChannabasappa@....com"
	<Smita.KoralahalliChannabasappa@....com>, "rrichter@....com"
	<rrichter@....com>, "linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>,
	"linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Tsaur, Erwin"
	<erwin.tsaur@...el.com>, "Kuppuswamy, Sathyanarayanan"
	<sathyanarayanan.kuppuswamy@...el.com>, "Williams, Dan J"
	<dan.j.williams@...el.com>, "Wanyan, Feiting" <feiting.wanyan@...el.com>,
	"Wang, Yudong" <yudong.wang@...el.com>, "Peng, Chao P"
	<chao.p.peng@...el.com>, "qingshun.wang@...ux.intel.com"
	<qingshun.wang@...ux.intel.com>
Subject: RE: [PATCH v4 1/3] PCI/AER: Store UNCOR_STATUS bits that might be
 ANFE in aer_err_info



>-----Original Message-----
>From: Kuppuswamy Sathyanarayanan
><sathyanarayanan.kuppuswamy@...ux.intel.com>
>Subject: Re: [PATCH v4 1/3] PCI/AER: Store UNCOR_STATUS bits that might
>be ANFE in aer_err_info
>
>
>On 6/13/24 7:39 PM, Duan, Zhenzhong wrote:
>> Hi
>>
>>> -----Original Message-----
>>> From: Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@...ux.intel.com>
>>> Subject: Re: [PATCH v4 1/3] PCI/AER: Store UNCOR_STATUS bits that
>might
>>> be ANFE in aer_err_info
>>>
>>> Hi,
>>>
>>> On 5/9/24 1:48 AM, Zhenzhong Duan wrote:
>>>> In some cases the detector of a Non-Fatal Error(NFE) is not the most
>>>> appropriate agent to determine the type of the error. For example,
>>>> when software performs a configuration read from a non-existent
>>>> device or Function, completer will send an ERR_NONFATAL Message.
>>>> On some platforms, ERR_NONFATAL results in a System Error, which
>>>> breaks normal software probing.
>>>>
>>>> Advisory Non-Fatal Error(ANFE) is a special case that can be used
>>>> in above scenario. It is predominantly determined by the role of the
>>>> detecting agent (Requester, Completer, or Receiver) and the specific
>>>> error. In such cases, an agent with AER signals the NFE (if enabled)
>>>> by sending an ERR_COR Message as an advisory to software, instead of
>>>> sending ERR_NONFATAL.
>>>>
>>>> When processing an ANFE, ideally both correctable error(CE) status and
>>>> uncorrectable error(UE) status should be cleared. However, there is no
>>>> way to fully identify the UE associated with ANFE. Even worse, Non-Fatal
>>>> Error(NFE) may set the same UE status bit as ANFE. Treating an ANFE as
>>>> NFE will reproduce above mentioned issue, i.e., breaking softwore
>probing;
>>>> treating NFE as ANFE will make us ignoring some UEs which need active
>>>> recover operation. To avoid clearing UEs that are not ANFE by accident,
>>>> the most conservative route is taken here: If any of the NFE Detected
>>>> bits is set in Device Status, do not touch UE status, they should be
>>>> cleared later by the UE handler. Otherwise, a specific set of UEs that
>>>> may be raised as ANFE according to the PCIe specification will be cleared
>>>> if their corresponding severity is Non-Fatal.
>>>>
>>>> To achieve above purpose, store UNCOR_STATUS bits that might be
>ANFE
>>>> in aer_err_info.anfe_status. So that those bits could be printed and
>>>> processed later.
>>>>
>>>> Tested-by: Yudong Wang <yudong.wang@...el.com>
>>>> Co-developed-by: "Wang, Qingshun" <qingshun.wang@...ux.intel.com>
>>>> Signed-off-by: "Wang, Qingshun" <qingshun.wang@...ux.intel.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@...el.com>
>>>> ---
>>>>  drivers/pci/pci.h      |  1 +
>>>>  drivers/pci/pcie/aer.c | 53
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 54 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>> index 17fed1846847..3f9eb807f9fd 100644
>>>> --- a/drivers/pci/pci.h
>>>> +++ b/drivers/pci/pci.h
>>>> @@ -412,6 +412,7 @@ struct aer_err_info {
>>>>
>>>>  	unsigned int status;		/* COR/UNCOR Error Status */
>>>>  	unsigned int mask;		/* COR/UNCOR Error Mask */
>>>> +	unsigned int anfe_status;	/* UNCOR Error Status for ANFE */
>>>>  	struct pcie_tlp_log tlp;	/* TLP Header */
>>>>  };
>>>>
>>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>>>> index ac6293c24976..f2839b51321a 100644
>>>> --- a/drivers/pci/pcie/aer.c
>>>> +++ b/drivers/pci/pcie/aer.c
>>>> @@ -107,6 +107,12 @@ struct aer_stats {
>>>>  					PCI_ERR_ROOT_MULTI_COR_RCV |
>>> 	\
>>>>  					PCI_ERR_ROOT_MULTI_UNCOR_RCV)
>>>>
>>>> +#define AER_ERR_ANFE_UNC_MASK
>>> 	(PCI_ERR_UNC_POISON_TLP |	\
>>>> +					PCI_ERR_UNC_COMP_TIME |
>>> 	\
>>>> +					PCI_ERR_UNC_COMP_ABORT |
>>> 	\
>>>> +					PCI_ERR_UNC_UNX_COMP |
>>> 	\
>>>> +					PCI_ERR_UNC_UNSUP)
>>>> +
>>>>  static int pcie_aer_disable;
>>>>  static pci_ers_result_t aer_root_reset(struct pci_dev *dev);
>>>>
>>>> @@ -1196,6 +1202,49 @@ void aer_recover_queue(int domain,
>unsigned
>>> int bus, unsigned int devfn,
>>>>  EXPORT_SYMBOL_GPL(aer_recover_queue);
>>>>  #endif
>>>>
>>>> +static void anfe_get_uc_status(struct pci_dev *dev, struct aer_err_info
>>> *info)
>>>> +{
>>>> +	u32 uncor_mask, uncor_status, anfe_status;
>>>> +	u16 device_status;
>>>> +	int aer = dev->aer_cap;
>>>> +
>>>> +	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS,
>>> &uncor_status);
>>>> +	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,
>>> &uncor_mask);
>>>> +	/*
>>>> +	 * According to PCIe Base Specification Revision 6.1,
>>>> +	 * Section 6.2.3.2.4, if an UNCOR error is raised as
>>>> +	 * Advisory Non-Fatal error, it will match the following
>>>> +	 * conditions:
>>>> +	 *	a. The severity of the error is Non-Fatal.
>>>> +	 *	b. The error is one of the following:
>>>> +	 *		1. Poisoned TLP           (Section 6.2.3.2.4.3)
>>>> +	 *		2. Completion Timeout     (Section 6.2.3.2.4.4)
>>>> +	 *		3. Completer Abort        (Section 6.2.3.2.4.1)
>>>> +	 *		4. Unexpected Completion  (Section 6.2.3.2.4.5)
>>>> +	 *		5. Unsupported Request    (Section 6.2.3.2.4.1)
>>>> +	 */
>>>> +	anfe_status = uncor_status & ~uncor_mask & ~info->severity &
>>>> +		      AER_ERR_ANFE_UNC_MASK;
>>>> +
>>>> +	if (pcie_capability_read_word(dev, PCI_EXP_DEVSTA,
>>> &device_status))
>>>> +		return;
>>>> +	/*
>>>> +	 * Take the most conservative route here. If there are Non-Fatal
>>> errors
>>>> +	 * detected, do not assume any bit in uncor_status is set by ANFE.
>>>> +	 */
>>>> +	if (device_status & PCI_EXP_DEVSTA_NFED)
>>>> +		return;
>>> You can move this check to the top of the function. You don't need to
>check
>>> the rest if NFE error is detected in device status.
>> The v3 just worked that way. Jonathan pointed a race that NFE triggered
>after
>> the check will be treated as ANFE and cleared. Check it after reading
>UNCOR_STATUS
>> can avoid the race.
>>
>> See https://lkml.org/lkml/2024/4/22/1011 for discussion details.
>
>Got it. I would recommend adding a comment about it in handler. May be
>some thing like,
>
>/*
> * To avoid race between device status read and error status register read,
>cache
> * uncorrectable error status before checking for NFE in device status *
>register. */

Good suggestion, will add.

Thanks
Zhenzhong

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ