[PATCH] ion: scatterlist offset not used for buffer map

[PATCH] ion: scatterlist offset not used for buffer map

[John Einar Reitan]

ion's default user/kernel page mapping code don't honor the offset
option for scatterlists. It uses sg_page and expect the whole page to be
mapped, while the offset could dictate an offset within a large page.

sg_phys correctly accounts for the offset, so should be used instead.

Signed-off-by: John Einar Reitan <john.reitan@foss.arm.com>
---
 drivers/staging/android/ion/ion_heap.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c
index ca15a87..e83002dc 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -47,7 +47,7 @@ void *ion_heap_map_kernel(struct ion_heap *heap,
 
 	for_each_sg(table->sgl, sg, table->nents, i) {
 		int npages_this_entry = PAGE_ALIGN(sg->length) / PAGE_SIZE;
-		struct page *page = sg_page(sg);
+		struct page *page = pfn_to_page(PFN_DOWN(sg_phys(sg)));
 
 		BUG_ON(i >= npages);
 		for (j = 0; j < npages_this_entry; j++)
@@ -79,7 +79,6 @@ int ion_heap_map_user(struct ion_heap *heap, struct ion_buffer *buffer,
 	int ret;
 
 	for_each_sg(table->sgl, sg, table->nents, i) {
-		struct page *page = sg_page(sg);
 		unsigned long remainder = vma->vm_end - addr;
 		unsigned long len = sg->length;
 
@@ -87,18 +86,18 @@ int ion_heap_map_user(struct ion_heap *heap, struct ion_buffer *buffer,
 			offset -= sg->length;
 			continue;
 		} else if (offset) {
-			page += offset / PAGE_SIZE;
 			len = sg->length - offset;
-			offset = 0;
 		}
 		len = min(len, remainder);
-		ret = remap_pfn_range(vma, addr, page_to_pfn(page), len,
-				vma->vm_page_prot);
+		ret = remap_pfn_range(vma, addr, PFN_DOWN(sg_phys(sg) + offset),
+				      len, vma->vm_page_prot);
 		if (ret)
 			return ret;
 		addr += len;
 		if (addr >= vma->vm_end)
 			return 0;
+
+		offset = 0;
 	}
 	return 0;
 }
-- 
2.7.3

Re: [PATCH] ion: scatterlist offset not used for buffer map

[Laura Abbott]

On 04/07/2016 04:29 AM, John Einar Reitan wrote:
> ion's default user/kernel page mapping code don't honor the offset
> option for scatterlists. It uses sg_page and expect the whole page to be
> mapped, while the offset could dictate an offset within a large page.
>
> sg_phys correctly accounts for the offset, so should be used instead.
>

Can you be more specific about which heap and which allocation pattern
is exposing this bug?

> Signed-off-by: John Einar Reitan <john.reitan@foss.arm.com>
> ---
>   drivers/staging/android/ion/ion_heap.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c
> index ca15a87..e83002dc 100644
> --- a/drivers/staging/android/ion/ion_heap.c
> +++ b/drivers/staging/android/ion/ion_heap.c
> @@ -47,7 +47,7 @@ void *ion_heap_map_kernel(struct ion_heap *heap,
>
>   	for_each_sg(table->sgl, sg, table->nents, i) {
>   		int npages_this_entry = PAGE_ALIGN(sg->length) / PAGE_SIZE;
> -		struct page *page = sg_page(sg);
> +		struct page *page = pfn_to_page(PFN_DOWN(sg_phys(sg)));
>   
>   		BUG_ON(i >= npages);
>   		for (j = 0; j < npages_this_entry; j++)
> @@ -79,7 +79,6 @@ int ion_heap_map_user(struct ion_heap *heap, struct ion_buffer *buffer,
>   	int ret;
>
>   	for_each_sg(table->sgl, sg, table->nents, i) {
> -		struct page *page = sg_page(sg);
>   		unsigned long remainder = vma->vm_end - addr;
>   		unsigned long len = sg->length;
>
> @@ -87,18 +86,18 @@ int ion_heap_map_user(struct ion_heap *heap, struct ion_buffer *buffer,
>   			offset -= sg->length;
>   			continue;
>   		} else if (offset) {
> -			page += offset / PAGE_SIZE;
>   			len = sg->length - offset;
> -			offset = 0;
>   		}
>   		len = min(len, remainder);
> -		ret = remap_pfn_range(vma, addr, page_to_pfn(page), len,
> -				vma->vm_page_prot);
> +		ret = remap_pfn_range(vma, addr, PFN_DOWN(sg_phys(sg) + offset),
> +				      len, vma->vm_page_prot);
>   		if (ret)
>   			return ret;
>   		addr += len;
>   		if (addr >= vma->vm_end)
>   			return 0;
> +
> +		offset = 0;
>   	}
>   	return 0;
>   }
>

Re: [PATCH] ion: scatterlist offset not used for buffer map

[John Einar Reitan]

On Thu, Apr 07, 2016 at 12:37:50PM -0700, Laura Abbott wrote:
> On 04/07/2016 04:29 AM, John Einar Reitan wrote:
> > ion's default user/kernel page mapping code don't honor the offset
> > option for scatterlists. It uses sg_page and expect the whole page to be
> > mapped, while the offset could dictate an offset within a large page.
> >
> > sg_phys correctly accounts for the offset, so should be used instead.
> >
> 
> Can you be more specific about which heap and which allocation pattern
> is exposing this bug?

The heap that exposed the bug is one I'm developing and will be posting
as a RFC soon. It uses compound pages and an sub-divides it into surface
buffers. The ion buffers are configured to hold sgl's with the compound
page and the correct offset of the buffer, via 
sg_set_page(.., compound_page, .., offset_of_logical_buffer);

sg_phys/sg_virt  includes this offset, but if you poke the sg and extract
the page with sg_page yourself you must include this offset in your
calculations too.

Re: [PATCH] ion: scatterlist offset not used for buffer map

[Laura Abbott]

On 04/07/2016 11:56 PM, John Einar Reitan wrote:
> On Thu, Apr 07, 2016 at 12:37:50PM -0700, Laura Abbott wrote:
>> On 04/07/2016 04:29 AM, John Einar Reitan wrote:
>>> ion's default user/kernel page mapping code don't honor the offset
>>> option for scatterlists. It uses sg_page and expect the whole page to be
>>> mapped, while the offset could dictate an offset within a large page.
>>>
>>> sg_phys correctly accounts for the offset, so should be used instead.
>>>
>>
>> Can you be more specific about which heap and which allocation pattern
>> is exposing this bug?
>
> The heap that exposed the bug is one I'm developing and will be posting
> as a RFC soon. It uses compound pages and an sub-divides it into surface
> buffers. The ion buffers are configured to hold sgl's with the compound
> page and the correct offset of the buffer, via
> sg_set_page(.., compound_page, .., offset_of_logical_buffer);
>
> sg_phys/sg_virt  includes this offset, but if you poke the sg and extract
> the page with sg_page yourself you must include this offset in your
> calculations too.
>

This patch should be re-sent when you have the RFC for the heap. Unless
there is a heap available in tree we don't really need this patch.

Thanks,
Laura

Re: [PATCH] ion: scatterlist offset not used for buffer map

[Colin Cross]

On Thu, Apr 7, 2016 at 11:56 PM, John Einar Reitan
<john.reitan@foss.arm.com> wrote:
> On Thu, Apr 07, 2016 at 12:37:50PM -0700, Laura Abbott wrote:
>> On 04/07/2016 04:29 AM, John Einar Reitan wrote:
>> > ion's default user/kernel page mapping code don't honor the offset
>> > option for scatterlists. It uses sg_page and expect the whole page to be
>> > mapped, while the offset could dictate an offset within a large page.
>> >
>> > sg_phys correctly accounts for the offset, so should be used instead.
>> >
>>
>> Can you be more specific about which heap and which allocation pattern
>> is exposing this bug?
>
> The heap that exposed the bug is one I'm developing and will be posting
> as a RFC soon. It uses compound pages and an sub-divides it into surface
> buffers. The ion buffers are configured to hold sgl's with the compound
> page and the correct offset of the buffer, via
> sg_set_page(.., compound_page, .., offset_of_logical_buffer);

I don't think this is right.  A compound_page still has a page struct
for every page, you should be passing the page struct where your data
starts.  Using an offset > PAGE_SIZE is going to break lots of places,
for example anywhere that uses kmap(sg_page(sg)).

> sg_phys/sg_virt  includes this offset, but if you poke the sg and extract
> the page with sg_page yourself you must include this offset in your
> calculations too.

Re: [PATCH] ion: scatterlist offset not used for buffer map

[John Einar Reitan]

> I don't think this is right.  A compound_page still has a page struct
> for every page, you should be passing the page struct where your data
> starts.  Using an offset > PAGE_SIZE is going to break lots of places,
> for example anywhere that uses kmap(sg_page(sg)).

Was kind-of expecting that response. Will update, thanks.