On Thu, Nov 17, 2022 at 04:32:00PM +0000, Sean Christopherson wrote:
> On Thu, Nov 17, 2022, Wei Liu wrote:
> > On Wed, Nov 16, 2022 at 10:41:36AM -0800, Michael Kelley wrote:
> > [...]
> > >
> > > +static void hv_pci_read_mmio(struct device *dev, phys_addr_t gpa, int size, u32 *val)
> > > +{
> > > + struct hv_mmio_read_input *in;
> > > + struct hv_mmio_read_output *out;
> > > + u64 ret;
> > > +
> > > + /*
> > > + * Must be called with interrupts disabled so it is safe
> > > + * to use the per-cpu input argument page. Use it for
> > > + * both input and output.
> > > + */
>
> There's no need to require interrupts to be disabled to safely use a per-cpu
> variable, simply disabling preemption also provides the necessary protection.
> And this_cpu_ptr() will complain with CONFIG_DEBUG_PREEMPT=y if preemption isn't
> disabled.
>
> IIUC, based on the existing code, what is really be guarded against is an IRQ arriving
> and initiating a different hypercall from IRQ context, and thus corrupting the page
> from this function's perspective.
Exactly. Michael's comment did not say this explicitly but that's what's
being guarded.
>
> > Perhaps adding something along this line?
> >
> > WARN_ON(!irqs_disabled());
>
> Given that every use of hyperv_pcpu_input_arg except hv_common_cpu_init() disables
> IRQs, what about adding a helper to retrieve the pointer and assert that IRQs are
> disabled? I.e. add the sanity for all usage, not just this one-off case.
>
We can potentially introduce a pair of get/put functions for these pages,
but let's not feature-creep here...
> And since CPUHP_AP_ONLINE_DYN => hv_common_cpu_init() runs after scheduling is
> activated by CPUHP_AP_SCHED_WAIT_EMPTY, I believe that hv_common_cpu_init() is
> theoretically broken. Maybe someone can look at that when fixing he KVM vs.
> Hyper-V issue?
>
> https://lore.kernel.org/linux-hyperv/878rkqr7ku.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxx
> https://lore.kernel.org/all/87sfikmuop.fsf@xxxxxxxxxx
I read the mails before have not looked into those since they are only
theoretical per those threads. Sorry.
The only scenario I can think of for CPU hotplug right now is the
(upcoming) Linux root kernel, I guess we will cross the bridge when we
get there.
Thanks,
Wei.
