linux-kernel - RE: [PATCH 1/3] iommu/vt-d: Add rate limited information when PRQ overflows

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: <MWHPR11MB18862D2EA5BD432BF22D99A48CA09@MWHPR11MB1886.namprd11.prod.outlook.com>
Date:   Fri, 22 Jan 2021 06:38:42 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Lu Baolu <baolu.lu@...ux.intel.com>,
        Joerg Roedel <joro@...tes.org>, "Will Deacon" <will@...nel.org>
CC:     "Raj, Ashok" <ashok.raj@...el.com>,
        Jacob Pan <jacob.jun.pan@...ux.intel.com>,
        "Liu, Yi L" <yi.l.liu@...el.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/3] iommu/vt-d: Add rate limited information when PRQ
 overflows

> From: Lu Baolu <baolu.lu@...ux.intel.com>
> Sent: Thursday, January 21, 2021 9:45 AM
> 
> So that the uses could get chances to know what happened.
> 
> Suggested-by: Ashok Raj <ashok.raj@...el.com>
> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
> ---
>  drivers/iommu/intel/svm.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 033b25886e57..f49fe715477b 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -895,6 +895,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>  	struct intel_iommu *iommu = d;
>  	struct intel_svm *svm = NULL;
>  	int head, tail, handled = 0;
> +	struct page_req_dsc *req;
> 
>  	/* Clear PPR bit before reading head/tail registers, to
>  	 * ensure that we get a new interrupt if needed. */
> @@ -904,7 +905,6 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>  	head = dmar_readq(iommu->reg + DMAR_PQH_REG) &
> PRQ_RING_MASK;
>  	while (head != tail) {
>  		struct vm_area_struct *vma;
> -		struct page_req_dsc *req;
>  		struct qi_desc resp;
>  		int result;
>  		vm_fault_t ret;
> @@ -1042,8 +1042,14 @@ static irqreturn_t prq_event_thread(int irq, void
> *d)
>  	 * Clear the page request overflow bit and wake up all threads that
>  	 * are waiting for the completion of this handling.
>  	 */
> -	if (readl(iommu->reg + DMAR_PRS_REG) & DMA_PRS_PRO)
> +	if (readl(iommu->reg + DMAR_PRS_REG) & DMA_PRS_PRO) {
> +		head = dmar_readq(iommu->reg + DMAR_PQH_REG) &
> PRQ_RING_MASK;
> +		req = &iommu->prq[head / sizeof(*req)];
> +		pr_warn_ratelimited("IOMMU: %s: Page request overflow:
> HEAD: %08llx %08llx",
> +				    iommu->name, ((unsigned long long
> *)req)[0],
> +				    ((unsigned long long *)req)[1]);
>  		writel(DMA_PRS_PRO, iommu->reg + DMAR_PRS_REG);
> +	}
> 

Not about rate limiting but I think we may have a problem in above
logic. It is incorrect to always clear PRO when it's set w/o first checking
whether the overflow condition has been cleared. This code assumes
that if an overflow condition occurs it must have been cleared by earlier
loop when hitting this check. However since this code runs in a threaded 
context, the overflow condition could occur even after you reset the head 
to the tail (under some extreme condition). To be sane I think we'd better
read both head/tail again after seeing a pending PRO here and only clear 
PRO when it becomes a false indicator based on latest head/tail.

Thanks
Kevin