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

Groups > linux.kernel > #1740004 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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

#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] | [next] | [standalone]

#1740878

| 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

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

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

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

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

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

| 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

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

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]

#1741563

On 28-09-2017 02:59, 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)
>
> 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.
Today I tried with "Intel_iommu=sp_off" boot option. Traffic runs without any error for more than 1 hrs. What magic this option did? :)
>
> Casey

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

#1741682

Thanks for trying that Harsh. 

sp_off turns of super page support. Which this mode, do you still see offsets greater than 4k?

On Thu, Sep 28, 2017 at 07:08:21PM +0530, Harsh Jain wrote:
> 
> 
> Today I tried with "Intel_iommu=sp_off" boot option. Traffic runs without any error for more than 1 hrs. What magic this option did? :)

Cheers,
Ashok

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

#1741927

On 28-09-2017 18:35, Raj, Ashok wrote:
> Thanks for trying that Harsh. 
>
> sp_off turns of super page support. Which this mode, do you still see offsets greater than 4k?
Yes, offset greater than 4k is still there. Refer below.
[56732.774872] offset 4110 len 76 dma addr 3a531200e dma len 76
[56732.804187] offset 4110 len 84 dma addr 3a63b200e dma len 84
[56732.805104] offset 4110 len 68 dma addr 3a531200e dma len 68
[56732.806870] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.808987] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.811215] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.813155] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.814823] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.816481] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.818159] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.819712] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.821629] offset 4110 len 56 dma addr 3a531200e dma len 56
[root@heptagon linux_t4_build]#
[root@heptagon linux_t4_build]#
[root@heptagon linux_t4_build]# cat /proc/cmdline
BOOT_IMAGE=/vmlinuz-4.9.51 root=UUID=ccbb7f18-b3f0-43df-89de-07521e9c02fe ro intel_iommu=sp_off crashkernel=auto rhgb quiet rhgb quiet console=ttyS0,115200, console=tty0 LANG=en_US.UTF-8

>
> On Thu, Sep 28, 2017 at 07:08:21PM +0530, Harsh Jain wrote:
>>
>> Today I tried with "Intel_iommu=sp_off" boot option. Traffic runs without any error for more than 1 hrs. What magic this option did? :)
> Cheers,
> Ashok

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

#1740914

| From: Robin Murphy <robin.murphy@arm.com>
| Sent: Wednesday, September 27, 2017 10:18 AM
| 
| 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. ...

  Okay, thanks Robin.  That'll help me in my reviews of your and Ashok's
suggested changes to the Intel I/O MMU __domain_mapping() routine.

Casey

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web