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

Groups > linux.kernel > #1733214 > unrolled thread

DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

Back to article view | Back to linux.kernel

Contents

  DMA error when sg->offset value is greater than PAGE_SIZE in Intel  IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-16 08:20 +0200
    Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Herbert Xu <herbert@gondor.apana.org.au> - 2017-09-20 10:10 +0200
      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Robin Murphy <robin.murphy@arm.com> - 2017-09-20 12:20 +0200
        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-20 13:30 +0200
        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-25 19:50 +0200
          Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-25 20:50 +0200
            Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-25 20:50 +0200
              Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-26 05:50 +0200
                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-26 14:30 +0200
                  Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Robin Murphy <robin.murphy@arm.com> - 2017-09-26 16:30 +0200
                    Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Dan Williams <dan.j.williams@intel.com> - 2017-09-26 18:20 +0200
                      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-27 18:40 +0200
                        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Dan Williams <dan.j.williams@intel.com> - 2017-09-27 19:20 +0200
                          Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Christoph Hellwig <hch@infradead.org> - 2017-10-01 11:00 +0200
                        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-27 19:50 +0200
                          Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-27 23:30 +0200
                            Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-28 00:10 +0200
                              Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-28 00:20 +0200
                                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-28 07:10 +0200
                                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Herbert Xu <herbert@gondor.apana.org.au> - 2017-09-28 12:40 +0200
                            Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-28 15:40 +0200
                              Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-28 18:10 +0200
                                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-29 07:40 +0200
                      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-27 19:40 +0200
                    Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-26 19:30 +0200
                      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-26 23:00 +0200
                    Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-26 19:30 +0200
                      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Robin Murphy <robin.murphy@arm.com> - 2017-09-26 20:20 +0200
                    Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-26 19:40 +0200
          Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Dan Williams <dan.j.williams@intel.com> - 2017-09-25 21:40 +0200
            Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-25 22:10 +0200
              Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Dan Williams <dan.j.williams@intel.com> - 2017-09-25 22:20 +0200
                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-25 23:50 +0200
                  Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-26 01:50 +0200
                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-26 15:10 +0200
      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-20 13:40 +0200
      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU David Woodhouse <dwmw2@infradead.org> - 2017-09-25 20:50 +0200
        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-25 22:20 +0200
        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-26 13:20 +0200

Page 1 of 2  [1] 2  Next page →

#1733214 — DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

Hi,

While debugging DMA mapping error in chelsio crypto driver we observed that when scatter/gather list received by driver has some entry with page->offset > 4096 (PAGE_SIZE). It starts giving DMA error.  Without IOMMU it works fine.

Before reaching to chelsio crypto driver(driver/crypto/chelsio) following entities change the sg'

1) IN esp_output() "__skb_to_sgvec()" convert skb frags to scatter gather list. At that moment sg->offset was 4094.
2) From esp_output control reaches to "crypto_authenc_encrypt()". Here in "scatterwalk_ffwd()" sg->offset become 4110.
3) Same sg list received by chelsio crypto driver(chcr). When chcr try to do DMA mapping it starts giving DMA errors.

Following error observed. first two prints are added for debugging in chcr. Kernel version used to reproduce is 4.9.28 on x86_64.

Sep 15 12:40:52 heptagon kernel: process_cipher req src ffff8803cb41f0a8
Sep 15 12:40:52 heptagon kernel: ========= issue    hit offset:4110 ======= dma_addr f24b000e ==> DMA mapped address returned by dma_map_sg()

Sep 15 12:40:52 heptagon kernel: DMAR: DRHD: handling fault status reg 2
Sep 15 12:40:52 heptagon kernel: DMAR: [DMA Write] Request device [02:00.4] fault addr f24b0000 [fault reason 05] PTE Write access is not set

 By applying following hack in kernel. Things start working.

diff --git a/crypto/scatterwalk.c b/crypto/scatterwalk.c
index c16c94f8..1d75a3a 100644
--- a/crypto/scatterwalk.c
+++ b/crypto/scatterwalk.c
@@ -78,6 +78,8 @@ struct scatterlist *scatterwalk_ffwd(struct scatterlist dst[2]
                                     struct scatterlist *src,
                                     unsigned int len)
 {
+       unsigned int mod_page_offset;
+
        for (;;) {
                if (!len)
                        return src;
@@ -90,7 +92,9 @@ struct scatterlist *scatterwalk_ffwd(struct scatterlist dst[2]
        }

        sg_init_table(dst, 2);
-       sg_set_page(dst, sg_page(src), src->length - len, src->offset + len);
+        mod_page_offset = (src->offset + len) / PAGE_SIZE;
+       sg_set_page(dst, sg_page(src) + mod_page_offset, src->length - len,
+                   (src->offset + len) - (mod_page_offset * PAGE_SIZE));
        scatterwalk_crypto_chain(dst, sg_next(src), 0, 2);


1) We are not expecting issue in "scatterwalk_ffwd" because it is not the only place where kernel 
updates src->offset without checking page boundary. similar logic used in "__skb_to_sgvec".
 
2) It cannot be driver's responsibilty to update received sg entries to adjust offset and page 
because we are not the only one who directly uses received sg list.

3) Since Without IOMMU every thing works fine. We are expecting IOMMU bugs.

Regards

Harsh Jain

[toc] | [next] | [standalone]

#1735611 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

Harsh Jain <Harsh@chelsio.com> wrote:
> 
> While debugging DMA mapping error in chelsio crypto driver we observed that when scatter/gather list received by driver has some entry with page->offset > 4096 (PAGE_SIZE). It starts giving DMA error.  Without IOMMU it works fine.

This is not a bug.  The network stack can and will feed us such
SG lists.

> 2) It cannot be driver's responsibilty to update received sg entries to adjust offset and page 
> because we are not the only one who directly uses received sg list.

No the driver must deal with this.  Having said that, if we can
improve our driver helper interface to make this easier then we
should do that too.  What we certainly shouldn't do is to take a
whack-a-mole approach like this patch does.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

#1735693 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

On 20/09/17 09:01, Herbert Xu wrote:
> Harsh Jain <Harsh@chelsio.com> wrote:
>>
>> While debugging DMA mapping error in chelsio crypto driver we observed that when scatter/gather list received by driver has some entry with page->offset > 4096 (PAGE_SIZE). It starts giving DMA error.  Without IOMMU it works fine.
> 
> This is not a bug.  The network stack can and will feed us such
> SG lists.
> 
>> 2) It cannot be driver's responsibilty to update received sg entries to adjust offset and page 
>> because we are not the only one who directly uses received sg list.
> 
> No the driver must deal with this.  Having said that, if we can
> improve our driver helper interface to make this easier then we
> should do that too.  What we certainly shouldn't do is to take a
> whack-a-mole approach like this patch does.

AFAICS this is entirely on intel-iommu - from a brief look it appears
that all the IOVA calculations would handle the offset correctly, but
then __domain_mapping() blindly uses sg_page() for the physical address,
so if offset is larger than a page it would end up with the DMA mapping
covering the wrong part of the buffer.

Does the diff below help?

Robin.

----->8-----
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index b3914fce8254..2ed43d928135 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2253,7 +2253,7 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 			sg_res = aligned_nrpages(sg->offset, sg->length);
 			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
 			sg->dma_length = sg->length;
-			pteval = page_to_phys(sg_page(sg)) | prot;
+			pteval = (sg_phys(sg) & PAGE_MASK) | prot;
 			phys_pfn = pteval >> VTD_PAGE_SHIFT;
 		}
 

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

#1735729 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

On 20-09-2017 15:42, Robin Murphy wrote:
> On 20/09/17 09:01, Herbert Xu wrote:
>> Harsh Jain <Harsh@chelsio.com> wrote:
>>> While debugging DMA mapping error in chelsio crypto driver we observed that when scatter/gather list received by driver has some entry with page->offset > 4096 (PAGE_SIZE). It starts giving DMA error.  Without IOMMU it works fine.
>> This is not a bug.  The network stack can and will feed us such
>> SG lists.
>>
>>> 2) It cannot be driver's responsibilty to update received sg entries to adjust offset and page 
>>> because we are not the only one who directly uses received sg list.
>> No the driver must deal with this.  Having said that, if we can
>> improve our driver helper interface to make this easier then we
>> should do that too.  What we certainly shouldn't do is to take a
>> whack-a-mole approach like this patch does.
> AFAICS this is entirely on intel-iommu - from a brief look it appears
> that all the IOVA calculations would handle the offset correctly, but
> then __domain_mapping() blindly uses sg_page() for the physical address,
> so if offset is larger than a page it would end up with the DMA mapping
> covering the wrong part of the buffer.
>
> Does the diff below help?
>
> Robin.
>
> ----->8-----
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index b3914fce8254..2ed43d928135 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2253,7 +2253,7 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
>  			sg_res = aligned_nrpages(sg->offset, sg->length);
>  			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
>  			sg->dma_length = sg->length;
> -			pteval = page_to_phys(sg_page(sg)) | prot;
> +			pteval = (sg_phys(sg) & PAGE_MASK) | prot;
>  			phys_pfn = pteval >> VTD_PAGE_SHIFT;
>  		}
Robin,
Still having following error with above change.

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


>  

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

#1739194 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

| From: Robin Murphy <robin.murphy@arm.com>
| Sent: Wednesday, September 20, 2017 3:12 AM
|
| On 20/09/17 09:01, Herbert Xu wrote:
| >
| > Harsh Jain <Harsh@chelsio.com> wrote:
| >>
| >> While debugging DMA mapping error in chelsio crypto driver we
| >> observed that when scatter/gather list received by driver has
| >> some entry with page->offset > 4096 (PAGE_SIZE). It starts
| >> giving DMA error.  Without IOMMU it works fine.
| >
| > This is not a bug.  The network stack can and will feed us such
| > SG lists.
| >
| >> 2) It cannot be driver's responsibilty to update received sg
| >> entries to adjust offset and page because we are not the only
| >> one who directly uses received sg list.
| >
| > No the driver must deal with this.  Having said that, if we can
| > improve our driver helper interface to make this easier then we
| > should do that too.  What we certainly shouldn't do is to take a
| > whack-a-mole approach like this patch does.
|
| AFAICS this is entirely on intel-iommu - from a brief look it appears
| that all the IOVA calculations would handle the offset correctly, but
| then __domain_mapping() blindly uses sg_page() for the physical address,
| so if offset is larger than a page it would end up with the DMA mapping
| covering the wrong part of the buffer.
|
| Does the diff below help?
|
| Robin.
|
| ----->8-----
| diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
| index b3914fce8254..2ed43d928135 100644
| --- a/drivers/iommu/intel-iommu.c
| +++ b/drivers/iommu/intel-iommu.c
| @@ -2253,7 +2253,7 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
|                          sg_res = aligned_nrpages(sg->offset, sg->length);
|                          sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
|                          sg->dma_length = sg->length;
| -                       pteval = page_to_phys(sg_page(sg)) | prot;
| +                       pteval = (sg_phys(sg) & PAGE_MASK) | prot;
|                          phys_pfn = pteval >> VTD_PAGE_SHIFT;
|                  }

  Adding some likely people to the Cc list so they can comment on this.
Dan Williams submitted that specific piece of code in kernel.org:3e6110fd54
... but there are lots of similar bits in that function.  Hopefully one of
the Intel I/O MMU Gurus will have a better idea of what may be going wrong
here.  In the mean time I've asked our team to gather far more detailed
debug traces showing the exact Scatter/Gather Lists we're getting, what they
get translated to in the DMA Mappings, and what DMA Addresses were seeing in
error.

Casey

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

#1739216 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

Hi Casey

Sorry, somehow didn't see this one come by.


On Mon, Sep 25, 2017 at 05:46:40PM +0000, Casey Leedom wrote:
> | From: Robin Murphy <robin.murphy@arm.com>
> | Sent: Wednesday, September 20, 2017 3:12 AM
> |
> | On 20/09/17 09:01, Herbert Xu wrote:
> | >
> | > Harsh Jain <Harsh@chelsio.com> wrote:
> | >>
> | >> While debugging DMA mapping error in chelsio crypto driver we
> | >> observed that when scatter/gather list received by driver has
> | >> some entry with page->offset > 4096 (PAGE_SIZE). It starts
> | >> giving DMA error.  Without IOMMU it works fine.

Not sure how the page->offset would end up being greater than page-size?

If you have additional traces, please send them by. 

Is this a new driver? wondering how we didn't run into this?


Cheers,
Ashok

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

#1739221 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

| From: Raj, Ashok <ashok.raj@intel.com>
| Sent: Monday, September 25, 2017 8:54 AM
|
| Not sure how the page->offset would end up being greater than page-size?
|
| If you have additional traces, please send them by.
|
| Is this a new driver? wondering how we didn't run into this?

  According to Herbert Xu and one of our own engineers, it's actually legal
for Scatter/Gather Lists to have this.  This isn't my area of expertise
though so I'm just passing that on.

  I've asked our team to produce a detailed trace of the exact
Scatter/Gather Lists they're seeing and what ends up coming out of the DMA
Mappings, etc.  They're in India, so I expect that they'll have this for you
by tomorrow morning.

Casey

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

#1739493 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

On 26-09-2017 00:16, Casey Leedom wrote:
> | From: Raj, Ashok <ashok.raj@intel.com>
> | Sent: Monday, September 25, 2017 8:54 AM
> |
> | Not sure how the page->offset would end up being greater than page-size?
Refer below
> |
> | If you have additional traces, please send them by.
> |
> | Is this a new driver? wondering how we didn't run into this?
>
>   According to Herbert Xu and one of our own engineers, it's actually legal
> for Scatter/Gather Lists to have this.  This isn't my area of expertise
> though so I'm just passing that on.
>
>   I've asked our team to produce a detailed trace of the exact
> Scatter/Gather Lists they're seeing and what ends up coming out of the DMA
> Mappings, etc.  They're in India, so I expect that they'll have this for you
> by tomorrow morning.
Below mentioned log was already there in 1st mail. Copied here for easy reference. Let me know if you need
additional traces.

1) IN esp_output() "__skb_to_sgvec()" convert skb frags to scatter gather list. 
At that moment sg->offset was 4094.
2) From esp_output control reaches to "crypto_authenc_encrypt()". Here in 
"scatterwalk_ffwd()" sg->offset become 4110.
3) Same sg list received by chelsio crypto driver(chcr). When chcr try to do 
DMA mapping it starts giving DMA errors.

Following error observed. first two prints are added for debugging in chcr. 
Kernel version used to reproduce is 4.9.28 on x86_64 with Page size 4K.

Sep 15 12:40:52 heptagon kernel: process_cipher req src ffff8803cb41f0a8
Sep 15 12:40:52 heptagon kernel: ========= issue    hit offset:4110 ======= 
dma_addr f24b000e ==> DMA mapped address returned by dma_map_sg()

Sep 15 12:40:52 heptagon kernel: DMAR: DRHD: handling fault status reg 2
Sep 15 12:40:52 heptagon kernel: DMAR: [DMA Write] Request device [02:00.4] 
fault addr f24b0000 [fault reason 05] PTE Write access is not set

>
> Casey

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

#1739837 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

[Multipart message — attachments visible in raw view] — view raw

Find attached new set of log. After repeated tries it panics.


On 26-09-2017 09:16, Harsh Jain wrote:
> On 26-09-2017 00:16, Casey Leedom wrote:
>> | From: Raj, Ashok <ashok.raj@intel.com>
>> | Sent: Monday, September 25, 2017 8:54 AM
>> |
>> | Not sure how the page->offset would end up being greater than page-size?
> Refer below
>> |
>> | If you have additional traces, please send them by.
>> |
>> | Is this a new driver? wondering how we didn't run into this?
>>
>>   According to Herbert Xu and one of our own engineers, it's actually legal
>> for Scatter/Gather Lists to have this.  This isn't my area of expertise
>> though so I'm just passing that on.
>>
>>   I've asked our team to produce a detailed trace of the exact
>> Scatter/Gather Lists they're seeing and what ends up coming out of the DMA
>> Mappings, etc.  They're in India, so I expect that they'll have this for you
>> by tomorrow morning.
> Below mentioned log was already there in 1st mail. Copied here for easy reference. Let me know if you need
> additional traces.
>
> 1) IN esp_output() "__skb_to_sgvec()" convert skb frags to scatter gather list. 
> At that moment sg->offset was 4094.
> 2) From esp_output control reaches to "crypto_authenc_encrypt()". Here in 
> "scatterwalk_ffwd()" sg->offset become 4110.
> 3) Same sg list received by chelsio crypto driver(chcr). When chcr try to do 
> DMA mapping it starts giving DMA errors.
>
> Following error observed. first two prints are added for debugging in chcr. 
> Kernel version used to reproduce is 4.9.28 on x86_64 with Page size 4K.
>
> Sep 15 12:40:52 heptagon kernel: process_cipher req src ffff8803cb41f0a8
> Sep 15 12:40:52 heptagon kernel: ========= issue    hit offset:4110 ======= 
> dma_addr f24b000e ==> DMA mapped address returned by dma_map_sg()
>
> Sep 15 12:40:52 heptagon kernel: DMAR: DRHD: handling fault status reg 2
> Sep 15 12:40:52 heptagon kernel: DMAR: [DMA Write] Request device [02:00.4] 
> fault addr f24b0000 [fault reason 05] PTE Write access is not set
>
>> Casey

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

#1739936 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

On 26/09/17 13:21, Harsh Jain wrote:
> Find attached new set of log. After repeated tries it panics.

Thanks, that makes things a bit clearer - looks like fixing the physical
address/pteval calculation to not be off by a page in one direction wasn't
helping much because the returned DMA address is actually also off by a
page in the other direction, and thus overflowing past the allocated IOVA
into whoever else's mapping happened to be there; complete carnage ensues.

After another look through the intel_map_sg() path, here's my second (still
completely untested) guess at a possible fix.

Robin.

----->8-----
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6784a05dd6b2..d7f7def81613 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) {
+			size_t off = 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) + off;
 			sg->dma_length = sg->length;
-			pteval = page_to_phys(sg_page(sg)) | prot;
+			pteval = (page_to_phys(sg_page(sg)) + sg->offset - off) | prot;
 			phys_pfn = pteval >> VTD_PAGE_SHIFT;
 		}
 
> 
> 
> On 26-09-2017 09:16, Harsh Jain wrote:
>> On 26-09-2017 00:16, Casey Leedom wrote:
>>> | From: Raj, Ashok <ashok.raj@intel.com>
>>> | Sent: Monday, September 25, 2017 8:54 AM
>>> |
>>> | Not sure how the page->offset would end up being greater than page-size?
>> Refer below
>>> |
>>> | If you have additional traces, please send them by.
>>> |
>>> | Is this a new driver? wondering how we didn't run into this?
>>>
>>>   According to Herbert Xu and one of our own engineers, it's actually legal
>>> for Scatter/Gather Lists to have this.  This isn't my area of expertise
>>> though so I'm just passing that on.
>>>
>>>   I've asked our team to produce a detailed trace of the exact
>>> Scatter/Gather Lists they're seeing and what ends up coming out of the DMA
>>> Mappings, etc.  They're in India, so I expect that they'll have this for you
>>> by tomorrow morning.
>> Below mentioned log was already there in 1st mail. Copied here for easy reference. Let me know if you need
>> additional traces.
>>
>> 1) IN esp_output() "__skb_to_sgvec()" convert skb frags to scatter gather list. 
>> At that moment sg->offset was 4094.
>> 2) From esp_output control reaches to "crypto_authenc_encrypt()". Here in 
>> "scatterwalk_ffwd()" sg->offset become 4110.
>> 3) Same sg list received by chelsio crypto driver(chcr). When chcr try to do 
>> DMA mapping it starts giving DMA errors.
>>
>> Following error observed. first two prints are added for debugging in chcr. 
>> Kernel version used to reproduce is 4.9.28 on x86_64 with Page size 4K.
>>
>> Sep 15 12:40:52 heptagon kernel: process_cipher req src ffff8803cb41f0a8
>> Sep 15 12:40:52 heptagon kernel: ========= issue    hit offset:4110 ======= 
>> dma_addr f24b000e ==> DMA mapped address returned by dma_map_sg()
>>
>> Sep 15 12:40:52 heptagon kernel: DMAR: DRHD: handling fault status reg 2
>> Sep 15 12:40:52 heptagon kernel: DMAR: [DMA Write] Request device [02:00.4] 
>> fault addr f24b0000 [fault reason 05] PTE Write access is not set
>>
>>> Casey
> 

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

#1740004 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

On Tue, Sep 26, 2017 at 9:06 AM, Casey Leedom <leedom@chelsio.com> wrote:
> | From: Robin Murphy <robin.murphy@arm.com>
> | Sent: Tuesday, September 26, 2017 7:22 AM
>
> |
> | On 26/09/17 13:21, Harsh Jain wrote:
> | > Find attached new set of log. After repeated tries it panics.
> |
> | Thanks, that makes things a bit clearer - looks like fixing the physical
> | address/pteval calculation to not be off by a page in one direction wasn't
> | helping much because the returned DMA address is actually also off by a
> | page in the other direction, and thus overflowing past the allocated IOVA
> | into whoever else's mapping happened to be there; complete carnage ensues.
> |
> | After another look through the intel_map_sg() path, here's my second
> (still
> | completely untested) guess at a possible fix.
> |
> | Robin.
> |
> | ----->8-----
> | diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> | index 6784a05dd6b2..d7f7def81613 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) {
> | +                       size_t off = 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) + off;
> |                          sg->dma_length = sg->length;
> | -                       pteval = page_to_phys(sg_page(sg)) | prot;
> | +                       pteval = (page_to_phys(sg_page(sg)) + sg->offset -
> off) | prot;
> |                          phys_pfn = pteval >> VTD_PAGE_SHIFT;
> |                  }
>
>   Thanks Robin.  And thanks Harsh for sending the detailed trace logs.  I'll
> see if I can get this tested today.  Harsh is probably headed towards bed,
> but there may be sufficiently good instructions in our internal bug system
> to reproduce the issue.
>
>   Regardless, it seems that you agree that there's an issue with the Intel
> I/O MMU support code with regard to the legal values which a (struct
> scatterlist) can take on?  I still can't find any documentation for this
> and, personally, I'm a bit baffled by a Page-oriented Scatter/Gather List
> representation where [Offset, Offset+Length) can reside outside the Page.

Consider the case where the page represents a huge page, then an
offset greater than PAGE_SIZE (up to HPAGE_SIZE) makes sense.

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

#1740878 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

| From: Dan Williams <dan.j.williams@intel.com>
| Sent: Tuesday, September 26, 2017 9:10 AM
|   
| On Tue, Sep 26, 2017 at 9:06 AM, Casey Leedom <leedom@chelsio.com> wrote:
| > | From: Robin Murphy <robin.murphy@arm.com>
| > | Sent: Tuesday, September 26, 2017 7:22 AM
| > |...
| > ...
| >   Regardless, it seems that you agree that there's an issue with the Intel
| > I/O MMU support code with regard to the legal values which a (struct
| > scatterlist) can take on?  I still can't find any documentation for this
| > and, personally, I'm a bit baffled by a Page-oriented Scatter/Gather List
| > representation where [Offset, Offset+Length) can reside outside the Page.
|
| Consider the case where the page represents a huge page, then an
| offset greater than PAGE_SIZE (up to HPAGE_SIZE) makes sense.

  Okay, but whatever the underlaying Page Size is, should [Offset,
Offset+Length) completely reside within the referenced Page?  I'm just
trying to understand the Invariance Conditions which are assumed by all of
the code which processes Scatter/gather Lists ...

Casey

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

#1740898 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

On Wed, Sep 27, 2017 at 9:31 AM, Casey Leedom <leedom@chelsio.com> wrote:
> | From: Dan Williams <dan.j.williams@intel.com>
> | Sent: Tuesday, September 26, 2017 9:10 AM
> |
> | On Tue, Sep 26, 2017 at 9:06 AM, Casey Leedom <leedom@chelsio.com> wrote:
> | > | From: Robin Murphy <robin.murphy@arm.com>
> | > | Sent: Tuesday, September 26, 2017 7:22 AM
> | > |...
> | > ...
> | >   Regardless, it seems that you agree that there's an issue with the Intel
> | > I/O MMU support code with regard to the legal values which a (struct
> | > scatterlist) can take on?  I still can't find any documentation for this
> | > and, personally, I'm a bit baffled by a Page-oriented Scatter/Gather List
> | > representation where [Offset, Offset+Length) can reside outside the Page.
> |
> | Consider the case where the page represents a huge page, then an
> | offset greater than PAGE_SIZE (up to HPAGE_SIZE) makes sense.
>
>   Okay, but whatever the underlaying Page Size is, should [Offset,
> Offset+Length) completely reside within the referenced Page?  I'm just
> trying to understand the Invariance Conditions which are assumed by all of
> the code which processes Scatter/gather Lists ...

As far as I can see "Offset can be greater than PAGE_SIZE" is the only
safe assumption for core code.

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

#1742755 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

On Wed, Sep 27, 2017 at 10:13:51AM -0700, Dan Williams wrote:
> As far as I can see "Offset can be greater than PAGE_SIZE" is the only
> safe assumption for core code.

It seems completely bogus to me, but if it is the current assumption
we'll have to document it.  But this brings me back to that
our scatterlists are a pretty horrible data structure to start
with as they try to mix virtual and physical addressing together.

We'd be much better of by passing a chain of bio_vecs where we
just need virtual addresses, a chain of [bus_addr,len] pairs where
we just need a physical address, and both where we need both instead
of this giant structure that tries to do both at the same time..

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

#1740921 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

Hi Robin

On Wed, Sep 27, 2017 at 06:18:02PM +0100, Robin Murphy wrote:
> On Wed, 27 Sep 2017 16:31:04 +0000
> Casey Leedom <leedom@chelsio.com> wrote:
> 
> > | From: Dan Williams <dan.j.williams@intel.com>
> > | Sent: Tuesday, September 26, 2017 9:10 AM
> > |   
> > | On Tue, Sep 26, 2017 at 9:06 AM, Casey Leedom <leedom@chelsio.com>
> > wrote: | > | From: Robin Murphy <robin.murphy@arm.com>
> > | > | Sent: Tuesday, September 26, 2017 7:22 AM
> > | > |...
> > | > ...
> > | >   Regardless, it seems that you agree that there's an issue with
> > the Intel | > I/O MMU support code with regard to the legal values
> > which a (struct | > scatterlist) can take on?  I still can't find any
> > documentation for this | > and, personally, I'm a bit baffled by a
> > Page-oriented Scatter/Gather List | > representation where [Offset,
> > Offset+Length) can reside outside the Page. |
> > | Consider the case where the page represents a huge page, then an
> > | offset greater than PAGE_SIZE (up to HPAGE_SIZE) makes sense.
> > 
> >   Okay, but whatever the underlaying Page Size is, should [Offset,
> > Offset+Length) completely reside within the referenced Page?  I'm just
> > trying to understand the Invariance Conditions which are assumed by
> > all of the code which processes Scatter/gather Lists ...
> 
> From my experience, in general terms each scatterlist segment
> represents some contiguous quantity of pages, of which sg->page is the
> first, while sg->length and sg->offset describe the specific bounds of
> that segment's data. As such, the length may certainly (and frequently
> does) exceed PAGE_SIZE; for the offset, it's unlikely that the producer
> would initially construct one greater than PAGE_SIZE instead of just
> pointing sg->page further forward, but it seems reasonable for it to
> come about if some intermediate subsystem is processing an existing
> list in-place (as seems to be the case with crypto here).
> 
> My opinion is that this may be a slightly unusual case, but I would
> not consider it an illegal one. I think most DMA mapping
> implementations would handle it whether intentionally or not.

In this specific case, it appears that 

scatterwalk_ffwd()->sg_set_page()

sg_set_page(dst, sg_page(src), src->length - len, src->offset + len);

and 

static inline void sg_set_page(struct scatterlist *sg, struct page *page,
                   unsigned int len, unsigned int offset)
{
    sg_assign_page(sg, page);
    sg->offset = offset;
    sg->length = len;
}

The src->offset + len seems to be the culprit putting it past the page.
Looks like in the cases when it breaks, the offset is already towards
the end of page.. and adding the len, puts it over the limit.

When dealing with the offset > PAGE_SIZE, is the expectation you have another
additional entry for sgl? for e.g.

if sg->page = X, and offset=4092. and len = 16. Since IOMMU only understands
4K pages this last entry needs to be adjusted?

I'm not sure if the offset+len is a buffer overflow situation or just 
trips IOMMU. 

Cheers,
Ashok

the scatter gather list, should we 

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

#1741031 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

Hey Raj,

  Let us know if you need help in gathering more debugging information.  For
the time being we've decided to ERRATA the use of the Intel I/O MMU with
IPsec till we Root Cause the issue.  But this is still at the top of Harsh's
bug list.
 
  With Robin's comments, I'm almost sure that the:

    (iov_pfn + sg->offset) << VTD_PAGE_SHIFT)

in your suggested patch is an issue.  iov_pfn is a Page Frame Number and
sg->offset is a Byte Offset.  It feels like this should be:

    size_t page_off = sg->offset & ~VTD_PAGE_MASK;
    unsigned long pfn_off = sg->offset >> VTD_PAGE_MASK;
    ...
    sg->dma_address = ((dma_addr_t)
                       (iov_pfn + pfn_off) << VTD_PAGE_SHIFT) + page_off;

When Harsh tried your original patch, Harsh' test system wouldn't even boot.

Casey

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

#1741060 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

Hi Casey

looking at the debug output i got from Harsh it still looks like
a bug in the code. 

[  538.284589] __domain_mapping nr_pages 0x1
[  538.284600] __domain_mapping sg_res 0x1 sg->dma_address 0xf291000e dma len 0x38 pteval 0x3cbce3003 phys_pfn 0x3cbce3
[  538.284604] chelsio driver - offset 4110 len 56 dma addr f291000e dma len 56
[  538.284667] DMAR: DRHD: handling fault status reg 2
[  538.290017] DMAR: [DMA Write] Request device [02:00.4] fault addr f2910000 [fault reason 05] PTE Write access is not set

somehow when crypto_authenc_encrypt() -> scatterwalk_ffwd()-> sg_set_page()

->sg_set_page(dst, sg_page(src), src->length - len, src->offset + len);

src->offset + len gets set as sg->offset in sg_set_page(). Either the 
assumption that there should be room is incorrect, or some higher order crypto
code that ends up setting the offset did the wrong calculation.

if src->offset is already towards the end of the page, then offset+len will
go beyond the end of page.


On Wed, Sep 27, 2017 at 09:29:23PM +0000, Casey Leedom wrote:
> Hey Raj,
> 
>   Let us know if you need help in gathering more debugging information.  For
> the time being we've decided to ERRATA the use of the Intel I/O MMU with
> IPsec till we Root Cause the issue.  But this is still at the top of Harsh's
> bug list.
>  
>   With Robin's comments, I'm almost sure that the:
> 
>     (iov_pfn + sg->offset) << VTD_PAGE_SHIFT)

true, but this is the IOVA- IO Virtual address generated by the 
dma_map call. Thought in cases when sg->offset is beyond a page, then 
the new iov_pfn should fall on the next page. But we can't randomly adjust
here, unless IOMMU has also allocated IOVA for the page overflow.

Cheers,
Ashok

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

#1741070 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

| From: Raj, Ashok <ashok.raj@intel.com>
| Sent: Wednesday, September 27, 2017 12:07 PM
|
| looking at the debug output i got from Harsh it still looks like a bug in
| the code.
|
| [  538.284589] __domain_mapping nr_pages 0x1
| [ 538.284600] __domain_mapping sg_res 0x1 sg->dma_address 0xf291000e dma len
| 0x38 pteval 0x3cbce3003 phys_pfn 0x3cbce3
| [ 538.284604] chelsio driver - offset 4110 len 56 dma addr f291000e dma len
| 56
| [  538.284667] DMAR: DRHD: handling fault status reg 2
| [ 538.290017] DMAR: [DMA Write] Request device [02:00.4] fault addr f2910000
| [fault reason 05] PTE Write access is not set
|
| somehow when crypto_authenc_encrypt() -> scatterwalk_ffwd()-> sg_set_page()
|
| ->sg_set_page(dst, sg_page(src), src->length - len, src->offset + len);
|
| src->offset + len gets set as sg->offset in sg_set_page(). Either the
| assumption that there should be room is incorrect, or some higher order
| crypto
| code that ends up setting the offset did the wrong calculation.
|
| if src->offset is already towards the end of the page, then offset+len will
| go beyond the end of page.

  Hhmmm, it seems like we need Herbert to comment on this.

  Herbert, is there any specific debugging information that you'd like to
see here?

Casey

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

#1741200 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

On 28-09-2017 03:43, Casey Leedom wrote:
> | From: Raj, Ashok <ashok.raj@intel.com>
> | Sent: Wednesday, September 27, 2017 12:07 PM
> |
> | looking at the debug output i got from Harsh it still looks like a bug in
> | the code.
> |
> | [  538.284589] __domain_mapping nr_pages 0x1
> | [ 538.284600] __domain_mapping sg_res 0x1 sg->dma_address 0xf291000e dma len
> | 0x38 pteval 0x3cbce3003 phys_pfn 0x3cbce3
> | [ 538.284604] chelsio driver - offset 4110 len 56 dma addr f291000e dma len
> | 56
> | [  538.284667] DMAR: DRHD: handling fault status reg 2
> | [ 538.290017] DMAR: [DMA Write] Request device [02:00.4] fault addr f2910000
> | [fault reason 05] PTE Write access is not set
> |
> | somehow when crypto_authenc_encrypt() -> scatterwalk_ffwd()-> sg_set_page()
> |
> | ->sg_set_page(dst, sg_page(src), src->length - len, src->offset + len);
> |
> | src->offset + len gets set as sg->offset in sg_set_page(). Either the
> | assumption that there should be room is incorrect, or some higher order
> | crypto
Input received from user(Here XFRM) contains AAD(Additional Authentication data) || DATA(enc/dec) || Tag(hash). before passing input
to Cipher engine(chelsio) crypto_authenc_encrypt has to skip AAD which is 16 in our case. To skip that 16 bytes they simply incremented offset by 16. 
I think Robin is right DMA mapping should handle it.
> | code that ends up setting the offset did the wrong calculation.
> |
> | if src->offset is already towards the end of the page, then offset+len will
> | go beyond the end of page.
>
>   Hhmmm, it seems like we need Herbert to comment on this.
>
>   Herbert, is there any specific debugging information that you'd like to
> see here?
>
> Casey

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

#1741403 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

On Wed, Sep 27, 2017 at 10:13:04PM +0000, Casey Leedom wrote:
> | From: Raj, Ashok <ashok.raj@intel.com>
> | Sent: Wednesday, September 27, 2017 12:07 PM
> |
> | looking at the debug output i got from Harsh it still looks like a bug in
> | the code.
> |
> | [  538.284589] __domain_mapping nr_pages 0x1
> | [ 538.284600] __domain_mapping sg_res 0x1 sg->dma_address 0xf291000e dma len
> | 0x38 pteval 0x3cbce3003 phys_pfn 0x3cbce3
> | [ 538.284604] chelsio driver - offset 4110 len 56 dma addr f291000e dma len
> | 56
> | [  538.284667] DMAR: DRHD: handling fault status reg 2
> | [ 538.290017] DMAR: [DMA Write] Request device [02:00.4] fault addr f2910000
> | [fault reason 05] PTE Write access is not set
> |
> | somehow when crypto_authenc_encrypt() -> scatterwalk_ffwd()-> sg_set_page()
> |
> | ->sg_set_page(dst, sg_page(src), src->length - len, src->offset + len);
> |
> | src->offset + len gets set as sg->offset in sg_set_page(). Either the
> | assumption that there should be room is incorrect, or some higher order
> | crypto
> | code that ends up setting the offset did the wrong calculation.
> |
> | if src->offset is already towards the end of the page, then offset+len will
> | go beyond the end of page.
> 
>   Hhmmm, it seems like we need Herbert to comment on this.
> 
>   Herbert, is there any specific debugging information that you'd like to
> see here?

OK I was mistaken.  While SG lists can contain entries that are
larger than PAGE_SIZE, there is no reason why scatterwalk_ffwd
should gratuitously insert a page_offset that is greater than
PAGE_SIZE.

Harsh, can you please submit your original patch with a sign-off?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web