On Fri, 9 Mar 2018, Vitaly Kuznetsov wrote:
> @@ -198,6 +218,12 @@ static int hv_cpu_die(unsigned int cpu)
> struct hv_reenlightenment_control re_ctrl;
> unsigned int new_cpu;
>
> + if (hv_vp_assist_page && hv_vp_assist_page[cpu]) {
> + wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);
> + vfree(hv_vp_assist_page[cpu]);
> + hv_vp_assist_page[cpu] = NULL;
So this is freed before the CPU is actually dead. And this runs in
preemtible context. Is the wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0); enough to
prevent eventual users of the assist page on the outgoing CPU from
accessing it?
> if (hv_reenlightenment_cb == NULL)
> return 0;
>
> @@ -241,6 +267,13 @@ void hyperv_init(void)
> if (!hv_vp_index)
> return;
>
> + hv_vp_assist_page = kcalloc(num_possible_cpus(),
> + sizeof(*hv_vp_assist_page), GFP_KERNEL);
> + if (!hv_vp_assist_page) {
> + ms_hyperv.hints &= ~HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
> + return;
> + }
> +
> if (cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "x86/hyperv_init:online",
> hv_cpu_init, hv_cpu_die) < 0)
> goto free_vp_index;
Shouldn't you free hv_vp_assist_page in the error path?
> +extern struct hv_vp_assist_page **hv_vp_assist_page;
> +
> +static inline struct hv_vp_assist_page *hv_get_vp_assist_page(unsigned int cpu)
> +{
> + return hv_vp_assist_page[cpu];
Shouldn't that check hv_vp_assist_page != NULL?
Thanks,
tglx
