On 12/19/2022 9:57 AM, Zheng Wang wrote:
> Hi Zhi,
>
> Thanks again for your reply and clear explaination about the function.
> I still have some doubt about the fix. Here is a invoke chain :
> ppgtt_populate_spt
> ->ppgtt_populate_shadow_entry
> ->split_2MB_gtt_entry
> As far as I'm concerned, when something error happens in DMA mapping,
> which will make intel_gvt_dma_map_guest_page return none-zero code,
> It will invoke ppgtt_invalidate_spt and call ppgtt_free_spt,which will
> finally free spt by kfree. But the caller doesn't notice that and frees
> spt by calling ppgtt_free_spt again. This is a typical UAF/Double Free
> vulnerability. So I think the key point is about how to handle spt properly.
> The handle newly allocated spt (aka sub_spt) is not the root cause of this
> issue. Could you please give me more advice about how to fix this security
> bug? Besides, I'm not sure if there are more similar problems in othe location.
>
> Best regards,
> Zheng Wang
>
I think it is a case-by-case thing. For example:
The current scenario in this function looks like below:
caller pass spt a
function
alloc spt b
something error
free spt a
return error
The problem is: the function wrongly frees the spt a instead free what
it allocates.
A proper fix should be:
caller pass spt a
function
alloc spt b
something error
*free spt b*
return error
Thanks,
Zhi.
Wang, Zhi A <[email protected]> 于2022年12月19日周一 16:22写道：
>
> I think it is a case-by-case thing. For example:
>
> The current scenario in this function looks like below:
>
> caller pass spt a
> function
> alloc spt b
> something error
> free spt a
> return error
>
> The problem is: the function wrongly frees the spt a instead free what
> it allocates.
Thanks for your clear explaination. It’s really helpfult to me.
I think I might know how to fix now.
> A proper fix should be:
>
> caller pass spt a
> function
> alloc spt b
> something error
> *free spt b*
> return error
>
As it's a case-by-case thing, I'll extract the un-done-mapping-dma part from
ppgtt_invalidate_spt and put it in error path. Then I'll add the code of freeing
new allocated spt. If I misunderstand your meaning, feel free to let me know.
Working on a new fix now.
Best regards,
Zheng Wang
If intel_gvt_dma_map_guest_page failed, it will call
ppgtt_invalidate_spt, which will finally free the spt. But the caller does
not notice that, it will free spt again in error path.
Fix this by undoing the mapping of DMA address and freeing sub_spt.
Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support")
Signed-off-by: Zheng Wang <[email protected]>
---
v4:
- fix by undo the mapping of DMA address and free sub_spt suggested by Zhi
v3:
- correct spelling mistake and remove unused variable suggested by Greg
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/
---
drivers/gpu/drm/i915/gvt/gtt.c | 58 +++++++++++++++++-----------------
1 file changed, 29 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 45271acc5038..b472e021e5a4 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -1209,7 +1209,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
for_each_shadow_entry(sub_spt, &sub_se, sub_index) {
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
PAGE_SIZE, &dma_addr);
- if (ret)
+ if (ret)
goto err;
sub_se.val64 = se->val64;
@@ -1233,34 +1233,34 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
/* Undone the existing mappings of DMA addr. */
for_each_present_shadow_entry(spt, &e, parent_index) {
switch (e.type) {
- case GTT_TYPE_PPGTT_PTE_4K_ENTRY:
- gvt_vdbg_mm("invalidate 4K entry\n");
- ppgtt_invalidate_pte(spt, &e);
- break;
- case GTT_TYPE_PPGTT_PTE_64K_ENTRY:
- /* We don't setup 64K shadow entry so far. */
- WARN(1, "suspicious 64K gtt entry\n");
- continue;
- case GTT_TYPE_PPGTT_PTE_2M_ENTRY:
- gvt_vdbg_mm("invalidate 2M entry\n");
- continue;
- case GTT_TYPE_PPGTT_PTE_1G_ENTRY:
- WARN(1, "GVT doesn't support 1GB page\n");
- continue;
- case GTT_TYPE_PPGTT_PML4_ENTRY:
- case GTT_TYPE_PPGTT_PDP_ENTRY:
- case GTT_TYPE_PPGTT_PDE_ENTRY:
- gvt_vdbg_mm("invalidate PMUL4/PDP/PDE entry\n");
- ret1 = ppgtt_invalidate_spt_by_shadow_entry(
- spt->vgpu, &e);
- if (ret1) {
- gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
- spt, e.val64, e.type);
- goto free_spt;
- }
- break;
- default:
- GEM_BUG_ON(1);
+ case GTT_TYPE_PPGTT_PTE_4K_ENTRY:
+ gvt_vdbg_mm("invalidate 4K entry\n");
+ ppgtt_invalidate_pte(spt, &e);
+ break;
+ case GTT_TYPE_PPGTT_PTE_64K_ENTRY:
+ /* We don't setup 64K shadow entry so far. */
+ WARN(1, "suspicious 64K gtt entry\n");
+ continue;
+ case GTT_TYPE_PPGTT_PTE_2M_ENTRY:
+ gvt_vdbg_mm("invalidate 2M entry\n");
+ continue;
+ case GTT_TYPE_PPGTT_PTE_1G_ENTRY:
+ WARN(1, "GVT doesn't support 1GB page\n");
+ continue;
+ case GTT_TYPE_PPGTT_PML4_ENTRY:
+ case GTT_TYPE_PPGTT_PDP_ENTRY:
+ case GTT_TYPE_PPGTT_PDE_ENTRY:
+ gvt_vdbg_mm("invalidate PMUL4/PDP/PDE entry\n");
+ ret1 = ppgtt_invalidate_spt_by_shadow_entry(
+ spt->vgpu, &e);
+ if (ret1) {
+ gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
+ spt, e.val64, e.type);
+ goto free_spt;
+ }
+ break;
+ default:
+ GEM_BUG_ON(1);
}
}
/* Release the new alloced apt. */
--
2.25.1
If intel_gvt_dma_map_guest_page failed, it will call
ppgtt_invalidate_spt, which will finally free the spt. But the caller does
not notice that, it will free spt again in error path.
Fix this by undoing the mapping of DMA address and freeing sub_spt.
Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support")
Signed-off-by: Zheng Wang <[email protected]>
---
v4:
- fix by undo the mapping of DMA address and free sub_spt suggested by Zhi
v3:
- correct spelling mistake and remove unused variable suggested by Greg
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/
---
drivers/gpu/drm/i915/gvt/gtt.c | 53 +++++++++++++++++++++++++++++-----
1 file changed, 46 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 51e5e8fb505b..b472e021e5a4 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -1192,11 +1192,11 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
{
const struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
struct intel_vgpu_ppgtt_spt *sub_spt;
- struct intel_gvt_gtt_entry sub_se;
+ struct intel_gvt_gtt_entry sub_se, e;
unsigned long start_gfn;
dma_addr_t dma_addr;
- unsigned long sub_index;
- int ret;
+ unsigned long sub_index, parent_index;
+ int ret, ret1;
gvt_dbg_mm("Split 2M gtt entry, index %lu\n", index);
@@ -1209,10 +1209,8 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
for_each_shadow_entry(sub_spt, &sub_se, sub_index) {
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
PAGE_SIZE, &dma_addr);
- if (ret) {
- ppgtt_invalidate_spt(spt);
- return ret;
- }
+ if (ret)
+ goto err;
sub_se.val64 = se->val64;
/* Copy the PAT field from PDE. */
@@ -1231,6 +1229,47 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
ops->set_pfn(se, sub_spt->shadow_page.mfn);
ppgtt_set_shadow_entry(spt, se, index);
return 0;
+err:
+ /* Undone the existing mappings of DMA addr. */
+ for_each_present_shadow_entry(spt, &e, parent_index) {
+ switch (e.type) {
+ case GTT_TYPE_PPGTT_PTE_4K_ENTRY:
+ gvt_vdbg_mm("invalidate 4K entry\n");
+ ppgtt_invalidate_pte(spt, &e);
+ break;
+ case GTT_TYPE_PPGTT_PTE_64K_ENTRY:
+ /* We don't setup 64K shadow entry so far. */
+ WARN(1, "suspicious 64K gtt entry\n");
+ continue;
+ case GTT_TYPE_PPGTT_PTE_2M_ENTRY:
+ gvt_vdbg_mm("invalidate 2M entry\n");
+ continue;
+ case GTT_TYPE_PPGTT_PTE_1G_ENTRY:
+ WARN(1, "GVT doesn't support 1GB page\n");
+ continue;
+ case GTT_TYPE_PPGTT_PML4_ENTRY:
+ case GTT_TYPE_PPGTT_PDP_ENTRY:
+ case GTT_TYPE_PPGTT_PDE_ENTRY:
+ gvt_vdbg_mm("invalidate PMUL4/PDP/PDE entry\n");
+ ret1 = ppgtt_invalidate_spt_by_shadow_entry(
+ spt->vgpu, &e);
+ if (ret1) {
+ gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
+ spt, e.val64, e.type);
+ goto free_spt;
+ }
+ break;
+ default:
+ GEM_BUG_ON(1);
+ }
+ }
+ /* Release the new alloced apt. */
+free_spt:
+ trace_spt_change(sub_spt->vgpu->id, "release", sub_spt,
+ sub_spt->guest_page.gfn, sub_spt->shadow_page.type);
+ ppgtt_free_spt(sub_spt);
+ sub_spt = NULL;
+ return ret;
}
static int split_64KB_gtt_entry(struct intel_vgpu *vgpu,
--
2.25.1
On 2022.12.19 20:52:04 +0800, Zheng Wang wrote:
> If intel_gvt_dma_map_guest_page failed, it will call
> ppgtt_invalidate_spt, which will finally free the spt. But the caller does
> not notice that, it will free spt again in error path.
>
It's not clear from this description which caller is actually wrong,
better to clarify the problem in ppgtt_populate_spt_by_guest_entry() function.
> Fix this by undoing the mapping of DMA address and freeing sub_spt.
>
> Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support")
> Signed-off-by: Zheng Wang <[email protected]>
> ---
> v4:
> - fix by undo the mapping of DMA address and free sub_spt suggested by Zhi
>
> v3:
> - correct spelling mistake and remove unused variable suggested by Greg
>
> v2: https://lore.kernel.org/all/[email protected]/
>
> v1: https://lore.kernel.org/all/[email protected]/
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 53 +++++++++++++++++++++++++++++-----
> 1 file changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 51e5e8fb505b..b472e021e5a4 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -1192,11 +1192,11 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
> {
> const struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
> struct intel_vgpu_ppgtt_spt *sub_spt;
> - struct intel_gvt_gtt_entry sub_se;
> + struct intel_gvt_gtt_entry sub_se, e;
> unsigned long start_gfn;
> dma_addr_t dma_addr;
> - unsigned long sub_index;
> - int ret;
> + unsigned long sub_index, parent_index;
> + int ret, ret1;
>
> gvt_dbg_mm("Split 2M gtt entry, index %lu\n", index);
>
> @@ -1209,10 +1209,8 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
> for_each_shadow_entry(sub_spt, &sub_se, sub_index) {
> ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
> PAGE_SIZE, &dma_addr);
> - if (ret) {
> - ppgtt_invalidate_spt(spt);
> - return ret;
> - }
> + if (ret)
> + goto err;
I think it's fine to remove this and leave to upper caller, but again please
describe the behavior change in commit message as well, e.g to fix the sanity
of spt destroy that leaving previous invalidate and free of spt to caller function
instead of within callee function.
> sub_se.val64 = se->val64;
>
> /* Copy the PAT field from PDE. */
> @@ -1231,6 +1229,47 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
> ops->set_pfn(se, sub_spt->shadow_page.mfn);
> ppgtt_set_shadow_entry(spt, se, index);
> return 0;
> +err:
> + /* Undone the existing mappings of DMA addr. */
> + for_each_present_shadow_entry(spt, &e, parent_index) {
sub_spt? We're undoing what's mapped for sub_spt right?
> + switch (e.type) {
> + case GTT_TYPE_PPGTT_PTE_4K_ENTRY:
> + gvt_vdbg_mm("invalidate 4K entry\n");
> + ppgtt_invalidate_pte(spt, &e);
> + break;
> + case GTT_TYPE_PPGTT_PTE_64K_ENTRY:
> + /* We don't setup 64K shadow entry so far. */
> + WARN(1, "suspicious 64K gtt entry\n");
> + continue;
> + case GTT_TYPE_PPGTT_PTE_2M_ENTRY:
> + gvt_vdbg_mm("invalidate 2M entry\n");
> + continue;
> + case GTT_TYPE_PPGTT_PTE_1G_ENTRY:
> + WARN(1, "GVT doesn't support 1GB page\n");
> + continue;
> + case GTT_TYPE_PPGTT_PML4_ENTRY:
> + case GTT_TYPE_PPGTT_PDP_ENTRY:
> + case GTT_TYPE_PPGTT_PDE_ENTRY:
I don't think this all entry type makes sense, as here we just split
2M entry for multiple 4K PTE entry.
> + gvt_vdbg_mm("invalidate PMUL4/PDP/PDE entry\n");
> + ret1 = ppgtt_invalidate_spt_by_shadow_entry(
> + spt->vgpu, &e);
> + if (ret1) {
> + gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
> + spt, e.val64, e.type);
> + goto free_spt;
> + }
for above reason, I don't think this is valid.
> + break;
> + default:
> + GEM_BUG_ON(1);
> + }
> + }
> + /* Release the new alloced apt. */
> +free_spt:
> + trace_spt_change(sub_spt->vgpu->id, "release", sub_spt,
> + sub_spt->guest_page.gfn, sub_spt->shadow_page.type);
> + ppgtt_free_spt(sub_spt);
> + sub_spt = NULL;
> + return ret;
> }
>
> static int split_64KB_gtt_entry(struct intel_vgpu *vgpu,
> --
> 2.25.1
>
Zhenyu Wang <[email protected]> 于2022年12月20日周二 16:25写道：
>
> On 2022.12.19 20:52:04 +0800, Zheng Wang wrote:
> > If intel_gvt_dma_map_guest_page failed, it will call
> > ppgtt_invalidate_spt, which will finally free the spt. But the caller does
> > not notice that, it will free spt again in error path.
> >
>
> It's not clear from this description which caller is actually wrong,
> better to clarify the problem in ppgtt_populate_spt_by_guest_entry() function.
>
Get it, will do in the next fix.
> > PAGE_SIZE, &dma_addr);
> > - if (ret) {
> > - ppgtt_invalidate_spt(spt);
> > - return ret;
> > - }
> > + if (ret)
> > + goto err;
>
> I think it's fine to remove this and leave to upper caller, but again please
> describe the behavior change in commit message as well, e.g to fix the sanity
> of spt destroy that leaving previous invalidate and free of spt to caller function
> instead of within callee function.
Sorry for my bad habit. Will do in the next version.
> > sub_se.val64 = se->val64;
> >
> > /* Copy the PAT field from PDE. */
> > @@ -1231,6 +1229,47 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
> > ops->set_pfn(se, sub_spt->shadow_page.mfn);
> > ppgtt_set_shadow_entry(spt, se, index);
> > return 0;
> > +err:
> > + /* Undone the existing mappings of DMA addr. */
> > + for_each_present_shadow_entry(spt, &e, parent_index) {
>
> sub_spt? We're undoing what's mapped for sub_spt right?
Yes, will change it to sub_spt in the next version.
>
> > + switch (e.type) {
> > + case GTT_TYPE_PPGTT_PTE_4K_ENTRY:
> > + gvt_vdbg_mm("invalidate 4K entry\n");
> > + ppgtt_invalidate_pte(spt, &e);
> > + break;
> > + case GTT_TYPE_PPGTT_PTE_64K_ENTRY:
> > + /* We don't setup 64K shadow entry so far. */
> > + WARN(1, "suspicious 64K gtt entry\n");
> > + continue;
> > + case GTT_TYPE_PPGTT_PTE_2M_ENTRY:
> > + gvt_vdbg_mm("invalidate 2M entry\n");
> > + continue;
> > + case GTT_TYPE_PPGTT_PTE_1G_ENTRY:
> > + WARN(1, "GVT doesn't support 1GB page\n");
> > + continue;
> > + case GTT_TYPE_PPGTT_PML4_ENTRY:
> > + case GTT_TYPE_PPGTT_PDP_ENTRY:
> > + case GTT_TYPE_PPGTT_PDE_ENTRY:
>
> I don't think this all entry type makes sense, as here we just split
> 2M entry for multiple 4K PTE entry.
I got it. I will leave the code for handling 4K PTE entry only.
>
> > + gvt_vdbg_mm("invalidate PMUL4/PDP/PDE entry\n");
> > + ret1 = ppgtt_invalidate_spt_by_shadow_entry(
> > + spt->vgpu, &e);
> > + if (ret1) {
> > + gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
> > + spt, e.val64, e.type);
> > + goto free_spt;
> > + }
>
> for above reason, I don't think this is valid.
Got it.
Thanks for your carefully reviewing. I'll try to fix that in the coming patch.
Best regards,
Zheng Wang
If intel_gvt_dma_map_guest_page failed, it will call
ppgtt_invalidate_spt, which will finally free the spt. But the
caller function ppgtt_populate_spt_by_guest_entry does not notice
that, it will free spt again in its error path.
Fix this by undoing the mapping of DMA address and freeing sub_spt.
Besides, leave the handle of spt destroy to caller function instead of
callee function when error occurs.
Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support")
Signed-off-by: Zheng Wang <[email protected]>
---
v5:
- remove unnecessary switch-case code for there is only one particular case,
correct the unmap target from parent_spt to sub_spt.add more details in
commit message. All suggested by Zhenyu
v4:
- fix by undo the mapping of DMA address and free sub_spt suggested by Zhi
v3:
- correct spelling mistake and remove unused variable suggested by Greg
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/
---
drivers/gpu/drm/i915/gvt/gtt.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 51e5e8fb505b..4d478a59eb7d 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -1209,10 +1209,8 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
for_each_shadow_entry(sub_spt, &sub_se, sub_index) {
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
PAGE_SIZE, &dma_addr);
- if (ret) {
- ppgtt_invalidate_spt(spt);
- return ret;
- }
+ if (ret)
+ goto err;
sub_se.val64 = se->val64;
/* Copy the PAT field from PDE. */
@@ -1231,6 +1229,18 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
ops->set_pfn(se, sub_spt->shadow_page.mfn);
ppgtt_set_shadow_entry(spt, se, index);
return 0;
+err:
+ /* Undone the existing mappings of DMA addr. */
+ for_each_present_shadow_entry(sub_spt, &sub_se, sub_index) {
+ gvt_vdbg_mm("invalidate 4K entry\n");
+ ppgtt_invalidate_pte(sub_spt, &sub_se);
+ }
+ /* Release the new allocated spt. */
+ trace_spt_change(sub_spt->vgpu->id, "release", sub_spt,
+ sub_spt->guest_page.gfn, sub_spt->shadow_page.type);
+ ppgtt_free_spt(sub_spt);
+ sub_spt = NULL;
+ return ret;
}
static int split_64KB_gtt_entry(struct intel_vgpu *vgpu,
--
2.25.1
On 2022.12.20 17:40:14 +0800, Zheng Wang wrote:
> If intel_gvt_dma_map_guest_page failed, it will call
> ppgtt_invalidate_spt, which will finally free the spt. But the
> caller function ppgtt_populate_spt_by_guest_entry does not notice
> that, it will free spt again in its error path.
indent
>
> Fix this by undoing the mapping of DMA address and freeing sub_spt.
> Besides, leave the handle of spt destroy to caller function instead of
> callee function when error occurs.
>
> Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support")
> Signed-off-by: Zheng Wang <[email protected]>
> ---
> v5:
> - remove unnecessary switch-case code for there is only one particular case,
> correct the unmap target from parent_spt to sub_spt.add more details in
> commit message. All suggested by Zhenyu
>
> v4:
> - fix by undo the mapping of DMA address and free sub_spt suggested by Zhi
>
> v3:
> - correct spelling mistake and remove unused variable suggested by Greg
>
> v2: https://lore.kernel.org/all/[email protected]/
>
> v1: https://lore.kernel.org/all/[email protected]/
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 51e5e8fb505b..4d478a59eb7d 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -1209,10 +1209,8 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
> for_each_shadow_entry(sub_spt, &sub_se, sub_index) {
> ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
> PAGE_SIZE, &dma_addr);
> - if (ret) {
> - ppgtt_invalidate_spt(spt);
> - return ret;
> - }
> + if (ret)
> + goto err;
> sub_se.val64 = se->val64;
>
> /* Copy the PAT field from PDE. */
> @@ -1231,6 +1229,18 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
> ops->set_pfn(se, sub_spt->shadow_page.mfn);
> ppgtt_set_shadow_entry(spt, se, index);
> return 0;
> +err:
> + /* Undone the existing mappings of DMA addr. */
We need a verb here for Undo.
> + for_each_present_shadow_entry(sub_spt, &sub_se, sub_index) {
> + gvt_vdbg_mm("invalidate 4K entry\n");
> + ppgtt_invalidate_pte(sub_spt, &sub_se);
> + }
> + /* Release the new allocated spt. */
> + trace_spt_change(sub_spt->vgpu->id, "release", sub_spt,
> + sub_spt->guest_page.gfn, sub_spt->shadow_page.type);
> + ppgtt_free_spt(sub_spt);
> + sub_spt = NULL;
Not need to reset local variable that has no use then.
I'll handle these trivial fixes during the merge.
Reviewed-by: Zhenyu Wang <[email protected]>
thanks
> + return ret;
> }
>
> static int split_64KB_gtt_entry(struct intel_vgpu *vgpu,
> --
> 2.25.1
>
Zhenyu Wang <[email protected]> 于2022年12月21日周三 11:01写道：
>
> On 2022.12.20 17:40:14 +0800, Zheng Wang wrote:
> > If intel_gvt_dma_map_guest_page failed, it will call
> > ppgtt_invalidate_spt, which will finally free the spt. But the
> > caller function ppgtt_populate_spt_by_guest_entry does not notice
> > that, it will free spt again in its error path.
>
> indent
Yeap :)
> > + if (ret)
> > + goto err;
> > sub_se.val64 = se->val64;
> >
> > /* Copy the PAT field from PDE. */
> > @@ -1231,6 +1229,18 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
> > ops->set_pfn(se, sub_spt->shadow_page.mfn);
> > ppgtt_set_shadow_entry(spt, se, index);
> > return 0;
> > +err:
> > + /* Undone the existing mappings of DMA addr. */
>
> We need a verb here for Undo.
Get it.
>
> > + for_each_present_shadow_entry(sub_spt, &sub_se, sub_index) {
> > + gvt_vdbg_mm("invalidate 4K entry\n");
> > + ppgtt_invalidate_pte(sub_spt, &sub_se);
> > + }
> > + /* Release the new allocated spt. */
> > + trace_spt_change(sub_spt->vgpu->id, "release", sub_spt,
> > + sub_spt->guest_page.gfn, sub_spt->shadow_page.type);
> > + ppgtt_free_spt(sub_spt);
> > + sub_spt = NULL;
>
> Not need to reset local variable that has no use then.
>
> I'll handle these trivial fixes during the merge.
>
Very thanks for that.
Best regards,
Zheng Wang
If intel_gvt_dma_map_guest_page failed, it will call
ppgtt_invalidate_spt, which will finally free the spt.
But the caller function ppgtt_populate_spt_by_guest_entry
does not notice that, it will free spt again in its error
path.
Fix this by canceling the mapping of DMA address and freeing sub_spt.
Besides, leave the handle of spt destroy to caller function instead
of callee function when error occurs.
Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support")
Signed-off-by: Zheng Wang <[email protected]>
Reviewed-by: Zhenyu Wang <[email protected]>
---
v6:
- remove the code for setting unused variable to NULL and fix type suggested
by Zhenyu
v5:
- remove unnecessary switch-case code for there is only one particular case,
correct the unmap target from parent_spt to sub_spt.add more details in
commit message. All suggested by Zhenyu
v4:
- fix by undo the mapping of DMA address and free sub_spt suggested by Zhi
v3:
- correct spelling mistake and remove unused variable suggested by Greg
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/
---
drivers/gpu/drm/i915/gvt/gtt.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 51e5e8fb505b..7379e8d98417 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -1209,10 +1209,8 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
for_each_shadow_entry(sub_spt, &sub_se, sub_index) {
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
PAGE_SIZE, &dma_addr);
- if (ret) {
- ppgtt_invalidate_spt(spt);
- return ret;
- }
+ if (ret)
+ goto err;
sub_se.val64 = se->val64;
/* Copy the PAT field from PDE. */
@@ -1231,6 +1229,17 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
ops->set_pfn(se, sub_spt->shadow_page.mfn);
ppgtt_set_shadow_entry(spt, se, index);
return 0;
+err:
+ /* Cancel the existing addess mappings of DMA addr. */
+ for_each_present_shadow_entry(sub_spt, &sub_se, sub_index) {
+ gvt_vdbg_mm("invalidate 4K entry\n");
+ ppgtt_invalidate_pte(sub_spt, &sub_se);
+ }
+ /* Release the new allocated spt. */
+ trace_spt_change(sub_spt->vgpu->id, "release", sub_spt,
+ sub_spt->guest_page.gfn, sub_spt->shadow_page.type);
+ ppgtt_free_spt(sub_spt);
+ return ret;
}
static int split_64KB_gtt_entry(struct intel_vgpu *vgpu,
--
2.25.1