[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/3] x86: fix resource leaks on arch_vcpu_create() error path

  • To: Jan Beulich <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Fri, 2 Oct 2020 12:13:09 +0100
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Fri, 02 Oct 2020 11:14:45 +0000
  • Ironport-sdr: f/pLEhB4TAsLSU9Nomz70Xt+CII5FRAqlZHbo4qYL1uGk4OkyDPoLPFhKZnoBabTaXJnqw18hW B0Bg7LTTNaiLlmm4UxG7lWMHHc3XZaOR4rq4PvDSJYLSpGRogl73FcHNHfYnzVGEmTVXi2HEKF kPN8vgw9sCTrkPItrFX8WXMoAU8e2rpRT3QDMmkSx4Fyx1n8q0v0nSMyLQyTgZkLCOGj3MzMHs yj8kRlDJmqgsVBTi9notaZcpKMASgjbxtAh1gAmBdN629rECGKZZLbyF2m/zHZadP0zBuaX2yP o0Q=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 02/10/2020 11:30, Jan Beulich wrote:
> {hvm,pv}_vcpu_initialise() have always been meant to be the final
> possible source of errors in arch_vcpu_create(), hence not requiring
> any unrolling of what they've done on the error path. (Of course this
> may change once the various involved paths all have become idempotent.)

I'd agree that the way the code was previously laid out expected
{hvm,pv}_vcpu_initialise() to be the final failing option.

I don't think "has always meant to be" is reasonable, because where is
the code comment explaining this design choice?

The idempotent plans will definitely be removing this misbehaviour, and
the memory leaks it causes.

> But even beyond this aspect I think it is more logical to do policy
> initialization ahead of the calling of these two functions, as they may
> in principle want to access it.

Not these MSRs.  They're currently a block of zeroes, and while that
will eventually change, it will still be a bunch of MSRs in their RESET

The interesting MSRs are the domain ones, not the vCPU ones.

> Fixes: 4187f79dc718 ("x86/msr: introduce struct msr_vcpu_policy")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> although I'd
prefer some adjustment to the commit message along the indicated lines.



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.