drivers/xen/swiotlb-xen.c | 51 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 2 deletions(-)
This commit is dedicated to fix incorrect convertion from
cpu_addr to page address in cases when we get virtual
address which allocated throught xen_swiotlb_alloc_coherent()
and can be mapped in the vmalloc range.
As the result, virt_to_page() cannot convert this address
properly and return incorrect page address.
Need to detect such cases and obtains the page address using
vmalloc_to_page() instead.
The reference code was copied from kernel/dma/ops_helpers.c
and modified to provide additional detections as described
above.
Signed-off-by: Roman Skakun <roman_skakun@epam.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Also, I have observed that the original common code didn't
make additional checks about contiguity of the memory region
represented by cpu_addr and size
May be, this means that these functions can get only physically
contiguous memory.
Is this correct?
Cheers!
---
drivers/xen/swiotlb-xen.c | 51 +++++++++++++++++++++++++++++++++++++--
1 file changed, 49 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 2b385c1b4a99..f99c98472927 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -563,6 +563,53 @@ xen_swiotlb_dma_supported(struct device *hwdev, u64 mask)
return xen_virt_to_bus(hwdev, xen_io_tlb_end - 1) <= mask;
}
+static int
+xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
+ void *cpu_addr, dma_addr_t dma_addr, size_t size,
+ unsigned long attrs)
+{
+ unsigned long user_count = vma_pages(vma);
+ unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
+ unsigned long off = vma->vm_pgoff;
+ struct page *page;
+
+ if (is_vmalloc_addr(cpu_addr))
+ page = vmalloc_to_page(cpu_addr);
+ else
+ page = virt_to_page(cpu_addr);
+
+ vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, attrs);
+
+ if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
+ return -ENXIO;
+
+ if (off >= count || user_count > count - off)
+ return -ENXIO;
+
+ return remap_pfn_range(vma, vma->vm_start,
+ page_to_pfn(page) + vma->vm_pgoff,
+ user_count << PAGE_SHIFT, vma->vm_page_prot);
+}
+
+static int
+xen_swiotlb_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
+ void *cpu_addr, dma_addr_t dma_addr, size_t size,
+ unsigned long attrs)
+{
+ struct page *page;
+ int ret;
+
+ if (is_vmalloc_addr(cpu_addr))
+ page = vmalloc_to_page(cpu_addr);
+ else
+ page = virt_to_page(cpu_addr);
+
+ ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
+ if (!ret)
+ sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0);
+ return ret;
+}
+
const struct dma_map_ops xen_swiotlb_dma_ops = {
.alloc = xen_swiotlb_alloc_coherent,
.free = xen_swiotlb_free_coherent,
@@ -575,8 +622,8 @@ const struct dma_map_ops xen_swiotlb_dma_ops = {
.map_page = xen_swiotlb_map_page,
.unmap_page = xen_swiotlb_unmap_page,
.dma_supported = xen_swiotlb_dma_supported,
- .mmap = dma_common_mmap,
- .get_sgtable = dma_common_get_sgtable,
+ .mmap = xen_swiotlb_dma_mmap,
+ .get_sgtable = xen_swiotlb_dma_get_sgtable,
.alloc_pages = dma_common_alloc_pages,
.free_pages = dma_common_free_pages,
};
--
2.27.0
On 6/11/21 5:55 AM, Roman Skakun wrote:
>
> +static int
> +xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> + void *cpu_addr, dma_addr_t dma_addr, size_t size,
> + unsigned long attrs)
> +{
> + unsigned long user_count = vma_pages(vma);
> + unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> + unsigned long off = vma->vm_pgoff;
> + struct page *page;
> +
> + if (is_vmalloc_addr(cpu_addr))
> + page = vmalloc_to_page(cpu_addr);
> + else
> + page = virt_to_page(cpu_addr);
> +
> + vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, attrs);
> +
> + if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
> + return -ENXIO;
> +
> + if (off >= count || user_count > count - off)
> + return -ENXIO;
> +
> + return remap_pfn_range(vma, vma->vm_start,
> + page_to_pfn(page) + vma->vm_pgoff,
> + user_count << PAGE_SHIFT, vma->vm_page_prot);
> +}
I suggest you create a helper for computing page value and then revert 922659ea771b3fd728149262c5ea15608fab9719 and pass result of the helper instead of cpu_addr. Here and in xen_swiotlb_dma_get_sgtable().
And use this new helper in xen_swiotlb_free_coherent() too. I am curious though why this was not a problem when Stefano was looking at the problem that introduced this vmalloc check (i.e. 8b1e868f66076490189a36d984fcce286cdd6295). Stefano?
-boris
Hey, Boris!
Thanks for the review.
I have an additional question about current implementation that disturbed me.
I think, that we can have cases when mapped memory can not be
physically contiguous.
In order to proceed this correctly need to apply some additional steps
to current implementation as well.
In mmap() :
1. Is the memory region physically contiguous?
2. Remap multiple ranges if it is not.
In get_sgtable() :
1. Is the memory region physically contiguous?
2. Create sgt that will be included multiple contiguous ranges if it is not.
What do you think about it?
Cheers!
Roman
пт, 11 июн. 2021 г. в 18:20, Boris Ostrovsky <boris.ostrovsky@oracle.com>:
>
>
> On 6/11/21 5:55 AM, Roman Skakun wrote:
> >
> > +static int
> > +xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> > + void *cpu_addr, dma_addr_t dma_addr, size_t size,
> > + unsigned long attrs)
> > +{
> > + unsigned long user_count = vma_pages(vma);
> > + unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> > + unsigned long off = vma->vm_pgoff;
> > + struct page *page;
> > +
> > + if (is_vmalloc_addr(cpu_addr))
> > + page = vmalloc_to_page(cpu_addr);
> > + else
> > + page = virt_to_page(cpu_addr);
> > +
> > + vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, attrs);
> > +
> > + if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
> > + return -ENXIO;
> > +
> > + if (off >= count || user_count > count - off)
> > + return -ENXIO;
> > +
> > + return remap_pfn_range(vma, vma->vm_start,
> > + page_to_pfn(page) + vma->vm_pgoff,
> > + user_count << PAGE_SHIFT, vma->vm_page_prot);
> > +}
>
>
> I suggest you create a helper for computing page value and then revert 922659ea771b3fd728149262c5ea15608fab9719 and pass result of the helper instead of cpu_addr. Here and in xen_swiotlb_dma_get_sgtable().
>
>
> And use this new helper in xen_swiotlb_free_coherent() too. I am curious though why this was not a problem when Stefano was looking at the problem that introduced this vmalloc check (i.e. 8b1e868f66076490189a36d984fcce286cdd6295). Stefano?
>
>
> -boris
--
Best Regards, Roman.
On 6/14/21 8:47 AM, Roman Skakun wrote: > Hey, Boris! > Thanks for the review. > > I have an additional question about current implementation that disturbed me. > I think, that we can have cases when mapped memory can not be > physically contiguous. > In order to proceed this correctly need to apply some additional steps > to current implementation as well. > > In mmap() : > 1. Is the memory region physically contiguous? > 2. Remap multiple ranges if it is not. > > In get_sgtable() : > 1. Is the memory region physically contiguous? > 2. Create sgt that will be included multiple contiguous ranges if it is not. > > What do you think about it? We make sure that we allocate contiguous memory in xen_swiotlb_alloc_coherent(). -boris
> We make sure that we allocate contiguous memory in xen_swiotlb_alloc_coherent(). I understood. Thanks! -- Best Regards, Roman.
This reverts commit 922659ea771b3fd728149262c5ea15608fab9719.
Signed-off-by: Roman Skakun <roman_skakun@epam.com>
---
drivers/xen/swiotlb-xen.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 2b385c1b4a99..90bc5fc321bc 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -563,6 +563,31 @@ xen_swiotlb_dma_supported(struct device *hwdev, u64 mask)
return xen_virt_to_bus(hwdev, xen_io_tlb_end - 1) <= mask;
}
+/*
+ * Create userspace mapping for the DMA-coherent memory.
+ * This function should be called with the pages from the current domain only,
+ * passing pages mapped from other domains would lead to memory corruption.
+ */
+static int
+xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
+ void *cpu_addr, dma_addr_t dma_addr, size_t size,
+ unsigned long attrs)
+{
+ return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size, attrs);
+}
+
+/*
+ * This function should be called with the pages from the current domain only,
+ * passing pages mapped from other domains would lead to memory corruption.
+ */
+static int
+xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
+ void *cpu_addr, dma_addr_t handle, size_t size,
+ unsigned long attrs)
+{
+ return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size, attrs);
+}
+
const struct dma_map_ops xen_swiotlb_dma_ops = {
.alloc = xen_swiotlb_alloc_coherent,
.free = xen_swiotlb_free_coherent,
@@ -575,8 +600,8 @@ const struct dma_map_ops xen_swiotlb_dma_ops = {
.map_page = xen_swiotlb_map_page,
.unmap_page = xen_swiotlb_unmap_page,
.dma_supported = xen_swiotlb_dma_supported,
- .mmap = dma_common_mmap,
- .get_sgtable = dma_common_get_sgtable,
+ .mmap = xen_swiotlb_dma_mmap,
+ .get_sgtable = xen_swiotlb_get_sgtable,
.alloc_pages = dma_common_alloc_pages,
.free_pages = dma_common_free_pages,
};
--
2.25.1
This commit is dedicated to fix incorrect conversion from
cpu_addr to page address in cases when we get virtual
address which allocated through xen_swiotlb_alloc_coherent()
and can be mapped in the vmalloc range.
As the result, virt_to_page() cannot convert this address
properly and return incorrect page address.
Need to detect such cases and obtains the page address using
vmalloc_to_page() instead.
The reference code for mmap() and get_sgtable() was copied
from kernel/dma/ops_helpers.c and modified to provide
additional detections as described above.
In order to simplify code there was added a new
dma_cpu_addr_to_page() helper.
Signed-off-by: Roman Skakun <roman_skakun@epam.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
drivers/xen/swiotlb-xen.c | 42 +++++++++++++++++++++++++++++++--------
1 file changed, 34 insertions(+), 8 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 90bc5fc321bc..9331a8500547 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -118,6 +118,14 @@ static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr)
return 0;
}
+static struct page *cpu_addr_to_page(void *cpu_addr)
+{
+ if (is_vmalloc_addr(cpu_addr))
+ return vmalloc_to_page(cpu_addr);
+ else
+ return virt_to_page(cpu_addr);
+}
+
static int
xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs)
{
@@ -337,7 +345,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
int order = get_order(size);
phys_addr_t phys;
u64 dma_mask = DMA_BIT_MASK(32);
- struct page *page;
+ struct page *page = cpu_addr_to_page(vaddr);
if (hwdev && hwdev->coherent_dma_mask)
dma_mask = hwdev->coherent_dma_mask;
@@ -349,11 +357,6 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
/* Convert the size to actually allocated. */
size = 1UL << (order + XEN_PAGE_SHIFT);
- if (is_vmalloc_addr(vaddr))
- page = vmalloc_to_page(vaddr);
- else
- page = virt_to_page(vaddr);
-
if (!WARN_ON((dev_addr + size - 1 > dma_mask) ||
range_straddles_page_boundary(phys, size)) &&
TestClearPageXenRemapped(page))
@@ -573,7 +576,23 @@ xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
void *cpu_addr, dma_addr_t dma_addr, size_t size,
unsigned long attrs)
{
- return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size, attrs);
+ unsigned long user_count = vma_pages(vma);
+ unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
+ unsigned long off = vma->vm_pgoff;
+ struct page *page = cpu_addr_to_page(cpu_addr);
+ int ret;
+
+ vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, attrs);
+
+ if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
+ return ret;
+
+ if (off >= count || user_count > count - off)
+ return -ENXIO;
+
+ return remap_pfn_range(vma, vma->vm_start,
+ page_to_pfn(page) + vma->vm_pgoff,
+ user_count << PAGE_SHIFT, vma->vm_page_prot);
}
/*
@@ -585,7 +604,14 @@ xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
void *cpu_addr, dma_addr_t handle, size_t size,
unsigned long attrs)
{
- return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size, attrs);
+ struct page *page = cpu_addr_to_page(cpu_addr);
+ int ret;
+
+ ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
+ if (!ret)
+ sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0);
+
+ return ret;
}
const struct dma_map_ops xen_swiotlb_dma_ops = {
--
2.25.1
On 6/16/21 7:42 AM, Roman Skakun wrote:
> This commit is dedicated to fix incorrect conversion from
> cpu_addr to page address in cases when we get virtual
> address which allocated through xen_swiotlb_alloc_coherent()
> and can be mapped in the vmalloc range.
> As the result, virt_to_page() cannot convert this address
> properly and return incorrect page address.
>
> Need to detect such cases and obtains the page address using
> vmalloc_to_page() instead.
>
> The reference code for mmap() and get_sgtable() was copied
> from kernel/dma/ops_helpers.c and modified to provide
> additional detections as described above.
>
> In order to simplify code there was added a new
> dma_cpu_addr_to_page() helper.
>
> Signed-off-by: Roman Skakun <roman_skakun@epam.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
> drivers/xen/swiotlb-xen.c | 42 +++++++++++++++++++++++++++++++--------
> 1 file changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 90bc5fc321bc..9331a8500547 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -118,6 +118,14 @@ static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr)
> return 0;
> }
>
> +static struct page *cpu_addr_to_page(void *cpu_addr)
> +{
> + if (is_vmalloc_addr(cpu_addr))
> + return vmalloc_to_page(cpu_addr);
> + else
> + return virt_to_page(cpu_addr);
> +}
> +
> static int
> xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs)
> {
> @@ -337,7 +345,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
> int order = get_order(size);
> phys_addr_t phys;
> u64 dma_mask = DMA_BIT_MASK(32);
> - struct page *page;
> + struct page *page = cpu_addr_to_page(vaddr);
>
> if (hwdev && hwdev->coherent_dma_mask)
> dma_mask = hwdev->coherent_dma_mask;
> @@ -349,11 +357,6 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
> /* Convert the size to actually allocated. */
> size = 1UL << (order + XEN_PAGE_SHIFT);
>
> - if (is_vmalloc_addr(vaddr))
> - page = vmalloc_to_page(vaddr);
> - else
> - page = virt_to_page(vaddr);
> -
> if (!WARN_ON((dev_addr + size - 1 > dma_mask) ||
> range_straddles_page_boundary(phys, size)) &&
> TestClearPageXenRemapped(page))
> @@ -573,7 +576,23 @@ xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> void *cpu_addr, dma_addr_t dma_addr, size_t size,
> unsigned long attrs)
> {
> - return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size, attrs);
> + unsigned long user_count = vma_pages(vma);
> + unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> + unsigned long off = vma->vm_pgoff;
> + struct page *page = cpu_addr_to_page(cpu_addr);
> + int ret;
> +
> + vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, attrs);
> +
> + if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
> + return ret;
> +
> + if (off >= count || user_count > count - off)
> + return -ENXIO;
> +
> + return remap_pfn_range(vma, vma->vm_start,
> + page_to_pfn(page) + vma->vm_pgoff,
> + user_count << PAGE_SHIFT, vma->vm_page_prot);
> }
I wonder now whether we could avoid code duplication between here and dma_common_mmap()/dma_common_get_sgtable() and use your helper there.
Christoph, would that work? I.e. something like
diff --git a/kernel/dma/ops_helpers.c b/kernel/dma/ops_helpers.c
index 910ae69cae77..43411c2fa47b 100644
--- a/kernel/dma/ops_helpers.c
+++ b/kernel/dma/ops_helpers.c
@@ -12,7 +12,7 @@ int dma_common_get_sgtable(struct device *dev, struct sg_table *sgt,
void *cpu_addr, dma_addr_t dma_addr, size_t size,
unsigned long attrs)
{
- struct page *page = virt_to_page(cpu_addr);
+ struct page *page = cpu_addr_to_page(cpu_addr);
int ret;
ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
@@ -43,7 +43,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
return -ENXIO;
return remap_pfn_range(vma, vma->vm_start,
- page_to_pfn(virt_to_page(cpu_addr)) + vma->vm_pgoff,
+ page_to_pfn(cpu_addr_to_page(cpu_addr)) + vma->vm_pgoff,
user_count << PAGE_SHIFT, vma->vm_page_prot);
#else
return -ENXIO;
-boris
>
> /*
> @@ -585,7 +604,14 @@ xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> void *cpu_addr, dma_addr_t handle, size_t size,
> unsigned long attrs)
> {
> - return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size, attrs);
> + struct page *page = cpu_addr_to_page(cpu_addr);
> + int ret;
> +
> + ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> + if (!ret)
> + sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0);
> +
> + return ret;
> }
>
> const struct dma_map_ops xen_swiotlb_dma_ops = {
On Wed, Jun 16, 2021 at 10:12:55AM -0400, Boris Ostrovsky wrote: > I wonder now whether we could avoid code duplication between here and dma_common_mmap()/dma_common_get_sgtable() and use your helper there. > > > Christoph, would that work? I.e. something like You should not duplicate the code at all, and just make the common helpers work with vmalloc addresses.
On 6/16/21 10:21 AM, Christoph Hellwig wrote: > On Wed, Jun 16, 2021 at 10:12:55AM -0400, Boris Ostrovsky wrote: >> I wonder now whether we could avoid code duplication between here and dma_common_mmap()/dma_common_get_sgtable() and use your helper there. >> >> >> Christoph, would that work? I.e. something like > You should not duplicate the code at all, and just make the common > helpers work with vmalloc addresses. Isn't the expectation of virt_to_page() that it only works on non-vmalloc'd addresses? (This is not a rhetorical question, I actually don't know). -boris
On Wed, Jun 16, 2021 at 11:33:50AM -0400, Boris Ostrovsky wrote: > Isn't the expectation of virt_to_page() that it only works on non-vmalloc'd addresses? (This is not a rhetorical question, I actually don't know). Yes. Thus is why I'd suggest to just do the vmalloc_to_page or virt_to_page dance in ops_helpers.c and just continue using that.
On 6/16/21 11:35 AM, Christoph Hellwig wrote: > On Wed, Jun 16, 2021 at 11:33:50AM -0400, Boris Ostrovsky wrote: >> Isn't the expectation of virt_to_page() that it only works on non-vmalloc'd addresses? (This is not a rhetorical question, I actually don't know). > Yes. Thus is why I'd suggest to just do the vmalloc_to_page or > virt_to_page dance in ops_helpers.c and just continue using that. Ah, OK, so something along the lines of what I suggested. (I thought by "helpers" you meant virt_to_page()). -boris
On Wed, Jun 16, 2021 at 11:39:07AM -0400, Boris Ostrovsky wrote: > > On 6/16/21 11:35 AM, Christoph Hellwig wrote: > > On Wed, Jun 16, 2021 at 11:33:50AM -0400, Boris Ostrovsky wrote: > >> Isn't the expectation of virt_to_page() that it only works on non-vmalloc'd addresses? (This is not a rhetorical question, I actually don't know). > > Yes. Thus is why I'd suggest to just do the vmalloc_to_page or > > virt_to_page dance in ops_helpers.c and just continue using that. > > > Ah, OK, so something along the lines of what I suggested. (I thought by "helpers" you meant virt_to_page()). Yes. Just keeping it contained in the common code without duplicating it into a xen-specific version.
This commit is dedicated to fix incorrect conversion from
cpu_addr to page address in cases when we get virtual
address which allocated in the vmalloc range.
As the result, virt_to_page() cannot convert this address
properly and return incorrect page address.
Need to detect such cases and obtains the page address using
vmalloc_to_page() instead.
Signed-off-by: Roman Skakun <roman_skakun@epam.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Hey!
Thanks for suggestions, Christoph!
I updated the patch according to your advice.
But, I'm so surprised because nobody catches this problem
in the common code before. It looks a bit strange as for me.
kernel/dma/ops_helpers.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/kernel/dma/ops_helpers.c b/kernel/dma/ops_helpers.c
index 910ae69cae77..782728d8a393 100644
--- a/kernel/dma/ops_helpers.c
+++ b/kernel/dma/ops_helpers.c
@@ -5,6 +5,14 @@
*/
#include <linux/dma-map-ops.h>
+static struct page *cpu_addr_to_page(void *cpu_addr)
+{
+ if (is_vmalloc_addr(cpu_addr))
+ return vmalloc_to_page(cpu_addr);
+ else
+ return virt_to_page(cpu_addr);
+}
+
/*
* Create scatter-list for the already allocated DMA buffer.
*/
@@ -12,7 +20,7 @@ int dma_common_get_sgtable(struct device *dev, struct sg_table *sgt,
void *cpu_addr, dma_addr_t dma_addr, size_t size,
unsigned long attrs)
{
- struct page *page = virt_to_page(cpu_addr);
+ struct page *page = cpu_addr_to_page(cpu_addr);
int ret;
ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
@@ -43,7 +51,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
return -ENXIO;
return remap_pfn_range(vma, vma->vm_start,
- page_to_pfn(virt_to_page(cpu_addr)) + vma->vm_pgoff,
+ page_to_pfn(cpu_addr_to_page(cpu_addr)) + vma->vm_pgoff,
user_count << PAGE_SHIFT, vma->vm_page_prot);
#else
return -ENXIO;
--
2.25.1
On Tue, 22 Jun 2021, Roman Skakun wrote:
> This commit is dedicated to fix incorrect conversion from
> cpu_addr to page address in cases when we get virtual
> address which allocated in the vmalloc range.
> As the result, virt_to_page() cannot convert this address
> properly and return incorrect page address.
>
> Need to detect such cases and obtains the page address using
> vmalloc_to_page() instead.
>
> Signed-off-by: Roman Skakun <roman_skakun@epam.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
> Hey!
> Thanks for suggestions, Christoph!
> I updated the patch according to your advice.
> But, I'm so surprised because nobody catches this problem
> in the common code before. It looks a bit strange as for me.
>
>
> kernel/dma/ops_helpers.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/dma/ops_helpers.c b/kernel/dma/ops_helpers.c
> index 910ae69cae77..782728d8a393 100644
> --- a/kernel/dma/ops_helpers.c
> +++ b/kernel/dma/ops_helpers.c
> @@ -5,6 +5,14 @@
> */
> #include <linux/dma-map-ops.h>
>
> +static struct page *cpu_addr_to_page(void *cpu_addr)
> +{
> + if (is_vmalloc_addr(cpu_addr))
> + return vmalloc_to_page(cpu_addr);
> + else
> + return virt_to_page(cpu_addr);
> +}
We have the same thing in xen_swiotlb_free_coherent. Can we find a way
to call cpu_addr_to_page() from xen_swiotlb_free_coherent? Maybe we can
make cpu_addr_to_page non-static? No problem if it is too much trouble.
> /*
> * Create scatter-list for the already allocated DMA buffer.
> */
> @@ -12,7 +20,7 @@ int dma_common_get_sgtable(struct device *dev, struct sg_table *sgt,
> void *cpu_addr, dma_addr_t dma_addr, size_t size,
> unsigned long attrs)
> {
> - struct page *page = virt_to_page(cpu_addr);
> + struct page *page = cpu_addr_to_page(cpu_addr);
> int ret;
>
> ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> @@ -43,7 +51,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
> return -ENXIO;
>
> return remap_pfn_range(vma, vma->vm_start,
> - page_to_pfn(virt_to_page(cpu_addr)) + vma->vm_pgoff,
> + page_to_pfn(cpu_addr_to_page(cpu_addr)) + vma->vm_pgoff,
> user_count << PAGE_SHIFT, vma->vm_page_prot);
> #else
> return -ENXIO;
> --
> 2.25.1
>
Hi, Stefano!
> We have the same thing in xen_swiotlb_free_coherent. Can we find a way
> to call cpu_addr_to_page() from xen_swiotlb_free_coherent?
> Maybe we can make cpu_addr_to_page non-static?
Yes, right, If we want to reuse this helper instead of the same code
block in xen_swiotlb_free_coherent() need to make cpu_addr_to_page() as
global and add a new declaration for this helper in include/linux/dma-map-ops.h.
What do you think?
Cheers!
ср, 14 июл. 2021 г. в 04:23, Stefano Stabellini <sstabellini@kernel.org>:
>
> On Tue, 22 Jun 2021, Roman Skakun wrote:
> > This commit is dedicated to fix incorrect conversion from
> > cpu_addr to page address in cases when we get virtual
> > address which allocated in the vmalloc range.
> > As the result, virt_to_page() cannot convert this address
> > properly and return incorrect page address.
> >
> > Need to detect such cases and obtains the page address using
> > vmalloc_to_page() instead.
> >
> > Signed-off-by: Roman Skakun <roman_skakun@epam.com>
> > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> > ---
> > Hey!
> > Thanks for suggestions, Christoph!
> > I updated the patch according to your advice.
> > But, I'm so surprised because nobody catches this problem
> > in the common code before. It looks a bit strange as for me.
> >
> >
> > kernel/dma/ops_helpers.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/dma/ops_helpers.c b/kernel/dma/ops_helpers.c
> > index 910ae69cae77..782728d8a393 100644
> > --- a/kernel/dma/ops_helpers.c
> > +++ b/kernel/dma/ops_helpers.c
> > @@ -5,6 +5,14 @@
> > */
> > #include <linux/dma-map-ops.h>
> >
> > +static struct page *cpu_addr_to_page(void *cpu_addr)
> > +{
> > + if (is_vmalloc_addr(cpu_addr))
> > + return vmalloc_to_page(cpu_addr);
> > + else
> > + return virt_to_page(cpu_addr);
> > +}
>
> We have the same thing in xen_swiotlb_free_coherent. Can we find a way
> to call cpu_addr_to_page() from xen_swiotlb_free_coherent? Maybe we can
> make cpu_addr_to_page non-static? No problem if it is too much trouble.
>
>
> > /*
> > * Create scatter-list for the already allocated DMA buffer.
> > */
> > @@ -12,7 +20,7 @@ int dma_common_get_sgtable(struct device *dev, struct sg_table *sgt,
> > void *cpu_addr, dma_addr_t dma_addr, size_t size,
> > unsigned long attrs)
> > {
> > - struct page *page = virt_to_page(cpu_addr);
> > + struct page *page = cpu_addr_to_page(cpu_addr);
> > int ret;
> >
> > ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> > @@ -43,7 +51,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
> > return -ENXIO;
> >
> > return remap_pfn_range(vma, vma->vm_start,
> > - page_to_pfn(virt_to_page(cpu_addr)) + vma->vm_pgoff,
> > + page_to_pfn(cpu_addr_to_page(cpu_addr)) + vma->vm_pgoff,
> > user_count << PAGE_SHIFT, vma->vm_page_prot);
> > #else
> > return -ENXIO;
> > --
> > 2.25.1
> >
--
Best Regards, Roman.
© 2016 - 2024 Red Hat, Inc.