Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1741579 > unrolled thread

[PATCH] iommu/vt-d: Fix scatterlist offset handling

Started byRobin Murphy <robin.murphy@arm.com>
First post2017-09-28 16:20 +0200
Last post2017-09-29 18:20 +0200
Articles 7 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] iommu/vt-d: Fix scatterlist offset handling Robin Murphy <robin.murphy@arm.com> - 2017-09-28 16:20 +0200
    Re: [PATCH] iommu/vt-d: Fix scatterlist offset handling Casey Leedom <leedom@chelsio.com> - 2017-09-28 18:20 +0200
      Re: [PATCH] iommu/vt-d: Fix scatterlist offset handling "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-28 18:30 +0200
        Re: [PATCH] iommu/vt-d: Fix scatterlist offset handling Robin Murphy <robin.murphy@arm.com> - 2017-09-28 19:00 +0200
          Re: [PATCH] iommu/vt-d: Fix scatterlist offset handling "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-28 20:40 +0200
    Re: [PATCH] iommu/vt-d: Fix scatterlist offset handling Harsh Jain <Harsh@chelsio.com> - 2017-09-29 10:20 +0200
      Re: [PATCH] iommu/vt-d: Fix scatterlist offset handling Casey Leedom <leedom@chelsio.com> - 2017-09-29 18:20 +0200

#1741579 — [PATCH] iommu/vt-d: Fix scatterlist offset handling

FromRobin Murphy <robin.murphy@arm.com>
Date2017-09-28 16:20 +0200
Subject[PATCH] iommu/vt-d: Fix scatterlist offset handling
Message-ID<uuHPs-1Bi-3@gated-at.bofh.it>
The intel-iommu DMA ops fail to correctly handle scatterlists where
sg->offset is greater than PAGE_SIZE - the IOVA allocation is computed
appropriately based on the page-aligned portion of the offset, but the
mapping is set up relative to sg->page, which means it fails to actually
cover the whole buffer (and in the worst case doesn't cover it at all):

    (sg->dma_address + sg->dma_len) ----+
    sg->dma_address ---------+          |
    iov_pfn------+           |          |
                 |           |          |
                 v           v          v
iova:   a        b        c        d        e        f
        |--------|--------|--------|--------|--------|
                          <...calculated....>
                 [_____mapped______]
pfn:    0        1        2        3        4        5
        |--------|--------|--------|--------|--------|
                 ^           ^          ^
                 |           |          |
    sg->page ----+           |          |
    sg->offset --------------+          |
    (sg->offset + sg->length) ----------+

As a result, the caller ends up overrunning the mapping into whatever
lies beyond, which usually goes badly:

[  429.645492] DMAR: DRHD: handling fault status reg 2
[  429.650847] DMAR: [DMA Write] Request device [02:00.4] fault addr f2682000 ...

Whilst this is a fairly rare occurrence, it can happen from the result
of intermediate scatterlist processing such as scatterwalk_ffwd() in the
crypto layer. Whilst that particular site could be fixed up, it still
seems worthwhile to bring intel-iommu in line with other DMA API
implementations in handling this robustly.

To that end, fix the intel_map_sg() path to line up the mapping
correctly (in units of MM pages rather than VT-d pages to match the
aligned_nrpages() calculation) regardless of the offset, and use
sg_phys() consistently for clarity.

Reported-by: Harsh Jain <Harsh@chelsio.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/intel-iommu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6784a05dd6b2..83f3d4831f94 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2254,10 +2254,12 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 		uint64_t tmp;
 
 		if (!sg_res) {
+			unsigned int pgoff = sg->offset & ~PAGE_MASK;
+
 			sg_res = aligned_nrpages(sg->offset, sg->length);
-			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
+			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + pgoff;
 			sg->dma_length = sg->length;
-			pteval = page_to_phys(sg_page(sg)) | prot;
+			pteval = (sg_phys(sg) - pgoff) | prot;
 			phys_pfn = pteval >> VTD_PAGE_SHIFT;
 		}
 
@@ -3790,7 +3792,7 @@ static int intel_nontranslate_map_sg(struct device *hddev,
 
 	for_each_sg(sglist, sg, nelems, i) {
 		BUG_ON(!sg_page(sg));
-		sg->dma_address = page_to_phys(sg_page(sg)) + sg->offset;
+		sg->dma_address = sg_phys(sg);
 		sg->dma_length = sg->length;
 	}
 	return nelems;
-- 
2.13.4.dirty

[toc] | [next] | [standalone]

#1741690

FromCasey Leedom <leedom@chelsio.com>
Date2017-09-28 18:20 +0200
Message-ID<uuJHA-2JP-9@gated-at.bofh.it>
In reply to#1741579
  Thanks Robin.  Harsh can certainly test your latest patch as soon as he's
back in the office tomorrow morning India time.  If your patch works and is
accepted, it sounds like the commit would be important enough to consider
backporting into various Long-Term Support releases and the affected
distributions.  What's the procedure for nominating a commit for LTS inclusion?
 
Casey

[toc] | [prev] | [next] | [standalone]

#1741695

From"Raj, Ashok" <ashok.raj@intel.com>
Date2017-09-28 18:30 +0200
Message-ID<uuJRf-2NO-5@gated-at.bofh.it>
In reply to#1741690
Hi Casey

On Thu, Sep 28, 2017 at 04:17:59PM +0000, Casey Leedom wrote:
>   Thanks Robin.  Harsh can certainly test your latest patch as soon as he's
> back in the office tomorrow morning India time.  If your patch works and is
> accepted, it sounds like the commit would be important enough to consider
> backporting into various Long-Term Support releases and the affected
> distributions.  What's the procedure for nominating a commit for LTS inclusion?

its documented in Documentation/process/submitting-patches

I didn't see a new patch fly by.. Robin, could you send that over?

Cheers,
Ashok

[toc] | [prev] | [next] | [standalone]

#1741709

FromRobin Murphy <robin.murphy@arm.com>
Date2017-09-28 19:00 +0200
Message-ID<uuKkh-2Zx-15@gated-at.bofh.it>
In reply to#1741695
On 28/09/17 14:29, Raj, Ashok wrote:
> Hi Casey
> 
> On Thu, Sep 28, 2017 at 04:17:59PM +0000, Casey Leedom wrote:
>>   Thanks Robin.  Harsh can certainly test your latest patch as soon as he's
>> back in the office tomorrow morning India time.  If your patch works and is
>> accepted, it sounds like the commit would be important enough to consider
>> backporting into various Long-Term Support releases and the affected
>> distributions.  What's the procedure for nominating a commit for LTS inclusion?

I tend to leave stable decisions up to Joerg as the subsystem
maintainer, particularly when it's code outside my usual areas of
familiarity. FWIW, from a real dig through the history, the fragile
logic seems to date from the 2.6 days, having snuck in with b536d24d212c
("intel-iommu: Clean up intel_map_sg(), remove domain_page_mapping()")

> its documented in Documentation/process/submitting-patches
> 
> I didn't see a new patch fly by.. Robin, could you send that over?

I hope our email server hasn't got blacklisted again... Said patch is
the top of this very thread we're replying on[1] - you were definitely
on cc :(

Robin.

[1]:https://lists.linuxfoundation.org/pipermail/iommu/2017-September/024371.html

[toc] | [prev] | [next] | [standalone]

#1741776

From"Raj, Ashok" <ashok.raj@intel.com>
Date2017-09-28 20:40 +0200
Message-ID<uuLT4-42q-21@gated-at.bofh.it>
In reply to#1741709
Hi Robin

thanks.. i have no idea.. i see all the other patches from you :-)
my email has decided to play games with me i suppose :-)

On Thu, Sep 28, 2017 at 05:59:11PM +0100, Robin Murphy wrote:
> I hope our email server hasn't got blacklisted again... Said patch is
> the top of this very thread we're replying on[1] - you were definitely
> on cc :(

        (sg->dma_address + sg->dma_len) ----+
        sg->dma_address ---------+          |
        iov_pfn------+           |          |
                     |           |          |
                     v           v          v
    iova:   a        b        c        d        e        f
            |--------|--------|--------|--------|--------|
                              <...calculated....>
                     [_____mapped______]
    pfn:    0        1        2        3        4        5
            |--------|--------|--------|--------|--------|
                     ^           ^          ^
                     |           |          |
        sg->page ----+           |          |
        sg->offset --------------+          |
        (sg->offset + sg->length) ----------+

The picture seems right. Looking at the code i'm not sure if i understand
it correctly.

pgoff = sg->offset & ~PAGE_MASK;

this gets the offset past the start of page.

sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + pgoff;

this would set dma_address at b+off instead of starting at c+off correct?

> 
> Robin
> 
> [1]:https://lists.linuxfoundation.org/pipermail/iommu/2017-September/024371.html

[toc] | [prev] | [next] | [standalone]

#1741966

FromHarsh Jain <Harsh@chelsio.com>
Date2017-09-29 10:20 +0200
Message-ID<uuYGC-3GT-11@gated-at.bofh.it>
In reply to#1741579
Robin,

I tried running patch on our test setup.

With "intel_iommu=on" : I can see single occurrence of DMAR Write failure on perf traffic with 10 thread.

    [  749.616480] perf: interrupt took too long (3203 > 3202), lowering kernel.perf_event_max_sample_rate to 62000
[  852.500671] DMAR: DRHD: handling fault status reg 2
[  852.506039] DMAR: [DMA Write] Request device [02:00.4] fault addr ef919000 [fault reason 05] PTE Write access is not set
[root@heptagon linux_t4_build]# cat /proc/cmdline
BOOT_IMAGE=/vmlinuz-4.9.51+ root=UUID=ccbb7f18-b3f0-43df-89de-07521e9c02fe ro intel_iommu=on crashkernel=auto rhgb quiet rhgb quiet console=ttyS0,115200, console=tty0 LANG=en_US.UTF-8

With intel_iommu=sp_off : It works fine for more than 30 minutes without any issues.


On 28-09-2017 19:44, Robin Murphy wrote:
> The intel-iommu DMA ops fail to correctly handle scatterlists where
> sg->offset is greater than PAGE_SIZE - the IOVA allocation is computed
> appropriately based on the page-aligned portion of the offset, but the
> mapping is set up relative to sg->page, which means it fails to actually
> cover the whole buffer (and in the worst case doesn't cover it at all):
>
>     (sg->dma_address + sg->dma_len) ----+
>     sg->dma_address ---------+          |
>     iov_pfn------+           |          |
>                  |           |          |
>                  v           v          v
> iova:   a        b        c        d        e        f
>         |--------|--------|--------|--------|--------|
>                           <...calculated....>
>                  [_____mapped______]
> pfn:    0        1        2        3        4        5
>         |--------|--------|--------|--------|--------|
>                  ^           ^          ^
>                  |           |          |
>     sg->page ----+           |          |
>     sg->offset --------------+          |
>     (sg->offset + sg->length) ----------+
>
> As a result, the caller ends up overrunning the mapping into whatever
> lies beyond, which usually goes badly:
>
> [  429.645492] DMAR: DRHD: handling fault status reg 2
> [  429.650847] DMAR: [DMA Write] Request device [02:00.4] fault addr f2682000 ...
>
> Whilst this is a fairly rare occurrence, it can happen from the result
> of intermediate scatterlist processing such as scatterwalk_ffwd() in the
> crypto layer. Whilst that particular site could be fixed up, it still
> seems worthwhile to bring intel-iommu in line with other DMA API
> implementations in handling this robustly.
>
> To that end, fix the intel_map_sg() path to line up the mapping
> correctly (in units of MM pages rather than VT-d pages to match the
> aligned_nrpages() calculation) regardless of the offset, and use
> sg_phys() consistently for clarity.
>
> Reported-by: Harsh Jain <Harsh@chelsio.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/intel-iommu.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 6784a05dd6b2..83f3d4831f94 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2254,10 +2254,12 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
>  		uint64_t tmp;
>  
>  		if (!sg_res) {
> +			unsigned int pgoff = sg->offset & ~PAGE_MASK;
> +
>  			sg_res = aligned_nrpages(sg->offset, sg->length);
> -			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
> +			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + pgoff;
>  			sg->dma_length = sg->length;
> -			pteval = page_to_phys(sg_page(sg)) | prot;
> +			pteval = (sg_phys(sg) - pgoff) | prot;
>  			phys_pfn = pteval >> VTD_PAGE_SHIFT;
>  		}
>  
> @@ -3790,7 +3792,7 @@ static int intel_nontranslate_map_sg(struct device *hddev,
>  
>  	for_each_sg(sglist, sg, nelems, i) {
>  		BUG_ON(!sg_page(sg));
> -		sg->dma_address = page_to_phys(sg_page(sg)) + sg->offset;
> +		sg->dma_address = sg_phys(sg);
>  		sg->dma_length = sg->length;
>  	}
>  	return nelems;

[toc] | [prev] | [next] | [standalone]

#1742231

FromCasey Leedom <leedom@chelsio.com>
Date2017-09-29 18:20 +0200
Message-ID<uv6b8-8tC-13@gated-at.bofh.it>
In reply to#1741966
| From: Harsh Jain <Harsh@chelsio.com>
| Sent: Friday, September 29, 2017 1:14:45 AM
|     
| Robin,
| 
| I tried running patch on our test setup.
| 
| With "intel_iommu=on" : I can see single occurrence of DMAR Write failure
| on perf traffic with 10 thread.
| 
| [  749.616480] perf: interrupt took too long (3203 > 3202), lowering kernel.perf_event_max_sample_rate to 62000
| [  852.500671] DMAR: DRHD: handling fault status reg 2
| [  852.506039] DMAR: [DMA Write] Request device [02:00.4] fault addr ef919000 [fault reason 05] PTE Write access is not set
| [root@heptagon linux_t4_build]# cat /proc/cmdline
| BOOT_IMAGE=/vmlinuz-4.9.51+ root=UUID=ccbb7f18-b3f0-43df-89de-07521e9c02fe ro intel_iommu=on crashkernel=auto rhgb quiet rhgb quiet console=ttyS0,115200, console=tty0 LANG=en_US.UTF-8

Harsh. Can you provide the debugging information for that one DMA FAILURE
trace?  It May be yet another corner case in __domain_mapping() or a
different path.

| With intel_iommu=sp_off : It works fine for more than 30 minutes without
| any issues.

I think that even without Robin's patch using intel_iommu=sp_off worked
without errors, right?

Casey

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web