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  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]
Date:   Wed, 6 Mar 2019 15:46:16 -0800
From:   Jacob Pan <jacob.jun.pan@...ux.intel.com>
To:     Jean-Philippe Brucker <jean-philippe.brucker@....com>
Cc:     Eric Auger <eric.auger@...hat.com>, eric.auger.pro@...il.com,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, kvmarm@...ts.cs.columbia.edu, joro@...tes.org,
        alex.williamson@...hat.com, yi.l.liu@...ux.intel.com,
        will.deacon@....com, robin.murphy@....com, marc.zyngier@....com,
        peter.maydell@...aro.org, kevin.tian@...el.com,
        ashok.raj@...el.com, christoffer.dall@....com,
        jacob.jun.pan@...ux.intel.com
Subject: Re: [PATCH v4 03/22] iommu: introduce device fault report API

On Tue, 5 Mar 2019 15:03:41 +0000
Jean-Philippe Brucker <jean-philippe.brucker@....com> wrote:

> On 18/02/2019 13:54, Eric Auger wrote:
> [...]> +/**
> > + * iommu_register_device_fault_handler() - Register a device fault
> > handler
> > + * @dev: the device
> > + * @handler: the fault handler
> > + * @data: private data passed as argument to the handler
> > + *
> > + * When an IOMMU fault event is received, call this handler with
> > the fault event
> > + * and data as argument. The handler should return 0 on success.
> > If the fault is
> > + * recoverable (IOMMU_FAULT_PAGE_REQ), the handler can also
> > complete
> > + * the fault by calling iommu_page_response() with one of the
> > following
> > + * response code:
> > + * - IOMMU_PAGE_RESP_SUCCESS: retry the translation
> > + * - IOMMU_PAGE_RESP_INVALID: terminate the fault
> > + * - IOMMU_PAGE_RESP_FAILURE: terminate the fault and stop
> > reporting
> > + *   page faults if possible.  
> 
> The comment refers to function and values that haven't been defined
> yet. Either the page_response() patch should come before, or we need
> to split this patch.
> 
> Something I missed before: if the handler fails (returns != 0) it
> should complete the fault by calling iommu_page_response(), if we're
> not doing it in iommu_report_device_fault(). It should be indicated
> in this comment. It's safe for the handler to call page_response()
> since we're not holding fault_param->lock when calling the handler.
> 
If the page request fault is to be reported to a guest, the report
function cannot wait for the completion status. As long as the fault is
injected into the guest, the handler should complete with success. If
the PRQ report fails, IMHO, the caller of iommu_report_device_fault()
should send page_response, perhaps after clean up all partial response
of the group too.

> > + *
> > + * Return 0 if the fault handler was installed successfully, or an
> > error.
> > + */  
> [...]
> > +/**
> > + * iommu_report_device_fault() - Report fault event to device
> > + * @dev: the device
> > + * @evt: fault event data
> > + *
> > + * Called by IOMMU model specific drivers when fault is detected,
> > typically
> > + * in a threaded IRQ handler.
> > + *
> > + * Return 0 on success, or an error.
> > + */
> > +int iommu_report_device_fault(struct device *dev, struct
> > iommu_fault_event *evt) +{
> > +	int ret = 0;
> > +	struct iommu_fault_event *evt_pending;
> > +	struct iommu_fault_param *fparam;
> > +
> > +	/* iommu_param is allocated when device is added to group
> > */
> > +	if (!dev->iommu_param | !evt)  
> 
> Typo: ||
> 
> Thanks,
> Jean
> 
> > +		return -EINVAL;
> > +	/* we only report device fault if there is a handler
> > registered */
> > +	mutex_lock(&dev->iommu_param->lock);
> > +	if (!dev->iommu_param->fault_param ||
> > +		!dev->iommu_param->fault_param->handler) {
> > +		ret = -EINVAL;
> > +		goto done_unlock;
> > +	}
> > +	fparam = dev->iommu_param->fault_param;
> > +	if (evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
> > +	    evt->fault.prm.flags &
> > IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE) {
> > +		evt_pending = kmemdup(evt, sizeof(struct
> > iommu_fault_event),
> > +				GFP_KERNEL);
> > +		if (!evt_pending) {
> > +			ret = -ENOMEM;
> > +			goto done_unlock;
> > +		}
> > +		mutex_lock(&fparam->lock);
> > +		list_add_tail(&evt_pending->list, &fparam->faults);
> > +		mutex_unlock(&fparam->lock);
> > +	}
> > +	ret = fparam->handler(evt, fparam->data);
> > +done_unlock:
> > +	mutex_unlock(&dev->iommu_param->lock);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_report_device_fault);  
> [...]

[Jacob Pan]

Powered by blists - more mailing lists