linux-kernel - Re: [PATCH 3/3] iommu: Convert response code from failure to invalid

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: <20240711233730.GT14050@ziepe.ca>
Date: Thu, 11 Jul 2024 20:37:30 -0300
From: Jason Gunthorpe <jgg@...pe.ca>
To: "Tian, Kevin" <kevin.tian@...el.com>
Cc: Lu Baolu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>,
	Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
	Nicolin Chen <nicolinc@...dia.com>,
	"Liu, Yi L" <yi.l.liu@...el.com>,
	"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] iommu: Convert response code from failure to invalid

On Wed, Jul 10, 2024 at 09:56:34AM +0000, Tian, Kevin wrote:
> > From: Lu Baolu <baolu.lu@...ux.intel.com>
> > Sent: Wednesday, July 10, 2024 4:34 PM
> > 
> > In the iopf deliver path, iommu_report_device_fault() currently responds
> > a code of "Response Failure" to the hardware if it can't find a suitable
> > iopf-capable domain where the page faults should be handled. The Response
> > Failure is defined in PCI spec (section 10.4.2.1) as a catastrophic error,
> > and the device will disable PRI for the function.
> > 
> > Failing to dispatch a set of fault messages is not a catastrophic error
> > and the iommu core has no code to recover the PRI once it is disabled.
> > 
> > Replace it with a response code of "Invalid Response".
> > 
> > Fixes: b554e396e51c ("iommu: Make iopf_group_response() return void")
> > Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
> > ---
> >  drivers/iommu/io-pgfault.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> > index cd679c13752e..b8270ee5dcdb 100644
> > --- a/drivers/iommu/io-pgfault.c
> > +++ b/drivers/iommu/io-pgfault.c
> > @@ -229,7 +229,7 @@ void iommu_report_device_fault(struct device *dev,
> > struct iopf_fault *evt)
> >  err_abort:
> >  	dev_warn_ratelimited(dev, "iopf with pasid %d aborted\n",
> >  			     fault->prm.pasid);
> > -	iopf_group_response(group, IOMMU_PAGE_RESP_FAILURE);
> > +	iopf_group_response(group, IOMMU_PAGE_RESP_INVALID);
> >  	if (group == &abort_group)
> >  		__iopf_free_group(group);
> >  	else
> 
> this doesn't match the spec words on the use of INVALID:
> 
>   One or more pages within the associated PRG do not exist or
>   requests access privilege(s) that cannot be granted. Unless the
>   page mapping associated with the Function is altered, re-issuance
>   of the associated request will never result in success.
> 
> this situation is not related to the permission of page mapping. Instead
> it's a global problem applying to all functions submitting page requests
> at this moment.
> 
> Though using FAILURE affects more functions sharing the PRI interface,
> it also proactively avoids adding more in-fly requests to worsen the
> low memory situation.
> 
> So none of the options looks perfect to me, but the existing code
> responding FAILURE is more aligned to the spec?

Yeah, I don't know what to do here either, I guess the system is going
to explode if you OOM during this stage.

If it is some problem we'd probably have to preallocate this memory in
a pool and rate limite the number of PRIs we allow open at once, using
INVALID to create some back pressure. Seems wild.

Lets keep it as FAILURE for now, or at least it can be rediscussed
another time.

Jason