Re: [PATCH] xen/gntdev: Fix the abuse of underlying struct page in DMA-buf import
Hi Oleksandr,
kernel test robot noticed the following build warnings:
[auto build test WARNING on xen-tip/linux-next]
[also build test WARNING on linus/master v6.7-rc8 next-20240105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Oleksandr-Tyshchenko/xen-gntdev-Fix-the-abuse-of-underlying-struct-page-in-DMA-buf-import/20240105-025516
base: https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git linux-next
patch link:
https://lore.kernel.org/r/20240104185327.177376-1-olekstysh%40gmail.com
patch subject: [PATCH] xen/gntdev: Fix the abuse of underlying struct page in
DMA-buf import
config: x86_64-allmodconfig
(https://download.01.org/0day-ci/archive/20240106/202401062122.it6zvlg0-...@intel.com/config)
compiler: ClangBuiltLinux clang version 17.0.6
(https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20240106/202401062122.it6zvlg0-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202401062122.it6zvlg0-...@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/xen/gntdev-dmabuf.c:623:6: warning: variable 'ret' is used
>> uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
623 | if (!gfns)
| ^
drivers/xen/gntdev-dmabuf.c:660:9: note: uninitialized use occurs here
660 | return ret;
|^~~
drivers/xen/gntdev-dmabuf.c:623:2: note: remove the 'if' if its condition is
always false
623 | if (!gfns)
| ^~
624 | goto fail_unmap;
| ~~~
drivers/xen/gntdev-dmabuf.c:569:43: note: initialize the variable 'ret' to
silence this warning
569 | struct gntdev_dmabuf *gntdev_dmabuf, *ret;
| ^
| = NULL
1 warning generated.
vim +623 drivers/xen/gntdev-dmabuf.c
564
565 static struct gntdev_dmabuf *
566 dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
567 int fd, int count, int domid)
568 {
569 struct gntdev_dmabuf *gntdev_dmabuf, *ret;
570 struct dma_buf *dma_buf;
571 struct dma_buf_attachment *attach;
572 struct sg_table *sgt;
573 struct sg_dma_page_iter sg_iter;
574 unsigned long *gfns;
575 int i;
576
577 dma_buf = dma_buf_get(fd);
578 if (IS_ERR(dma_buf))
579 return ERR_CAST(dma_buf);
580
581 gntdev_dmabuf = dmabuf_imp_alloc_storage(count);
582 if (IS_ERR(gntdev_dmabuf)) {
583 ret = gntdev_dmabuf;
584 goto fail_put;
585 }
586
587 gntdev_dmabuf->priv = priv;
588 gntdev_dmabuf->fd = fd;
589
590 attach = dma_buf_attach(dma_buf, dev);
591 if (IS_ERR(attach)) {
592 ret = ERR_CAST(attach);
593 goto fail_free_obj;
594 }
595
596 gntdev_dmabuf->u.imp.attach = attach;
597
598 sgt = dma_buf_map_attachment_unlocked(attach,
DMA_BIDIRECTIONAL);
599 if (IS_ERR(sgt)) {
600 ret = ERR_CAST(sgt);
601 goto fail_detach;
602 }
603
604 /* Check that we have zero offset. */
605 if (sgt->sgl->offset) {
606 ret = ERR_PTR(-EINVAL);
607 pr_debug("DMA buffer has %d bytes offset, user-space
expects 0\n",
608 sgt->sgl->offset);
609 goto fail_unmap;
610 }
611
612 /* Check number of pages that imported buffer has. */
613 if (attach->dmabuf->size != gntdev_dmabuf->nr_pages <<
PAGE_SHIFT) {
614 ret = ERR_PTR(-EINVAL);
615 pr_debug("DMA buffer has %zu pages, user-space expects
%d\n",
616 attach->dmabuf->size, gntdev_dmabuf->nr_pages);
617 goto fail_unmap;
618 }
619
620 gntdev_dmabuf->u.imp.sgt = sgt;
621
622 gfns = kcalloc(count, sizeof(*gfns), GFP_KERNEL);
> 623 if (!gfns)
624
Re: [PATCH] xen/gntdev: Fix the abuse of underlying struct page in DMA-buf import
Am 04.01.24 um 19:53 schrieb Oleksandr Tyshchenko:
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
I can't say that I can judge the full technical background, but that
looks reasonable to me.
Acked-by: Christian König
---
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.
---
---
drivers/xen/gntdev-dmabuf.c | 42 +++--
1 file changed, 17 insertions(+), 25 deletions(-)
diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index 4440e626b797..0dde49fca9a5 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,23 @@ 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)
+ goto fail_unmap;
+
+ /* Now convert sgt to array of gfns without accessing underlying pages.
*/
i = 0;
- for_each_sgtable_page(sgt, &sg_iter, 0) {
- struct page *page = sg_page_iter_page(&sg_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, &sg_iter, 0) {
+ dma_addr_t addr = sg_page_iter_dma_address(&sg_iter);
+ unsigned long pfn = bfn_to_pfn(XEN_PFN_DOWN(dma_to_phys(dev,
addr)));
- gntdev_dmabuf->pages[i++] = page;
+ gfns[i++] = pfn_to_gfn(pfn);
}
- ret = ERR_PTR(dmabuf_imp_grant_foreign_access(gntdev_dmabuf->pages,
+ ret = ERR_PTR(
Re: [PATCH] xen/gntdev: Fix the abuse of underlying struct page in DMA-buf import
On Thu, 4 Jan 2024, 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
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.
> ---
> ---
> drivers/xen/gntdev-dmabuf.c | 42 +++--
> 1 file changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
> index 4440e626b797..0dde49fca9a5 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,23 @@ 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)
> + goto fail_unmap;
> +
> + /* Now convert sgt to array of gfns without accessing underlying pages.
> */
> i = 0;
> - for_each_sgtable_page(sgt, &sg_iter, 0) {
> - struct page *page = sg_page_iter_page(&sg_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, &sg_iter, 0) {
> + dma_addr_t addr = sg_page_iter_dma_address(&sg_iter);
> + unsigned long pfn = bfn_to_pfn(XEN_PFN_DOWN(dma_to_phys(dev,
> addr)));
>
> - gntdev_dmabuf->pages[i++] = page;
> + gfns[i++] = pfn_to_gfn(pfn);
> }
>
> -
[PATCH] 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
---
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.
---
---
drivers/xen/gntdev-dmabuf.c | 42 +++--
1 file changed, 17 insertions(+), 25 deletions(-)
diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index 4440e626b797..0dde49fca9a5 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,23 @@ 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)
+ goto fail_unmap;
+
+ /* Now convert sgt to array of gfns without accessing underlying pages.
*/
i = 0;
- for_each_sgtable_page(sgt, &sg_iter, 0) {
- struct page *page = sg_page_iter_page(&sg_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, &sg_iter, 0) {
+ dma_addr_t addr = sg_page_iter_dma_address(&sg_iter);
+ unsigned long pfn = bfn_to_pfn(XEN_PFN_DOWN(dma_to_phys(dev,
addr)));
- gntdev_dmabuf->pages[i++] = page;
+ gfns[i++] = pfn_to_gfn(pfn);
}
- ret = ERR_PTR(dmabuf_imp_grant_foreign_access(gntdev_dmabuf->pages,
+ ret = ERR_PTR(dmabuf_imp_grant_foreign_access(gfns,
gntdev_dmabuf->u.imp.refs,
