[RESEND PATCH v4] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry

[Zheng Hacker]

Zhenyu Wang <zhenyuw at 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