2021-07-29 13:39:44

by Siddharth Chandrasekaran

[permalink] [raw]
Subject: [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering

According to Hyper-V TLFS Version 6.0b, struct hv_message_header members
should be defined in the order:

message_type, reserved, message_flags, payload_size

but we have it defined in the order:

message_type, payload_size, message_flags, reserved

that is, the payload_size and reserved members swapped. Due to this mix
up, we were inadvertently causing two issues:

- The payload_size field has invalid data; it didn't cause an issue
so far because we are delivering only timer messages which has fixed
size payloads the guest probably did a sizeof(payload) instead
relying on the value of payload_size member.

- The message_flags was always delivered as 0 to the guest;
fortunately, according to section 13.3.1 message_flags is also
treated as a reserved field.

Although this is not causing an issue now, it might in future (we are
adding more message types in our VSM implementation) so fix it to
reflect the specification.

Signed-off-by: Siddharth Chandrasekaran <[email protected]>
---
include/asm-generic/hyperv-tlfs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
index 56348a541c50..a5540e9b171f 100644
--- a/include/asm-generic/hyperv-tlfs.h
+++ b/include/asm-generic/hyperv-tlfs.h
@@ -284,9 +284,9 @@ union hv_port_id {
/* Define synthetic interrupt controller message header. */
struct hv_message_header {
__u32 message_type;
- __u8 payload_size;
- union hv_message_flags message_flags;
__u8 reserved[2];
+ union hv_message_flags message_flags;
+ __u8 payload_size;
union {
__u64 sender;
union hv_port_id port;
--
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879





2021-07-29 13:55:06

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering

Siddharth Chandrasekaran <[email protected]> writes:

> According to Hyper-V TLFS Version 6.0b, struct hv_message_header members
> should be defined in the order:
>
> message_type, reserved, message_flags, payload_size
>
> but we have it defined in the order:
>
> message_type, payload_size, message_flags, reserved
>
> that is, the payload_size and reserved members swapped.

Indeed,

typedef struct
{
HV_MESSAGE_TYPE MessageType;
UINT16 Reserved;
HV_MESSAGE_FLAGS MessageFlags;
UINT8 PayloadSize;
union
{
UINT64 OriginationId;
HV_PARTITION_ID Sender;
HV_PORT_ID Port;
};
} HV_MESSAGE_HEADER;

> Due to this mix
> up, we were inadvertently causing two issues:
>
> - The payload_size field has invalid data; it didn't cause an issue
> so far because we are delivering only timer messages which has fixed
> size payloads the guest probably did a sizeof(payload) instead
> relying on the value of payload_size member.
>
> - The message_flags was always delivered as 0 to the guest;
> fortunately, according to section 13.3.1 message_flags is also
> treated as a reserved field.
>
> Although this is not causing an issue now, it might in future (we are
> adding more message types in our VSM implementation) so fix it to
> reflect the specification.

I'm wondering how this works for Linux-on-Hyper-V as
e.g. vmbus_on_msg_dpc() checks for mininum and maximum payload length:

payload_size = msg_copy.header.payload_size;
if (payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT) {
WARN_ONCE(1, "payload size is too large (%d)\n", payload_size);
goto msg_handled;
}

entry = &channel_message_table[msgtype];

if (!entry->message_handler)
goto msg_handled;

if (payload_size < entry->min_payload_len) {
WARN_ONCE(1, "message too short: msgtype=%d len=%d\n", msgtype, payload_size);
goto msg_handled;
}

Maybe it's vice versa, the header is correct and the documentation is wrong?

>
> Signed-off-by: Siddharth Chandrasekaran <[email protected]>
> ---
> include/asm-generic/hyperv-tlfs.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
> index 56348a541c50..a5540e9b171f 100644
> --- a/include/asm-generic/hyperv-tlfs.h
> +++ b/include/asm-generic/hyperv-tlfs.h
> @@ -284,9 +284,9 @@ union hv_port_id {
> /* Define synthetic interrupt controller message header. */
> struct hv_message_header {
> __u32 message_type;
> - __u8 payload_size;
> - union hv_message_flags message_flags;
> __u8 reserved[2];
> + union hv_message_flags message_flags;
> + __u8 payload_size;
> union {
> __u64 sender;
> union hv_port_id port;

--
Vitaly


2021-07-29 14:13:14

by Wei Liu

[permalink] [raw]
Subject: Re: [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering

On Thu, Jul 29, 2021 at 03:52:46PM +0200, Vitaly Kuznetsov wrote:
> Siddharth Chandrasekaran <[email protected]> writes:
>
> > According to Hyper-V TLFS Version 6.0b, struct hv_message_header members
> > should be defined in the order:
> >
> > message_type, reserved, message_flags, payload_size
> >
> > but we have it defined in the order:
> >
> > message_type, payload_size, message_flags, reserved
> >
> > that is, the payload_size and reserved members swapped.
>
> Indeed,
>
> typedef struct
> {
> HV_MESSAGE_TYPE MessageType;
> UINT16 Reserved;
> HV_MESSAGE_FLAGS MessageFlags;
> UINT8 PayloadSize;
> union
> {
> UINT64 OriginationId;
> HV_PARTITION_ID Sender;
> HV_PORT_ID Port;
> };
> } HV_MESSAGE_HEADER;

Well. I think TLFS is wrong. Let me ask around.

Wei.

2021-07-29 14:28:09

by Siddharth Chandrasekaran

[permalink] [raw]
Subject: Re: [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering

On Thu, Jul 29, 2021 at 02:07:05PM +0000, Wei Liu wrote:
> On Thu, Jul 29, 2021 at 03:52:46PM +0200, Vitaly Kuznetsov wrote:
> > Siddharth Chandrasekaran <[email protected]> writes:
> >
> > > According to Hyper-V TLFS Version 6.0b, struct hv_message_header members
> > > should be defined in the order:
> > >
> > > message_type, reserved, message_flags, payload_size
> > >
> > > but we have it defined in the order:
> > >
> > > message_type, payload_size, message_flags, reserved
> > >
> > > that is, the payload_size and reserved members swapped.
> >
> > Indeed,
> >
> > typedef struct
> > {
> > HV_MESSAGE_TYPE MessageType;
> > UINT16 Reserved;
> > HV_MESSAGE_FLAGS MessageFlags;
> > UINT8 PayloadSize;
> > union
> > {
> > UINT64 OriginationId;
> > HV_PARTITION_ID Sender;
> > HV_PORT_ID Port;
> > };
> > } HV_MESSAGE_HEADER;
>
> Well. I think TLFS is wrong. Let me ask around.

TBH, I hadn't considered that possibility :). I assumed it was a
regression on our side. But I spent some time tracing the history of that
struct all the way back to when it was in staging (in 2009) and now I'm
inclined to believe a later version of TLFS is at fault here.

Based on what we decide in this thread, I will open an issue on the TLFS
GitHub repository.

~ Sid.



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




2021-07-29 16:57:47

by Wei Liu

[permalink] [raw]
Subject: Re: [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering

On Thu, Jul 29, 2021 at 04:26:54PM +0200, Siddharth Chandrasekaran wrote:
> On Thu, Jul 29, 2021 at 02:07:05PM +0000, Wei Liu wrote:
> > On Thu, Jul 29, 2021 at 03:52:46PM +0200, Vitaly Kuznetsov wrote:
> > > Siddharth Chandrasekaran <[email protected]> writes:
> > >
> > > > According to Hyper-V TLFS Version 6.0b, struct hv_message_header members
> > > > should be defined in the order:
> > > >
> > > > message_type, reserved, message_flags, payload_size
> > > >
> > > > but we have it defined in the order:
> > > >
> > > > message_type, payload_size, message_flags, reserved
> > > >
> > > > that is, the payload_size and reserved members swapped.
> > >
> > > Indeed,
> > >
> > > typedef struct
> > > {
> > > HV_MESSAGE_TYPE MessageType;
> > > UINT16 Reserved;
> > > HV_MESSAGE_FLAGS MessageFlags;
> > > UINT8 PayloadSize;
> > > union
> > > {
> > > UINT64 OriginationId;
> > > HV_PARTITION_ID Sender;
> > > HV_PORT_ID Port;
> > > };
> > > } HV_MESSAGE_HEADER;
> >
> > Well. I think TLFS is wrong. Let me ask around.
>
> TBH, I hadn't considered that possibility :). I assumed it was a
> regression on our side. But I spent some time tracing the history of that
> struct all the way back to when it was in staging (in 2009) and now I'm
> inclined to believe a later version of TLFS is at fault here.
>
> Based on what we decide in this thread, I will open an issue on the TLFS
> GitHub repository.
>

I have confirmation TLFS is wrong and shall be fixed. Feel free to open
an issue on GitHub too.

Wei.

2021-07-30 09:38:03

by Siddharth Chandrasekaran

[permalink] [raw]
Subject: Re: [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering

On Thu, Jul 29, 2021 at 04:56:38PM +0000, Wei Liu wrote:
> On Thu, Jul 29, 2021 at 04:26:54PM +0200, Siddharth Chandrasekaran wrote:
> > On Thu, Jul 29, 2021 at 02:07:05PM +0000, Wei Liu wrote:
> > > On Thu, Jul 29, 2021 at 03:52:46PM +0200, Vitaly Kuznetsov wrote:
> > > > Siddharth Chandrasekaran <[email protected]> writes:
> > > >
> > > > > According to Hyper-V TLFS Version 6.0b, struct hv_message_header members
> > > > > should be defined in the order:
> > > > >
> > > > > message_type, reserved, message_flags, payload_size
> > > > >
> > > > > but we have it defined in the order:
> > > > >
> > > > > message_type, payload_size, message_flags, reserved
> > > > >
> > > > > that is, the payload_size and reserved members swapped.
> > > >
> > > > Indeed,
> > > >
> > > > typedef struct
> > > > {
> > > > HV_MESSAGE_TYPE MessageType;
> > > > UINT16 Reserved;
> > > > HV_MESSAGE_FLAGS MessageFlags;
> > > > UINT8 PayloadSize;
> > > > union
> > > > {
> > > > UINT64 OriginationId;
> > > > HV_PARTITION_ID Sender;
> > > > HV_PORT_ID Port;
> > > > };
> > > > } HV_MESSAGE_HEADER;
> > >
> > > Well. I think TLFS is wrong. Let me ask around.
> >
> > TBH, I hadn't considered that possibility :). I assumed it was a
> > regression on our side. But I spent some time tracing the history of that
> > struct all the way back to when it was in staging (in 2009) and now I'm
> > inclined to believe a later version of TLFS is at fault here.
> >
> > Based on what we decide in this thread, I will open an issue on the TLFS
> > GitHub repository.
> >
>
> I have confirmation TLFS is wrong and shall be fixed. Feel free to open
> an issue on GitHub too.

Thanks for the confirmation, I created an issue [1] to track this.

~ Sid.

[1]: https://github.com/MicrosoftDocs/Virtualization-Documentation/issues/1624



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




2021-07-31 06:34:43

by kernel test robot

[permalink] [raw]
Subject: [asm] b5b51922df: kvm-unit-tests.hyperv_stimer.fail



Greeting,

FYI, we noticed the following commit (built with gcc-9):

commit: b5b51922dfc59033acfd0d59cdc26cca9b4388dc ("[PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering")
url: https://github.com/0day-ci/linux/commits/Siddharth-Chandrasekaran/asm-generic-hyperv-Fix-struct-hv_message_header-ordering/20210729-213824
base: https://git.kernel.org/cgit/linux/kernel/git/arnd/asm-generic.git master

in testcase: kvm-unit-tests
version: kvm-unit-tests-x86_64-bc6f264-1_20210701
with following parameters:

ucode: 0xde



on test machine: 4 threads 1 sockets Intel(R) Core(TM) i7-7567U CPU @ 3.50GHz with 32G memory

caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):




If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>

2021-07-30 16:01:53 ./run_tests.sh
PASS apic-split (56 tests)
PASS ioapic-split (19 tests)
PASS apic (56 tests)
PASS ioapic (26 tests)
SKIP cmpxchg8b (i386 only)
PASS smptest (1 tests)
PASS smptest3 (1 tests)
PASS vmexit_cpuid
PASS vmexit_vmcall
PASS vmexit_mov_from_cr8
PASS vmexit_mov_to_cr8
PASS vmexit_inl_pmtimer
PASS vmexit_ipi
PASS vmexit_ipi_halt
PASS vmexit_ple_round_robin
PASS vmexit_tscdeadline
PASS vmexit_tscdeadline_immed
PASS access
SKIP access-reduced-maxphyaddr (/sys/module/kvm_intel/parameters/allow_smaller_maxphyaddr not equal to Y)
PASS smap (18 tests)
SKIP pku (0 tests)
SKIP pks (0 tests)
SKIP asyncpf (0 tests)
PASS emulator (135 tests, 2 skipped)
PASS eventinj (13 tests)
PASS hypercall (2 tests)
PASS idt_test (4 tests)
PASS memory (8 tests)
PASS msr (17 tests)
FAIL pmu (142 tests, 10 unexpected failures)
PASS pmu_lbr (3 tests)
PASS vmware_backdoors (11 tests)
PASS realmode
PASS s3
PASS setjmp (10 tests)
PASS sieve
PASS syscall (2 tests)
PASS tsc (3 tests)
PASS tsc_adjust (5 tests)
PASS xsave (17 tests)
PASS rmap_chain
SKIP svm (0 tests)
SKIP taskswitch (i386 only)
SKIP taskswitch2 (i386 only)
PASS kvmclock_test
PASS pcid-enabled (2 tests)
PASS pcid-disabled (2 tests)
PASS pcid-asymmetric (2 tests)
PASS rdpru (1 tests)
PASS umip (21 tests)
SKIP la57 (i386 only)
FAIL vmx (429297 tests, 1 unexpected failures, 2 expected failures, 7 skipped)
SKIP ept (0 tests)
SKIP vmx_eoi_bitmap_ioapic_scan (1 tests, 1 skipped)
SKIP vmx_hlt_with_rvi_test (1 tests, 1 skipped)
SKIP vmx_apicv_test (2 tests, 2 skipped)
PASS vmx_apic_passthrough_thread (8 tests)
PASS vmx_init_signal_test (9 tests)
PASS vmx_sipi_signal_test (11 tests)
PASS vmx_apic_passthrough_tpr_threshold_test (6 tests)
PASS vmx_vmcs_shadow_test (142173 tests)
PASS debug (13 tests)
PASS hyperv_synic (1 tests)
PASS hyperv_connections (7 tests)
FAIL hyperv_stimer (2 tests, 2 unexpected failures)
PASS hyperv_clock
PASS intel_iommu (11 tests)
SKIP tsx-ctrl (1 tests, 1 skipped)
SKIP intel_cet (0 tests)



To reproduce:

git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
bin/lkp run generated-yaml-file



---
0DAY/LKP+ Test Infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation

Thanks,
Oliver Sang


Attachments:
(No filename) (3.99 kB)
config-5.13.0-rc1-00019-gb5b51922dfc5 (176.63 kB)
job-script (5.41 kB)
kmsg.xz (20.58 kB)
kvm-unit-tests (2.64 kB)
job.yaml (4.35 kB)
reproduce (16.00 B)
Download all attachments