* [PATCH 0/3] iommufd: Avoid PRI Response Failure
@ 2024-07-10 8:33 Lu Baolu
2024-07-10 8:33 ` [PATCH 1/3] iommufd: Remove IOMMUFD_PAGE_RESP_FAILURE Lu Baolu
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Lu Baolu @ 2024-07-10 8:33 UTC (permalink / raw)
To: Jason Gunthorpe, Kevin Tian, Joerg Roedel, Will Deacon,
Robin Murphy, Nicolin Chen, Yi Liu
Cc: iommu, linux-kernel, Lu Baolu
This series is a follow-up for the discussion that happened here.
https://lore.kernel.org/linux-iommu/20240709173643.GI14050@ziepe.ca/
It prevents user space from responding to a group of page faults with a
response code other than IOMMUFD_PAGE_RESP_SUCCESS and
IOMMUFD_PAGE_RESP_INVALID, which will be treated by the device as
Response Failure according to the PCI spec.
Please help review and comment.
Lu Baolu (3):
iommufd: Remove IOMMUFD_PAGE_RESP_FAILURE
iommufd: Add check on user response code
iommu: Convert response code from failure to invalid
include/uapi/linux/iommufd.h | 4 ----
drivers/iommu/io-pgfault.c | 2 +-
drivers/iommu/iommufd/fault.c | 6 ++++++
3 files changed, 7 insertions(+), 5 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] iommufd: Remove IOMMUFD_PAGE_RESP_FAILURE
2024-07-10 8:33 [PATCH 0/3] iommufd: Avoid PRI Response Failure Lu Baolu
@ 2024-07-10 8:33 ` Lu Baolu
2024-07-10 9:45 ` Tian, Kevin
2024-07-10 8:33 ` [PATCH 2/3] iommufd: Add check on user response code Lu Baolu
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Lu Baolu @ 2024-07-10 8:33 UTC (permalink / raw)
To: Jason Gunthorpe, Kevin Tian, Joerg Roedel, Will Deacon,
Robin Murphy, Nicolin Chen, Yi Liu
Cc: iommu, linux-kernel, Lu Baolu
The response code of IOMMUFD_PAGE_RESP_FAILURE was defined to be
equivalent to the "Response Failure" in PCI spec, section 10.4.2.1.
This response code indicates that one or more pages within the
associated request group have encountered or caused an unrecoverable
error. Therefore, this response disables the PRI at the function.
Modern I/O virtualization technologies, like SR-IOV, share PRI among
the assignable device units. Therefore, a response failure on one unit
might cause I/O failure on other units.
Remove this response code so that user space can only respond with
SUCCESS or INVALID. The VMM is recommended to emulate a failure response
as a PRI reset, or PRI disable and changing to a non-PRI domain.
Fixes: c714f15860fc ("iommufd: Add fault and response message definitions")
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
include/uapi/linux/iommufd.h | 4 ----
1 file changed, 4 deletions(-)
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index ede2b464a761..e31385b75d0b 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -765,14 +765,10 @@ struct iommu_hwpt_pgfault {
* @IOMMUFD_PAGE_RESP_INVALID: Could not handle this fault, don't retry the
* access. This is the "Invalid Request" in PCI
* 10.4.2.1.
- * @IOMMUFD_PAGE_RESP_FAILURE: General error. Drop all subsequent faults from
- * this device if possible. This is the "Response
- * Failure" in PCI 10.4.2.1.
*/
enum iommufd_page_response_code {
IOMMUFD_PAGE_RESP_SUCCESS = 0,
IOMMUFD_PAGE_RESP_INVALID,
- IOMMUFD_PAGE_RESP_FAILURE,
};
/**
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] iommufd: Add check on user response code
2024-07-10 8:33 [PATCH 0/3] iommufd: Avoid PRI Response Failure Lu Baolu
2024-07-10 8:33 ` [PATCH 1/3] iommufd: Remove IOMMUFD_PAGE_RESP_FAILURE Lu Baolu
@ 2024-07-10 8:33 ` Lu Baolu
2024-07-10 9:47 ` Tian, Kevin
2024-07-11 23:39 ` Jason Gunthorpe
2024-07-10 8:33 ` [PATCH 3/3] iommu: Convert response code from failure to invalid Lu Baolu
2024-07-11 23:45 ` [PATCH 0/3] iommufd: Avoid PRI Response Failure Jason Gunthorpe
3 siblings, 2 replies; 13+ messages in thread
From: Lu Baolu @ 2024-07-10 8:33 UTC (permalink / raw)
To: Jason Gunthorpe, Kevin Tian, Joerg Roedel, Will Deacon,
Robin Murphy, Nicolin Chen, Yi Liu
Cc: iommu, linux-kernel, Lu Baolu
The response code from user space is only allowed to be SUCCESS or
INVALID. All other values are treated by the device as a response
code of Response Failure according to PCI spec, section 10.4.2.1.
This response disables the Page Request Interface for the Function.
Add a check in iommufd_fault_fops_write() to avoid invalid response
code.
Fixes: 07838f7fd529 ("iommufd: Add iommufd fault object")
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/iommufd/fault.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index 54d6cd20a673..044b9b97da31 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -305,6 +305,12 @@ static ssize_t iommufd_fault_fops_write(struct file *filep, const char __user *b
if (rc)
break;
+ if (response.code != IOMMUFD_PAGE_RESP_SUCCESS &&
+ response.code != IOMMUFD_PAGE_RESP_INVALID) {
+ rc = -EINVAL;
+ break;
+ }
+
group = xa_erase(&fault->response, response.cookie);
if (!group) {
rc = -EINVAL;
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] iommu: Convert response code from failure to invalid
2024-07-10 8:33 [PATCH 0/3] iommufd: Avoid PRI Response Failure Lu Baolu
2024-07-10 8:33 ` [PATCH 1/3] iommufd: Remove IOMMUFD_PAGE_RESP_FAILURE Lu Baolu
2024-07-10 8:33 ` [PATCH 2/3] iommufd: Add check on user response code Lu Baolu
@ 2024-07-10 8:33 ` Lu Baolu
2024-07-10 9:56 ` Tian, Kevin
2024-07-11 23:45 ` [PATCH 0/3] iommufd: Avoid PRI Response Failure Jason Gunthorpe
3 siblings, 1 reply; 13+ messages in thread
From: Lu Baolu @ 2024-07-10 8:33 UTC (permalink / raw)
To: Jason Gunthorpe, Kevin Tian, Joerg Roedel, Will Deacon,
Robin Murphy, Nicolin Chen, Yi Liu
Cc: iommu, linux-kernel, Lu Baolu
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@linux.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
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* RE: [PATCH 1/3] iommufd: Remove IOMMUFD_PAGE_RESP_FAILURE
2024-07-10 8:33 ` [PATCH 1/3] iommufd: Remove IOMMUFD_PAGE_RESP_FAILURE Lu Baolu
@ 2024-07-10 9:45 ` Tian, Kevin
0 siblings, 0 replies; 13+ messages in thread
From: Tian, Kevin @ 2024-07-10 9:45 UTC (permalink / raw)
To: Lu Baolu, Jason Gunthorpe, Joerg Roedel, Will Deacon,
Robin Murphy, Nicolin Chen, Liu, Yi L
Cc: iommu@lists.linux.dev, linux-kernel@vger.kernel.org
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Wednesday, July 10, 2024 4:34 PM
>
> The response code of IOMMUFD_PAGE_RESP_FAILURE was defined to be
> equivalent to the "Response Failure" in PCI spec, section 10.4.2.1.
> This response code indicates that one or more pages within the
> associated request group have encountered or caused an unrecoverable
> error. Therefore, this response disables the PRI at the function.
>
> Modern I/O virtualization technologies, like SR-IOV, share PRI among
> the assignable device units. Therefore, a response failure on one unit
> might cause I/O failure on other units.
>
> Remove this response code so that user space can only respond with
> SUCCESS or INVALID. The VMM is recommended to emulate a failure
> response
> as a PRI reset, or PRI disable and changing to a non-PRI domain.
>
> Fixes: c714f15860fc ("iommufd: Add fault and response message definitions")
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> include/uapi/linux/iommufd.h | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
> index ede2b464a761..e31385b75d0b 100644
> --- a/include/uapi/linux/iommufd.h
> +++ b/include/uapi/linux/iommufd.h
> @@ -765,14 +765,10 @@ struct iommu_hwpt_pgfault {
> * @IOMMUFD_PAGE_RESP_INVALID: Could not handle this fault, don't retry
> the
> * access. This is the "Invalid Request" in PCI
> * 10.4.2.1.
> - * @IOMMUFD_PAGE_RESP_FAILURE: General error. Drop all subsequent
> faults from
> - * this device if possible. This is the "Response
> - * Failure" in PCI 10.4.2.1.
> */
> enum iommufd_page_response_code {
> IOMMUFD_PAGE_RESP_SUCCESS = 0,
> IOMMUFD_PAGE_RESP_INVALID,
> - IOMMUFD_PAGE_RESP_FAILURE,
> };
>
It's probably good to add some words here for why FAILURE is
not allowed.
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 2/3] iommufd: Add check on user response code
2024-07-10 8:33 ` [PATCH 2/3] iommufd: Add check on user response code Lu Baolu
@ 2024-07-10 9:47 ` Tian, Kevin
2024-07-11 23:39 ` Jason Gunthorpe
1 sibling, 0 replies; 13+ messages in thread
From: Tian, Kevin @ 2024-07-10 9:47 UTC (permalink / raw)
To: Lu Baolu, Jason Gunthorpe, Joerg Roedel, Will Deacon,
Robin Murphy, Nicolin Chen, Liu, Yi L
Cc: iommu@lists.linux.dev, linux-kernel@vger.kernel.org
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Wednesday, July 10, 2024 4:34 PM
>
> The response code from user space is only allowed to be SUCCESS or
> INVALID. All other values are treated by the device as a response
> code of Response Failure according to PCI spec, section 10.4.2.1.
> This response disables the Page Request Interface for the Function.
>
> Add a check in iommufd_fault_fops_write() to avoid invalid response
> code.
"avoid invalid response code" but "RESP_INVALID is allowed" 😊.
I know what it means though...
>
> Fixes: 07838f7fd529 ("iommufd: Add iommufd fault object")
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 3/3] iommu: Convert response code from failure to invalid
2024-07-10 8:33 ` [PATCH 3/3] iommu: Convert response code from failure to invalid Lu Baolu
@ 2024-07-10 9:56 ` Tian, Kevin
2024-07-11 4:42 ` Baolu Lu
2024-07-11 23:37 ` Jason Gunthorpe
0 siblings, 2 replies; 13+ messages in thread
From: Tian, Kevin @ 2024-07-10 9:56 UTC (permalink / raw)
To: Lu Baolu, Jason Gunthorpe, Joerg Roedel, Will Deacon,
Robin Murphy, Nicolin Chen, Liu, Yi L
Cc: iommu@lists.linux.dev, linux-kernel@vger.kernel.org
> From: Lu Baolu <baolu.lu@linux.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@linux.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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] iommu: Convert response code from failure to invalid
2024-07-10 9:56 ` Tian, Kevin
@ 2024-07-11 4:42 ` Baolu Lu
2024-07-11 23:37 ` Jason Gunthorpe
1 sibling, 0 replies; 13+ messages in thread
From: Baolu Lu @ 2024-07-11 4:42 UTC (permalink / raw)
To: Tian, Kevin, Jason Gunthorpe, Joerg Roedel, Will Deacon,
Robin Murphy, Nicolin Chen, Liu, Yi L
Cc: baolu.lu, iommu@lists.linux.dev, linux-kernel@vger.kernel.org
On 7/10/24 5:56 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.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@linux.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?
Fair enough. Perhaps we can keep the existing code as-is unless any real
issues arise.
Thanks,
baolu
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] iommu: Convert response code from failure to invalid
2024-07-10 9:56 ` Tian, Kevin
2024-07-11 4:42 ` Baolu Lu
@ 2024-07-11 23:37 ` Jason Gunthorpe
1 sibling, 0 replies; 13+ messages in thread
From: Jason Gunthorpe @ 2024-07-11 23:37 UTC (permalink / raw)
To: Tian, Kevin
Cc: Lu Baolu, Joerg Roedel, Will Deacon, Robin Murphy, Nicolin Chen,
Liu, Yi L, iommu@lists.linux.dev, linux-kernel@vger.kernel.org
On Wed, Jul 10, 2024 at 09:56:34AM +0000, Tian, Kevin wrote:
> > From: Lu Baolu <baolu.lu@linux.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@linux.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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] iommufd: Add check on user response code
2024-07-10 8:33 ` [PATCH 2/3] iommufd: Add check on user response code Lu Baolu
2024-07-10 9:47 ` Tian, Kevin
@ 2024-07-11 23:39 ` Jason Gunthorpe
2024-07-12 2:54 ` Baolu Lu
1 sibling, 1 reply; 13+ messages in thread
From: Jason Gunthorpe @ 2024-07-11 23:39 UTC (permalink / raw)
To: Lu Baolu
Cc: Kevin Tian, Joerg Roedel, Will Deacon, Robin Murphy, Nicolin Chen,
Yi Liu, iommu, linux-kernel
On Wed, Jul 10, 2024 at 04:33:40PM +0800, Lu Baolu wrote:
> The response code from user space is only allowed to be SUCCESS or
> INVALID. All other values are treated by the device as a response
> code of Response Failure according to PCI spec, section 10.4.2.1.
> This response disables the Page Request Interface for the Function.
>
> Add a check in iommufd_fault_fops_write() to avoid invalid response
> code.
>
> Fixes: 07838f7fd529 ("iommufd: Add iommufd fault object")
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> drivers/iommu/iommufd/fault.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
> index 54d6cd20a673..044b9b97da31 100644
> --- a/drivers/iommu/iommufd/fault.c
> +++ b/drivers/iommu/iommufd/fault.c
> @@ -305,6 +305,12 @@ static ssize_t iommufd_fault_fops_write(struct file *filep, const char __user *b
> if (rc)
> break;
>
> + if (response.code != IOMMUFD_PAGE_RESP_SUCCESS &&
> + response.code != IOMMUFD_PAGE_RESP_INVALID) {
> + rc = -EINVAL;
> + break;
> + }
I added this:
static_assert(IOMMUFD_PAGE_RESP_SUCCESS ==
IOMMU_PAGE_RESP_SUCCESS);
static_assert(IOMMUFD_PAGE_RESP_INVALID ==
IOMMU_PAGE_RESP_INVALID);
As well
Jason
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] iommufd: Avoid PRI Response Failure
2024-07-10 8:33 [PATCH 0/3] iommufd: Avoid PRI Response Failure Lu Baolu
` (2 preceding siblings ...)
2024-07-10 8:33 ` [PATCH 3/3] iommu: Convert response code from failure to invalid Lu Baolu
@ 2024-07-11 23:45 ` Jason Gunthorpe
3 siblings, 0 replies; 13+ messages in thread
From: Jason Gunthorpe @ 2024-07-11 23:45 UTC (permalink / raw)
To: Lu Baolu
Cc: Kevin Tian, Joerg Roedel, Will Deacon, Robin Murphy, Nicolin Chen,
Yi Liu, iommu, linux-kernel
On Wed, Jul 10, 2024 at 04:33:38PM +0800, Lu Baolu wrote:
> This series is a follow-up for the discussion that happened here.
>
> https://lore.kernel.org/linux-iommu/20240709173643.GI14050@ziepe.ca/
>
> It prevents user space from responding to a group of page faults with a
> response code other than IOMMUFD_PAGE_RESP_SUCCESS and
> IOMMUFD_PAGE_RESP_INVALID, which will be treated by the device as
> Response Failure according to the PCI spec.
>
> Please help review and comment.
>
> Lu Baolu (3):
> iommufd: Remove IOMMUFD_PAGE_RESP_FAILURE
> iommufd: Add check on user response code
> iommu: Convert response code from failure to invalid
Applied to iommufd for-next
Thanks,
Jason
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] iommufd: Add check on user response code
2024-07-11 23:39 ` Jason Gunthorpe
@ 2024-07-12 2:54 ` Baolu Lu
2024-07-12 12:43 ` Jason Gunthorpe
0 siblings, 1 reply; 13+ messages in thread
From: Baolu Lu @ 2024-07-12 2:54 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: baolu.lu, Kevin Tian, Joerg Roedel, Will Deacon, Robin Murphy,
Nicolin Chen, Yi Liu, iommu, linux-kernel
On 7/12/24 7:39 AM, Jason Gunthorpe wrote:
> On Wed, Jul 10, 2024 at 04:33:40PM +0800, Lu Baolu wrote:
>> The response code from user space is only allowed to be SUCCESS or
>> INVALID. All other values are treated by the device as a response
>> code of Response Failure according to PCI spec, section 10.4.2.1.
>> This response disables the Page Request Interface for the Function.
>>
>> Add a check in iommufd_fault_fops_write() to avoid invalid response
>> code.
>>
>> Fixes: 07838f7fd529 ("iommufd: Add iommufd fault object")
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>> drivers/iommu/iommufd/fault.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
>> index 54d6cd20a673..044b9b97da31 100644
>> --- a/drivers/iommu/iommufd/fault.c
>> +++ b/drivers/iommu/iommufd/fault.c
>> @@ -305,6 +305,12 @@ static ssize_t iommufd_fault_fops_write(struct file *filep, const char __user *b
>> if (rc)
>> break;
>>
>> + if (response.code != IOMMUFD_PAGE_RESP_SUCCESS &&
>> + response.code != IOMMUFD_PAGE_RESP_INVALID) {
>> + rc = -EINVAL;
>> + break;
>> + }
>
> I added this:
>
> static_assert(IOMMUFD_PAGE_RESP_SUCCESS ==
> IOMMU_PAGE_RESP_SUCCESS);
> static_assert(IOMMUFD_PAGE_RESP_INVALID ==
> IOMMU_PAGE_RESP_INVALID);
Above change cause below build warning:
drivers/iommu/iommufd/fault.c: In function ‘iommufd_fault_fops_write’:
drivers/iommu/iommufd/fault.c:308:57: warning: comparison between ‘enum
iommufd_page_response_code’ and ‘enum iommu_page_response_code’
[-Wenum-compare]
308 | static_assert(IOMMUFD_PAGE_RESP_SUCCESS ==
| ^~
./include/linux/build_bug.h:78:56: note: in definition of macro
‘__static_assert’
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~
drivers/iommu/iommufd/fault.c:308:17: note: in expansion of macro
‘static_assert’
308 | static_assert(IOMMUFD_PAGE_RESP_SUCCESS ==
| ^~~~~~~~~~~~~
drivers/iommu/iommufd/fault.c:310:57: warning: comparison between ‘enum
iommufd_page_response_code’ and ‘enum iommu_page_response_code’
[-Wenum-compare]
310 | static_assert(IOMMUFD_PAGE_RESP_INVALID ==
| ^~
./include/linux/build_bug.h:78:56: note: in definition of macro
‘__static_assert’
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~
drivers/iommu/iommufd/fault.c:310:17: note: in expansion of macro
‘static_assert’
310 | static_assert(IOMMUFD_PAGE_RESP_INVALID ==
| ^~~~~~~~~~~~~
Perhaps convert them to 'int' before compare?
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index 29f522819759..a643d5c7c535 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -305,10 +305,10 @@ static ssize_t iommufd_fault_fops_write(struct
file *filep, const char __user *b
if (rc)
break;
- static_assert(IOMMUFD_PAGE_RESP_SUCCESS ==
- IOMMU_PAGE_RESP_SUCCESS);
- static_assert(IOMMUFD_PAGE_RESP_INVALID ==
- IOMMU_PAGE_RESP_INVALID);
+ static_assert((int)IOMMUFD_PAGE_RESP_SUCCESS ==
+ (int)IOMMU_PAGE_RESP_SUCCESS);
+ static_assert((int)IOMMUFD_PAGE_RESP_INVALID ==
+ (int)IOMMU_PAGE_RESP_INVALID);
if (response.code != IOMMUFD_PAGE_RESP_SUCCESS &&
response.code != IOMMUFD_PAGE_RESP_INVALID) {
rc = -EINVAL;
Thanks,
baolu
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] iommufd: Add check on user response code
2024-07-12 2:54 ` Baolu Lu
@ 2024-07-12 12:43 ` Jason Gunthorpe
0 siblings, 0 replies; 13+ messages in thread
From: Jason Gunthorpe @ 2024-07-12 12:43 UTC (permalink / raw)
To: Baolu Lu
Cc: Kevin Tian, Joerg Roedel, Will Deacon, Robin Murphy, Nicolin Chen,
Yi Liu, iommu, linux-kernel
On Fri, Jul 12, 2024 at 10:54:11AM +0800, Baolu Lu wrote:
> diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
> index 29f522819759..a643d5c7c535 100644
> --- a/drivers/iommu/iommufd/fault.c
> +++ b/drivers/iommu/iommufd/fault.c
> @@ -305,10 +305,10 @@ static ssize_t iommufd_fault_fops_write(struct file
> *filep, const char __user *b
> if (rc)
> break;
>
> - static_assert(IOMMUFD_PAGE_RESP_SUCCESS ==
> - IOMMU_PAGE_RESP_SUCCESS);
> - static_assert(IOMMUFD_PAGE_RESP_INVALID ==
> - IOMMU_PAGE_RESP_INVALID);
> + static_assert((int)IOMMUFD_PAGE_RESP_SUCCESS ==
> + (int)IOMMU_PAGE_RESP_SUCCESS);
> + static_assert((int)IOMMUFD_PAGE_RESP_INVALID ==
> + (int)IOMMU_PAGE_RESP_INVALID);
> if (response.code != IOMMUFD_PAGE_RESP_SUCCESS &&
> response.code != IOMMUFD_PAGE_RESP_INVALID) {
> rc = -EINVAL;
Yep, my compiler doesn't warn on that apparently..
Jason
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-07-12 12:43 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-10 8:33 [PATCH 0/3] iommufd: Avoid PRI Response Failure Lu Baolu
2024-07-10 8:33 ` [PATCH 1/3] iommufd: Remove IOMMUFD_PAGE_RESP_FAILURE Lu Baolu
2024-07-10 9:45 ` Tian, Kevin
2024-07-10 8:33 ` [PATCH 2/3] iommufd: Add check on user response code Lu Baolu
2024-07-10 9:47 ` Tian, Kevin
2024-07-11 23:39 ` Jason Gunthorpe
2024-07-12 2:54 ` Baolu Lu
2024-07-12 12:43 ` Jason Gunthorpe
2024-07-10 8:33 ` [PATCH 3/3] iommu: Convert response code from failure to invalid Lu Baolu
2024-07-10 9:56 ` Tian, Kevin
2024-07-11 4:42 ` Baolu Lu
2024-07-11 23:37 ` Jason Gunthorpe
2024-07-11 23:45 ` [PATCH 0/3] iommufd: Avoid PRI Response Failure Jason Gunthorpe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).