Re: [PATCH v2] xen/gntdev: Fix the abuse of underlying struct page in DMA-buf import
On 08.01.24 14:05, Daniel Vetter wrote:
Hello Daniel
> On Sun, 7 Jan 2024 at 11:35, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko
>>
>> DO NOT access the underlying struct page of an sg table exported
>> by DMA-buf in dmabuf_imp_to_refs(), this is not allowed.
>> Please see drivers/dma-buf/dma-buf.c:mangle_sg_table() for details.
>>
>> Fortunately, here (for special Xen device) we can avoid using
>> pages and calculate gfns directly from dma addresses provided by
>> the sg table.
>>
>> Suggested-by: Daniel Vetter
>> Signed-off-by: Oleksandr Tyshchenko
>> Acked-by: Christian König
>> Reviewed-by: Stefano Stabellini
>> ---
>> Please note, I didn't manage to test the patch against the latest master
>> branch
>> on real HW (patch was only build tested there). Patch was tested on Arm64
>> guests using Linux v5.10.41 from vendor's BSP, this is the environment where
>> running this use-case is possible and to which I have an access (Xen PV
>> display
>> with zero-copy and backend domain as a buffer provider - be-alloc=1, so
>> dma-buf
>> import part was involved). A little bit old, but the dma-buf import code
>> in gntdev-dmabuf.c hasn't been changed much since that time, all context
>> remains allmost the same according to my code inspection.
>>
>>v2:
>> - add R-b and A-b
>> - fix build warning noticed by kernel test robot by initializing
>> "ret" in case of error
>>
>> https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/202401062122.it6zvlg0-...@intel.com/__;!!GF_29dbcQIUBPA!38-mwT9HCtOeZC3m4I-m9n0hragYMHfmWcHKgDxEpGs9mg35M0bpPWWORK8aichxHtO36GZ_JnCWTLdJXdZYBmCv$
>> [lore[.]kernel[.]org]
>> ---
>> ---
>> drivers/xen/gntdev-dmabuf.c | 44 -
>> 1 file changed, 19 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
>> index 4440e626b797..272c0ab01ef5 100644
>> --- a/drivers/xen/gntdev-dmabuf.c
>> +++ b/drivers/xen/gntdev-dmabuf.c
>> @@ -11,6 +11,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -50,7 +51,7 @@ struct gntdev_dmabuf {
>>
>> /* Number of pages this buffer has. */
>> int nr_pages;
>> - /* Pages of this buffer. */
>> + /* Pages of this buffer (only for dma-buf export). */
>> struct page **pages;
>> };
>>
>> @@ -484,7 +485,7 @@ static int dmabuf_exp_from_refs(struct gntdev_priv
>> *priv, int flags,
>> /* DMA buffer import support. */
>>
>> static int
>> -dmabuf_imp_grant_foreign_access(struct page **pages, u32 *refs,
>> +dmabuf_imp_grant_foreign_access(unsigned long *gfns, u32 *refs,
>> int count, int domid)
>> {
>> grant_ref_t priv_gref_head;
>> @@ -507,7 +508,7 @@ dmabuf_imp_grant_foreign_access(struct page **pages, u32
>> *refs,
>> }
>>
>> gnttab_grant_foreign_access_ref(cur_ref, domid,
>> - xen_page_to_gfn(pages[i]),
>> 0);
>> + gfns[i], 0);
>> refs[i] = cur_ref;
>> }
>>
>> @@ -529,7 +530,6 @@ static void dmabuf_imp_end_foreign_access(u32 *refs, int
>> count)
>>
>> static void dmabuf_imp_free_storage(struct gntdev_dmabuf *gntdev_dmabuf)
>> {
>> - kfree(gntdev_dmabuf->pages);
>> kfree(gntdev_dmabuf->u.imp.refs);
>> kfree(gntdev_dmabuf);
>> }
>> @@ -549,12 +549,6 @@ static struct gntdev_dmabuf
>> *dmabuf_imp_alloc_storage(int count)
>> if (!gntdev_dmabuf->u.imp.refs)
>> goto fail;
>>
>> - gntdev_dmabuf->pages = kcalloc(count,
>> - sizeof(gntdev_dmabuf->pages[0]),
>> - GFP_KERNEL);
>> - if (!gntdev_dmabuf->pages)
>> - goto fail;
>> -
>> gntdev_dmabuf->nr_pages = count;
>>
>> for (i = 0; i < count; i++)
>> @@ -576,7 +570,8 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv,
>> struct device *dev,
>> struct dma_buf *dma_buf;
>> struct dma_buf_attachment *attach;
>> struct sg_table *sgt;
>> - struct sg_page_iter sg_iter;
>> + struct sg_dma_page_iter sg_iter;
>> + unsigned long *gfns;
>> int i;
>>
>> dma_buf = dma_buf_get(fd);
>> @@ -624,26 +619,25 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv,
>> struct device *dev,
>>
>> gntdev_dmabuf->u.imp.sgt = sgt;
>>
>> - /* Now convert sgt to array of pages and check for page validity. */
>> + gfns = kcalloc(count, sizeof(*gfns), GFP_KERNEL);
>> + if (!gfns) {
>> + ret = ERR_PTR(-ENOMEM);
>> + goto fail_unmap;
>> + }
>> +
>> + /* Now convert sgt to array of gfns without accessing underlying
>> pages. */
>> i = 0;
>> - for_each_sgtable_page(sgt, _iter, 0) {
>> -
Re: [PATCH v2] xen/gntdev: Fix the abuse of underlying struct page in DMA-buf import
On Sun, 7 Jan 2024 at 11:35, Oleksandr Tyshchenko wrote:
>
> From: Oleksandr Tyshchenko
>
> DO NOT access the underlying struct page of an sg table exported
> by DMA-buf in dmabuf_imp_to_refs(), this is not allowed.
> Please see drivers/dma-buf/dma-buf.c:mangle_sg_table() for details.
>
> Fortunately, here (for special Xen device) we can avoid using
> pages and calculate gfns directly from dma addresses provided by
> the sg table.
>
> Suggested-by: Daniel Vetter
> Signed-off-by: Oleksandr Tyshchenko
> Acked-by: Christian König
> Reviewed-by: Stefano Stabellini
> ---
> Please note, I didn't manage to test the patch against the latest master
> branch
> on real HW (patch was only build tested there). Patch was tested on Arm64
> guests using Linux v5.10.41 from vendor's BSP, this is the environment where
> running this use-case is possible and to which I have an access (Xen PV
> display
> with zero-copy and backend domain as a buffer provider - be-alloc=1, so
> dma-buf
> import part was involved). A little bit old, but the dma-buf import code
> in gntdev-dmabuf.c hasn't been changed much since that time, all context
> remains allmost the same according to my code inspection.
>
> v2:
>- add R-b and A-b
>- fix build warning noticed by kernel test robot by initializing
> "ret" in case of error
>
> https://lore.kernel.org/oe-kbuild-all/202401062122.it6zvlg0-...@intel.com/
> ---
> ---
> drivers/xen/gntdev-dmabuf.c | 44 -
> 1 file changed, 19 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
> index 4440e626b797..272c0ab01ef5 100644
> --- a/drivers/xen/gntdev-dmabuf.c
> +++ b/drivers/xen/gntdev-dmabuf.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -50,7 +51,7 @@ struct gntdev_dmabuf {
>
> /* Number of pages this buffer has. */
> int nr_pages;
> - /* Pages of this buffer. */
> + /* Pages of this buffer (only for dma-buf export). */
> struct page **pages;
> };
>
> @@ -484,7 +485,7 @@ static int dmabuf_exp_from_refs(struct gntdev_priv *priv,
> int flags,
> /* DMA buffer import support. */
>
> static int
> -dmabuf_imp_grant_foreign_access(struct page **pages, u32 *refs,
> +dmabuf_imp_grant_foreign_access(unsigned long *gfns, u32 *refs,
> int count, int domid)
> {
> grant_ref_t priv_gref_head;
> @@ -507,7 +508,7 @@ dmabuf_imp_grant_foreign_access(struct page **pages, u32
> *refs,
> }
>
> gnttab_grant_foreign_access_ref(cur_ref, domid,
> - xen_page_to_gfn(pages[i]), 0);
> + gfns[i], 0);
> refs[i] = cur_ref;
> }
>
> @@ -529,7 +530,6 @@ static void dmabuf_imp_end_foreign_access(u32 *refs, int
> count)
>
> static void dmabuf_imp_free_storage(struct gntdev_dmabuf *gntdev_dmabuf)
> {
> - kfree(gntdev_dmabuf->pages);
> kfree(gntdev_dmabuf->u.imp.refs);
> kfree(gntdev_dmabuf);
> }
> @@ -549,12 +549,6 @@ static struct gntdev_dmabuf
> *dmabuf_imp_alloc_storage(int count)
> if (!gntdev_dmabuf->u.imp.refs)
> goto fail;
>
> - gntdev_dmabuf->pages = kcalloc(count,
> - sizeof(gntdev_dmabuf->pages[0]),
> - GFP_KERNEL);
> - if (!gntdev_dmabuf->pages)
> - goto fail;
> -
> gntdev_dmabuf->nr_pages = count;
>
> for (i = 0; i < count; i++)
> @@ -576,7 +570,8 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv,
> struct device *dev,
> struct dma_buf *dma_buf;
> struct dma_buf_attachment *attach;
> struct sg_table *sgt;
> - struct sg_page_iter sg_iter;
> + struct sg_dma_page_iter sg_iter;
> + unsigned long *gfns;
> int i;
>
> dma_buf = dma_buf_get(fd);
> @@ -624,26 +619,25 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv,
> struct device *dev,
>
> gntdev_dmabuf->u.imp.sgt = sgt;
>
> - /* Now convert sgt to array of pages and check for page validity. */
> + gfns = kcalloc(count, sizeof(*gfns), GFP_KERNEL);
> + if (!gfns) {
> + ret = ERR_PTR(-ENOMEM);
> + goto fail_unmap;
> + }
> +
> + /* Now convert sgt to array of gfns without accessing underlying
> pages. */
> i = 0;
> - for_each_sgtable_page(sgt, _iter, 0) {
> - struct page *page = sg_page_iter_page(_iter);
> - /*
> -* Check if page is valid: this can happen if we are given
> -* a page from VRAM or other resources which are not backed
> -* by a struct page.
> -*/
> - if (!pfn_valid(page_to_pfn(page))) {
> - ret = ERR_PTR(-EINVAL);
> -
[PATCH v2] xen/gntdev: Fix the abuse of underlying struct page in DMA-buf import
From: Oleksandr Tyshchenko
DO NOT access the underlying struct page of an sg table exported
by DMA-buf in dmabuf_imp_to_refs(), this is not allowed.
Please see drivers/dma-buf/dma-buf.c:mangle_sg_table() for details.
Fortunately, here (for special Xen device) we can avoid using
pages and calculate gfns directly from dma addresses provided by
the sg table.
Suggested-by: Daniel Vetter
Signed-off-by: Oleksandr Tyshchenko
Acked-by: Christian König
Reviewed-by: Stefano Stabellini
---
Please note, I didn't manage to test the patch against the latest master branch
on real HW (patch was only build tested there). Patch was tested on Arm64
guests using Linux v5.10.41 from vendor's BSP, this is the environment where
running this use-case is possible and to which I have an access (Xen PV display
with zero-copy and backend domain as a buffer provider - be-alloc=1, so dma-buf
import part was involved). A little bit old, but the dma-buf import code
in gntdev-dmabuf.c hasn't been changed much since that time, all context
remains allmost the same according to my code inspection.
v2:
- add R-b and A-b
- fix build warning noticed by kernel test robot by initializing
"ret" in case of error
https://lore.kernel.org/oe-kbuild-all/202401062122.it6zvlg0-...@intel.com/
---
---
drivers/xen/gntdev-dmabuf.c | 44 -
1 file changed, 19 insertions(+), 25 deletions(-)
diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index 4440e626b797..272c0ab01ef5 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -50,7 +51,7 @@ struct gntdev_dmabuf {
/* Number of pages this buffer has. */
int nr_pages;
- /* Pages of this buffer. */
+ /* Pages of this buffer (only for dma-buf export). */
struct page **pages;
};
@@ -484,7 +485,7 @@ static int dmabuf_exp_from_refs(struct gntdev_priv *priv,
int flags,
/* DMA buffer import support. */
static int
-dmabuf_imp_grant_foreign_access(struct page **pages, u32 *refs,
+dmabuf_imp_grant_foreign_access(unsigned long *gfns, u32 *refs,
int count, int domid)
{
grant_ref_t priv_gref_head;
@@ -507,7 +508,7 @@ dmabuf_imp_grant_foreign_access(struct page **pages, u32
*refs,
}
gnttab_grant_foreign_access_ref(cur_ref, domid,
- xen_page_to_gfn(pages[i]), 0);
+ gfns[i], 0);
refs[i] = cur_ref;
}
@@ -529,7 +530,6 @@ static void dmabuf_imp_end_foreign_access(u32 *refs, int
count)
static void dmabuf_imp_free_storage(struct gntdev_dmabuf *gntdev_dmabuf)
{
- kfree(gntdev_dmabuf->pages);
kfree(gntdev_dmabuf->u.imp.refs);
kfree(gntdev_dmabuf);
}
@@ -549,12 +549,6 @@ static struct gntdev_dmabuf *dmabuf_imp_alloc_storage(int
count)
if (!gntdev_dmabuf->u.imp.refs)
goto fail;
- gntdev_dmabuf->pages = kcalloc(count,
- sizeof(gntdev_dmabuf->pages[0]),
- GFP_KERNEL);
- if (!gntdev_dmabuf->pages)
- goto fail;
-
gntdev_dmabuf->nr_pages = count;
for (i = 0; i < count; i++)
@@ -576,7 +570,8 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct
device *dev,
struct dma_buf *dma_buf;
struct dma_buf_attachment *attach;
struct sg_table *sgt;
- struct sg_page_iter sg_iter;
+ struct sg_dma_page_iter sg_iter;
+ unsigned long *gfns;
int i;
dma_buf = dma_buf_get(fd);
@@ -624,26 +619,25 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv,
struct device *dev,
gntdev_dmabuf->u.imp.sgt = sgt;
- /* Now convert sgt to array of pages and check for page validity. */
+ gfns = kcalloc(count, sizeof(*gfns), GFP_KERNEL);
+ if (!gfns) {
+ ret = ERR_PTR(-ENOMEM);
+ goto fail_unmap;
+ }
+
+ /* Now convert sgt to array of gfns without accessing underlying pages.
*/
i = 0;
- for_each_sgtable_page(sgt, _iter, 0) {
- struct page *page = sg_page_iter_page(_iter);
- /*
-* Check if page is valid: this can happen if we are given
-* a page from VRAM or other resources which are not backed
-* by a struct page.
-*/
- if (!pfn_valid(page_to_pfn(page))) {
- ret = ERR_PTR(-EINVAL);
- goto fail_unmap;
- }
+ for_each_sgtable_dma_page(sgt, _iter, 0) {
+ dma_addr_t addr = sg_page_iter_dma_address(_iter);
+ unsigned long pfn = bfn_to_pfn(XEN_PFN_DOWN(dma_to_phys(dev,
addr)));
- gntdev_dmabuf->pages[i++] = page;
+
