linux-kernel - RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h not asm/mshyperv.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 12 Nov 2022 21:55:56 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>
CC:     "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
        Andy Lutomirski <luto@...nel.org>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>
Subject: RE: [PATCH] clocksource/drivers/hyper-v: Include asm/hyperv-tlfs.h
 not asm/mshyperv.h

From: Thomas Gleixner <tglx@...utronix.de> Sent: Saturday, November 12, 2022 11:03 AM
> 
> clocksource/hyperv_timer.h is included into the VDSO build. It includes
> asm/mshyperv.h which in turn includes the world and some more.
> 
> This worked so far by chance, but any subtle change in the include chain
> results in a build breakage because VDSO builds are building user space
> libraries.
> 
> Include asm/hyperv-tlfs.h instead which contains everything what the VDSO
> build needs and move the hv_get_raw_timer() define into the header file.

We've been keeping x86-isms out of hyperv_timer.c and hyperv_timer.h so
that they are architecture independent.  That's why the #define for
hv_get_raw_timer() is in an x86-specific include file.

But I can see the problem with too much getting dragged into the VDSO
builds.  If hv_get_raw_timer() is added to hyperv_timer.h, it should
be under #ifdef CONFIG_X86.  Adding an #ifdef isn't ideal, and a more
more proper solution might be to have a separate hyperv_timer.h include
file under arch/x86/include/asm.  But the latter seems like overkill for just
hv_get_raw_timer(), so I'm OK with the #ifdef.

Or does someone have a better idea?

Michael

> 
> Fixup drivers/hv/vmbus_drv.c which relies on the indirect include of
> asm/mshyperv.h.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  arch/x86/include/asm/mshyperv.h    |    2 --
>  drivers/hv/vmbus_drv.c             |    1 +
>  include/clocksource/hyperv_timer.h |    4 +++-
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -19,8 +19,6 @@ typedef int (*hyperv_fill_flush_list_fun
>  		struct hv_guest_mapping_flush_list *flush,
>  		void *data);
> 
> -#define hv_get_raw_timer() rdtsc_ordered()
> -
>  void hyperv_vector_handler(struct pt_regs *regs);
> 
>  #if IS_ENABLED(CONFIG_HYPERV)
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -37,6 +37,7 @@
>  #include <linux/dma-map-ops.h>
>  #include <linux/pci.h>
>  #include <clocksource/hyperv_timer.h>
> +#include <asm/mshyperv.h>
>  #include "hyperv_vmbus.h"
> 
>  struct vmbus_dynid {
> --- a/include/clocksource/hyperv_timer.h
> +++ b/include/clocksource/hyperv_timer.h
> @@ -15,7 +15,7 @@
> 
>  #include <linux/clocksource.h>
>  #include <linux/math64.h>
> -#include <asm/mshyperv.h>
> +#include <asm/hyperv-tlfs.h>
> 
>  #define HV_MAX_MAX_DELTA_TICKS 0xffffffff
>  #define HV_MIN_DELTA_TICKS 1
> @@ -34,6 +34,8 @@ extern void hv_init_clocksource(void);
> 
>  extern struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
> 
> +#define hv_get_raw_timer() rdtsc_ordered()
> +
>  static inline notrace u64
>  hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
>  {