RE: [PATCH] mshv_vtl: Check per-CPU register page before mmap

[Michael Kelley]

From: Hardik Garg <hargar@xxxxxxxxxxxxxxxxxxx>
> 
> Register-page setup is optional for each CPU. If allocation or
> registration fails, mshv_vtl_configure_reg_page() warns and leaves that
> CPU's reg_page NULL, but initial context setup continues successfully.
> Meanwhile, successful setup on another CPU sets the global
> mshv_has_reg_page flag.
> 
> mshv_vtl_fault() checks this global flag before selecting the requested
> CPU's register page. With mixed setup results across online CPUs, the
> check passes even for a CPU with no register page, reaching
> get_page(NULL) when userspace faults in that mapping.

I'm a bit late in reviewing this because I was travelling all last week.

This patch seems like it is just papering over the real problem, which
is that a global variable like mshv_has_reg_page can't represent the
status of an operation that may succeed or fail on a per-cpu basis.
I pointed this out (as did Sashiko) in review comments back in
April [1].

The only other place mshv_has_reg_page is used is in 
mshv_ioctl_check_extensions() where its value is returned from an
ioctl() system call made by user space. That's a questionable practice
since ioctl() usually returns 0 on success, though the man page for
ioctl() does admit that some ioctls use the return value as an output
parameter and return a non-negative value on success. But even
then, a single value can't accurately reflect the status of an
operation that may succeed or fail on a per-cpu basis. User
space would presumably have a similar problem to what is
being fixed by this patch.

Is there any reason that the real problem couldn't be fixed
instead of doing this fix on top of something that is fundamentally
broken?

Michael

[1] https://lore.kernel.org/linux-hyperv/SN6PR02MB4157CF364DA2C0CC657A6DCBD450A@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

> 
> Check the selected per-CPU register page before taking its reference and
> return VM_FAULT_SIGBUS if it is absent.
> 
> Fixes: 7bfe3b8ea6e3 ("Drivers: hv: Introduce mshv_vtl driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Hardik Garg <hargar@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/hv/mshv_vtl_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hv/mshv_vtl_main.c b/drivers/hv/mshv_vtl_main.c
> index fc993560a45c..3296ab5c0243 100644
> --- a/drivers/hv/mshv_vtl_main.c
> +++ b/drivers/hv/mshv_vtl_main.c
> @@ -912,6 +912,9 @@ static vm_fault_t mshv_vtl_fault(struct vm_fault *vmf)
>  		return VM_FAULT_NOPAGE;
>  	}
> 
> +	if (!page)
> +		return VM_FAULT_SIGBUS;
> +
>  	get_page(page);
>  	vmf->page = page;
>