* [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering
@ 2021-07-29 13:37 Siddharth Chandrasekaran
2021-07-29 13:52 ` Vitaly Kuznetsov
0 siblings, 1 reply; 6+ messages in thread
From: Siddharth Chandrasekaran @ 2021-07-29 13:37 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Dexuan Cui, Arnd Bergmann
Cc: Siddharth Chandrasekaran, Liran Alon, Ioannis Aslanidis,
linux-hyperv, linux-arch, linux-kernel, Siddharth Chandrasekaran
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 <sidcha@amazon.de>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering
2021-07-29 13:37 [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering Siddharth Chandrasekaran
@ 2021-07-29 13:52 ` Vitaly Kuznetsov
2021-07-29 14:07 ` Wei Liu
0 siblings, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2021-07-29 13:52 UTC (permalink / raw)
To: Siddharth Chandrasekaran
Cc: Siddharth Chandrasekaran, Liran Alon, Ioannis Aslanidis,
linux-hyperv, linux-arch, linux-kernel, Siddharth Chandrasekaran,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Dexuan Cui, Arnd Bergmann
Siddharth Chandrasekaran <sidcha@amazon.de> 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 <sidcha@amazon.de>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering
2021-07-29 13:52 ` Vitaly Kuznetsov
@ 2021-07-29 14:07 ` Wei Liu
2021-07-29 14:26 ` Siddharth Chandrasekaran
0 siblings, 1 reply; 6+ messages in thread
From: Wei Liu @ 2021-07-29 14:07 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Siddharth Chandrasekaran, Siddharth Chandrasekaran, Liran Alon,
Ioannis Aslanidis, linux-hyperv, linux-arch, linux-kernel,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Dexuan Cui, Arnd Bergmann
On Thu, Jul 29, 2021 at 03:52:46PM +0200, Vitaly Kuznetsov wrote:
> Siddharth Chandrasekaran <sidcha@amazon.de> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering
2021-07-29 14:07 ` Wei Liu
@ 2021-07-29 14:26 ` Siddharth Chandrasekaran
2021-07-29 16:56 ` Wei Liu
0 siblings, 1 reply; 6+ messages in thread
From: Siddharth Chandrasekaran @ 2021-07-29 14:26 UTC (permalink / raw)
To: Wei Liu
Cc: Vitaly Kuznetsov, Siddharth Chandrasekaran, Liran Alon,
Ioannis Aslanidis, linux-hyperv, linux-arch, linux-kernel,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui,
Arnd Bergmann
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 <sidcha@amazon.de> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering
2021-07-29 14:26 ` Siddharth Chandrasekaran
@ 2021-07-29 16:56 ` Wei Liu
2021-07-30 9:35 ` Siddharth Chandrasekaran
0 siblings, 1 reply; 6+ messages in thread
From: Wei Liu @ 2021-07-29 16:56 UTC (permalink / raw)
To: Siddharth Chandrasekaran
Cc: Wei Liu, Vitaly Kuznetsov, Siddharth Chandrasekaran, Liran Alon,
Ioannis Aslanidis, linux-hyperv, linux-arch, linux-kernel,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui,
Arnd Bergmann
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 <sidcha@amazon.de> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering
2021-07-29 16:56 ` Wei Liu
@ 2021-07-30 9:35 ` Siddharth Chandrasekaran
0 siblings, 0 replies; 6+ messages in thread
From: Siddharth Chandrasekaran @ 2021-07-30 9:35 UTC (permalink / raw)
To: Wei Liu
Cc: Vitaly Kuznetsov, Siddharth Chandrasekaran, Liran Alon,
Ioannis Aslanidis, linux-hyperv, linux-arch, linux-kernel,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui,
Arnd Bergmann
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 <sidcha@amazon.de> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-30 9:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-29 13:37 [PATCH] asm-generic/hyperv: Fix struct hv_message_header ordering Siddharth Chandrasekaran
2021-07-29 13:52 ` Vitaly Kuznetsov
2021-07-29 14:07 ` Wei Liu
2021-07-29 14:26 ` Siddharth Chandrasekaran
2021-07-29 16:56 ` Wei Liu
2021-07-30 9:35 ` Siddharth Chandrasekaran
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).