lkml.org 
[lkml]   [2022]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RESEND PATCH v4] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
Zhenyu Wang <zhenyuw@linux.intel.com> 于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

\
 
 \ /
  Last update: 2023-03-26 23:15    [W:0.049 / U:0.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site