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