On Wed, Aug 14, 2024 at 09:20:06AM +0800,
Yuan Yao <yuan.yao@xxxxxxxxxxxxxxx> wrote:
> > > > + ret = -EIO;
> > > > + pr_tdx_error(TDH_VP_CREATE, err);
> > > > + goto free_tdvpx;
> > > > + }
> > > > +
> > > > + for (i = 0; i < tdx_sysinfo_nr_tdcx_pages(); i++) {
> > > > + va = __get_free_page(GFP_KERNEL_ACCOUNT);
> > > > + if (!va) {
> > > > + ret = -ENOMEM;
> > > > + goto free_tdvpx;
> > >
> > > It's possible that some pages already added into TD by
> > > tdh_vp_addcx() below and they won't be handled by
> > > tdx_vcpu_free() if goto free_tdvpx here;
> >
> > Due to TDX TD state check, we can't free partially assigned TDCS pages.
> > TDX module seems to assume that TDH.VP.ADDCX() won't fail in the middle.
>
> The already partially added TDCX pages are initialized by
> MOVDIR64 with the TD's private HKID in TDX module, the above
> 'goto free_tdvpx' frees them back to kernel directly w/o
> take back the ownership with shared HKID. This violates the
> rule that a page's ownership should be taken back with shared
> HKID before release to kernel if they were initialized by any
> private HKID before.
>
> How about do tdh_vp_addcx() afer allocated all TDCX pages
> and give WARN_ON_ONCE() to the return value of
> tdh_vp_addcx() if the tdh_vp_addcx() won't fail except some
> BUG inside TDX module in our current usage ?
Yes, that makes sense. Those error recovery paths need to be simplified.
--
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
