(Extending Cc: list,)
On Sun, Jan 09, 2022 at 05:55:16PM +0800, Yanming Liu wrote:
Commit adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer") introduced a notion of maximum packet size in vmbus channel and used that size to initialize a buffer holding all incoming packet along with their vmbus packet header. Currently, some vmbus drivers set max_pkt_size to the size of their receive buffer passed to vmbus_recvpacket, however vmbus_open expects this size to also include vmbus packet header. This leads to corruption of the ring buffer state when receiving a maximum sized packet.
Specifically, in hv_balloon I have observed of a dm_unballoon_request message of 4096 bytes being truncated to 4080 bytes. When the driver tries to read next packet it starts from a wrong read_index, receives garbage and prints a lot of "Unhandled message: type: <garbage>" in dmesg.
The same mismatch also happens in hv_fcopy, hv_kvp, hv_snapshot, hv_util, hyperv_drm and hyperv_fb, though bad cases are not observed yet.
Allocate the buffer with HV_HYP_PAGE_SIZE more bytes to make room for the descriptor, assuming the vmbus packet header will never be larger than HV_HYP_PAGE_SIZE. This is essentially free compared to just adding 'sizeof(struct vmpacket_descriptor)' because these buffers are all more than HV_HYP_PAGE_SIZE bytes so kmalloc rounds them up anyway.
Fixes: adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer") Suggested-by: Andrea Parri (Microsoft) parri.andrea@gmail.com Signed-off-by: Yanming Liu yanminglr@gmail.com
Thanks for sorting this out; the patch looks good to me:
Reviewed-by: Andrea Parri (Microsoft) parri.andrea@gmail.com
In future submissions (if any), please include LKML as well as subsystem lists&maintainers; scripts/get_maintainer.pl can be useful to this end.
Andrea
v2: Changed to modify max_pkt_size in individual drivers instead of in vmbus code as suggested by Andrea Parri.
drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 2 ++ drivers/hv/hv_balloon.c | 7 +++++++ drivers/hv/hv_fcopy.c | 2 +- drivers/hv/hv_kvp.c | 2 +- drivers/hv/hv_snapshot.c | 2 +- drivers/hv/hv_util.c | 17 +++++++++++++++++ drivers/video/fbdev/hyperv_fb.c | 2 ++ 7 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c index c0155c6271bf..bf1548054276 100644 --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c @@ -478,6 +478,8 @@ int hyperv_connect_vsp(struct hv_device *hdev) struct drm_device *dev = &hv->dev; int ret;
- hdev->channel->max_pkt_size = HV_HYP_PAGE_SIZE + VMBUS_MAX_PACKET_SIZE;
- ret = vmbus_open(hdev->channel, VMBUS_RING_BUFSIZE, VMBUS_RING_BUFSIZE, NULL, 0, hyperv_receive, hdev); if (ret) {
diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c index ca873a3b98db..ee2527c3d3b8 100644 --- a/drivers/hv/hv_balloon.c +++ b/drivers/hv/hv_balloon.c @@ -1660,6 +1660,13 @@ static int balloon_connect_vsp(struct hv_device *dev) unsigned long t; int ret;
- /*
* max_pkt_size should be large enough for one vmbus packet header plus* our receive buffer size. We assume vmbus packet header is smaller* than HV_HYP_PAGE_SIZE.*/- dev->channel->max_pkt_size = HV_HYP_PAGE_SIZE * 2;
- ret = vmbus_open(dev->channel, dm_ring_size, dm_ring_size, NULL, 0, balloon_onchannelcallback, dev); if (ret)
diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c index 660036da7449..07a508ce65db 100644 --- a/drivers/hv/hv_fcopy.c +++ b/drivers/hv/hv_fcopy.c @@ -349,7 +349,7 @@ int hv_fcopy_init(struct hv_util_service *srv) { recv_buffer = srv->recv_buffer; fcopy_transaction.recv_channel = srv->channel;
- fcopy_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 2;
fcopy_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 3;
/*
- When this driver loads, the user level daemon that
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c index c698592b83e4..b85d725ae5b1 100644 --- a/drivers/hv/hv_kvp.c +++ b/drivers/hv/hv_kvp.c @@ -757,7 +757,7 @@ hv_kvp_init(struct hv_util_service *srv) { recv_buffer = srv->recv_buffer; kvp_transaction.recv_channel = srv->channel;
- kvp_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 4;
kvp_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 5;
/*
- When this driver loads, the user level daemon that
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c index 6018b9d1b1fb..dba6baacbf17 100644 --- a/drivers/hv/hv_snapshot.c +++ b/drivers/hv/hv_snapshot.c @@ -375,7 +375,7 @@ hv_vss_init(struct hv_util_service *srv) } recv_buffer = srv->recv_buffer; vss_transaction.recv_channel = srv->channel;
- vss_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 2;
vss_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 3;
/*
- When this driver loads, the user level daemon that
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index 835e6039c186..a7b88c067c07 100644 --- a/drivers/hv/hv_util.c +++ b/drivers/hv/hv_util.c @@ -112,6 +112,8 @@ static int hv_shutdown_init(struct hv_util_service *srv)
hibernation_supported = hv_is_hibernation_supported();
- channel->max_pkt_size = HV_HYP_PAGE_SIZE * 2;
- return 0;
}
@@ -133,9 +135,11 @@ static struct hv_util_service util_timesynch = { .util_deinit = hv_timesync_deinit, };
+static int heartbeat_init(struct hv_util_service *src); static void heartbeat_onchannelcallback(void *context); static struct hv_util_service util_heartbeat = { .util_cb = heartbeat_onchannelcallback,
- .util_init = heartbeat_init,
};
static struct hv_util_service util_kvp = { @@ -553,6 +557,15 @@ static void heartbeat_onchannelcallback(void *context) } }
+static int heartbeat_init(struct hv_util_service *srv) +{
- struct vmbus_channel *channel = srv->channel;
- channel->max_pkt_size = HV_HYP_PAGE_SIZE * 2;
- return 0;
+}
#define HV_UTIL_RING_SEND_SIZE VMBUS_RING_SIZE(3 * HV_HYP_PAGE_SIZE) #define HV_UTIL_RING_RECV_SIZE VMBUS_RING_SIZE(3 * HV_HYP_PAGE_SIZE)
@@ -734,6 +747,8 @@ static struct ptp_clock *hv_ptp_clock;
static int hv_timesync_init(struct hv_util_service *srv) {
struct vmbus_channel *channel = srv->channel;
spin_lock_init(&host_ts.lock);
INIT_WORK(&adj_time_work, hv_set_host_time);
@@ -750,6 +765,8 @@ static int hv_timesync_init(struct hv_util_service *srv) hv_ptp_clock = NULL; }
- channel->max_pkt_size = HV_HYP_PAGE_SIZE * 2;
- return 0;
}
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c index 23999df52739..ae4240777f7d 100644 --- a/drivers/video/fbdev/hyperv_fb.c +++ b/drivers/video/fbdev/hyperv_fb.c @@ -636,6 +636,8 @@ static int synthvid_connect_vsp(struct hv_device *hdev) struct hvfb_par *par = info->par; int ret;
- hdev->channel->max_pkt_size = HV_HYP_PAGE_SIZE + MAX_VMBUS_PKT_SIZE;
- ret = vmbus_open(hdev->channel, RING_BUFSIZE, RING_BUFSIZE, NULL, 0, synthvid_receive, hdev); if (ret) {
-- 2.34.1
On Mon, Jan 10, 2022 at 01:44:19AM +0100, Andrea Parri wrote:
(Extending Cc: list,)
On Sun, Jan 09, 2022 at 05:55:16PM +0800, Yanming Liu wrote:
Commit adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer") introduced a notion of maximum packet size in vmbus channel and used that size to initialize a buffer holding all incoming packet along with their vmbus packet header. Currently, some vmbus drivers set max_pkt_size to the size of their receive buffer passed to vmbus_recvpacket, however vmbus_open expects this size to also include vmbus packet header. This leads to corruption of the ring buffer state when receiving a maximum sized packet.
Specifically, in hv_balloon I have observed of a dm_unballoon_request message of 4096 bytes being truncated to 4080 bytes. When the driver tries to read next packet it starts from a wrong read_index, receives garbage and prints a lot of "Unhandled message: type: <garbage>" in dmesg.
The same mismatch also happens in hv_fcopy, hv_kvp, hv_snapshot, hv_util, hyperv_drm and hyperv_fb, though bad cases are not observed yet.
Allocate the buffer with HV_HYP_PAGE_SIZE more bytes to make room for the descriptor, assuming the vmbus packet header will never be larger than HV_HYP_PAGE_SIZE. This is essentially free compared to just adding 'sizeof(struct vmpacket_descriptor)' because these buffers are all more than HV_HYP_PAGE_SIZE bytes so kmalloc rounds them up anyway.
Fixes: adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer") Suggested-by: Andrea Parri (Microsoft) parri.andrea@gmail.com Signed-off-by: Yanming Liu yanminglr@gmail.com
Thanks for sorting this out; the patch looks good to me:
Reviewed-by: Andrea Parri (Microsoft) parri.andrea@gmail.com
Thanks. I will pick this up after 5.17-rc1 is out.
Wei.
From: Wei Liu wei.liu@kernel.org Sent: Friday, January 14, 2022 11:13 AM
On Mon, Jan 10, 2022 at 01:44:19AM +0100, Andrea Parri wrote:
(Extending Cc: list,)
On Sun, Jan 09, 2022 at 05:55:16PM +0800, Yanming Liu wrote:
Commit adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer") introduced a notion of maximum packet size in vmbus channel and used that size to initialize a buffer holding all incoming packet along with their vmbus packet header. Currently, some vmbus drivers set max_pkt_size to the size of their receive buffer passed to vmbus_recvpacket, however vmbus_open expects this size to also include vmbus packet header. This leads to corruption of the ring buffer state when receiving a maximum sized packet.
Specifically, in hv_balloon I have observed of a dm_unballoon_request message of 4096 bytes being truncated to 4080 bytes. When the driver tries to read next packet it starts from a wrong read_index, receives garbage and prints a lot of "Unhandled message: type: <garbage>" in dmesg.
The same mismatch also happens in hv_fcopy, hv_kvp, hv_snapshot, hv_util, hyperv_drm and hyperv_fb, though bad cases are not observed yet.
Allocate the buffer with HV_HYP_PAGE_SIZE more bytes to make room for the descriptor, assuming the vmbus packet header will never be larger than HV_HYP_PAGE_SIZE. This is essentially free compared to just adding 'sizeof(struct vmpacket_descriptor)' because these buffers are all more than HV_HYP_PAGE_SIZE bytes so kmalloc rounds them up anyway.
Fixes: adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer") Suggested-by: Andrea Parri (Microsoft) parri.andrea@gmail.com Signed-off-by: Yanming Liu yanminglr@gmail.com
Thanks for sorting this out; the patch looks good to me:
Reviewed-by: Andrea Parri (Microsoft) parri.andrea@gmail.com
Thanks. I will pick this up after 5.17-rc1 is out.
Wei.
I'm NACK'ing this set of changes. I've spent some further time investigating, so let me explain.
I'm good with the overall approach of fixing individual drivers to set the max_pkt_size to account for the VMbus packet header, as this is an important aspect that was missed in the original coding. But interestingly, all but one of the miscellaneous VMbus drivers allocate significantly more receive buffer space than is actually needed, and the max_pkt_size matching that receive buffer size is already bigger than needed. In all these cases, there is already plenty of space for the VMbus packet header.
These hv-util.c drivers allocate a receive buffer 4 Kbytes in size, and all receive only small fixed-size packets: heartbeat, shutdown, timesync. I don't think any changes are needed for these drivers because the default max_pkt_size value of 4 Kbytes bytes is plenty of space even when accounting for the VMbus packet header.
The VSS driver in hv_snapshot.c allocates a receive buffer of 8 Kbytes and sets max_pkt_size to 8 Kbytes. But the received messages are all fixed size and small. I don't know why the driver uses an 8 Kbyte receive buffer instead of 4 Kbytes, but the current settings are more than sufficient.
The FCOPY driver in hv_fcopy.c allocates a receive buffer of 8 Kbytes and sets max_pkt_size to 8 Kbytes. The received messages have some header overhead plus up to 6 Kbytes of data, so the 8 Kbyte receive buffer is definitely needed. And while this one is a little closer to filling up the available receive space than the previous ones, there's still plenty of room for the VMbus packet header. I don't think any changes are needed.
The KVP driver in hv_kvp.c allocates a receive buffer of 16 Kbytes and sets max_pkt_size to 16 Kbytes. From what I can tell, the received messages max out at close to 4 Kbytes. Key exchange messages have 512 bytes of key name and 2048 bytes of key value, plus some header overhead. ipaddr_value messages are the largest, with 3 IP addresses @ 1024 bytes each, plus a gateway with 512 bytes, and an adapter ID with 128 bytes. But altogether, that is still less than 4096. I don't know why the receive buffer is 16 Kbytes, but it is plenty big and no changes are needed.
The two frame buffer drivers also use 16 Kbyte receive buffers and set max_pkt_size to 16 Kbytes. Again, this looks to be overkill as the messages received are mostly fixed size. One message returns a variable size list of supported screen resolutions, but each entry in the list is only 4 bytes, and we're looking at a few tens of resolutions at the very most. Again, no changes are needed.
After all this analysis, the balloon driver is the only one that needs changing. It uses a 4 Kbyte receive buffer, and indeed Hyper-V may fill that receive buffer in the case of unballoon messages. And that where the original problem was observed.
Two other aspects for completeness. First, all these drivers do protocol version negotiation with the Hyper-V host. The negotiation includes a variable-size list of supported versions. Each version in the list takes 4 bytes, but there would never be enough different versions to come close to filling a 4 Kbyte buffer. So there's no problem here.
The other lurking issue is the 'offset8' field in the VMbus packet header, which says where the payload starts relative to the VMbus packet header. In practice, this value is always small, so there's no significant additional space to account for. While it's theoretically possible that Hyper-V could use a much larger value, and cause max_pkt_size to be exceeded, there's no real way to fix this problem. Adding an extra page to max_pkt_size, as it done in this patch, certainly provides some extra room, but doesn't guarantee the problem can't happen. But since there's no indication Hyper-V would ever put a big value into offset8, I don't think we need to worry about the possibility.
My bottom-line: Let's fix the balloon driver. But now that we know the other drivers are safe "as is", let's leave them unchanged and not waste the additional memory.
Michael
On Thu, Jan 20, 2022 at 2:12 AM Michael Kelley (LINUX) mikelley@microsoft.com wrote:
From: Wei Liu wei.liu@kernel.org Sent: Friday, January 14, 2022 11:13 AM
On Mon, Jan 10, 2022 at 01:44:19AM +0100, Andrea Parri wrote:
(Extending Cc: list,)
On Sun, Jan 09, 2022 at 05:55:16PM +0800, Yanming Liu wrote:
Commit adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer") introduced a notion of maximum packet size in vmbus channel and used that size to initialize a buffer holding all incoming packet along with their vmbus packet header. Currently, some vmbus drivers set max_pkt_size to the size of their receive buffer passed to vmbus_recvpacket, however vmbus_open expects this size to also include vmbus packet header. This leads to corruption of the ring buffer state when receiving a maximum sized packet.
Specifically, in hv_balloon I have observed of a dm_unballoon_request message of 4096 bytes being truncated to 4080 bytes. When the driver tries to read next packet it starts from a wrong read_index, receives garbage and prints a lot of "Unhandled message: type: <garbage>" in dmesg.
The same mismatch also happens in hv_fcopy, hv_kvp, hv_snapshot, hv_util, hyperv_drm and hyperv_fb, though bad cases are not observed yet.
Allocate the buffer with HV_HYP_PAGE_SIZE more bytes to make room for the descriptor, assuming the vmbus packet header will never be larger than HV_HYP_PAGE_SIZE. This is essentially free compared to just adding 'sizeof(struct vmpacket_descriptor)' because these buffers are all more than HV_HYP_PAGE_SIZE bytes so kmalloc rounds them up anyway.
Fixes: adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer") Suggested-by: Andrea Parri (Microsoft) parri.andrea@gmail.com Signed-off-by: Yanming Liu yanminglr@gmail.com
Thanks for sorting this out; the patch looks good to me:
Reviewed-by: Andrea Parri (Microsoft) parri.andrea@gmail.com
Thanks. I will pick this up after 5.17-rc1 is out.
Wei.
I'm NACK'ing this set of changes. I've spent some further time investigating, so let me explain.
I'm good with the overall approach of fixing individual drivers to set the max_pkt_size to account for the VMbus packet header, as this is an important aspect that was missed in the original coding. But interestingly, all but one of the miscellaneous VMbus drivers allocate significantly more receive buffer space than is actually needed, and the max_pkt_size matching that receive buffer size is already bigger than needed. In all these cases, there is already plenty of space for the VMbus packet header.
Appreciate for the additional insight on what Hyper-V would do!
These hv-util.c drivers allocate a receive buffer 4 Kbytes in size, and all receive only small fixed-size packets: heartbeat, shutdown, timesync. I don't think any changes are needed for these drivers because the default max_pkt_size value of 4 Kbytes bytes is plenty of space even when accounting for the VMbus packet header.
The VSS driver in hv_snapshot.c allocates a receive buffer of 8 Kbytes and sets max_pkt_size to 8 Kbytes. But the received messages are all fixed size and small. I don't know why the driver uses an 8 Kbyte receive buffer instead of 4 Kbytes, but the current settings are more than sufficient.
Well, I'm not sure, on August 2021 there was a patch changing max_pkt_size to 8 KiB for VSS driver: https://lore.kernel.org/linux-hyperv/20210825190217.qh2c6yq5qr3ntum5@liuwe-d...
The patch mentioned a 6304 bytes VSS message. Which is part of the reason I tried to address the more "general" problem of potentially mismatching buffer size.
The FCOPY driver in hv_fcopy.c allocates a receive buffer of 8 Kbytes and sets max_pkt_size to 8 Kbytes. The received messages have some header overhead plus up to 6 Kbytes of data, so the 8 Kbyte receive buffer is definitely needed. And while this one is a little closer to filling up the available receive space than the previous ones, there's still plenty of room for the VMbus packet header. I don't think any changes are needed.
The KVP driver in hv_kvp.c allocates a receive buffer of 16 Kbytes and sets max_pkt_size to 16 Kbytes. From what I can tell, the received messages max out at close to 4 Kbytes. Key exchange messages have 512 bytes of key name and 2048 bytes of key value, plus some header overhead. ipaddr_value messages are the largest, with 3 IP addresses @ 1024 bytes each, plus a gateway with 512 bytes, and an adapter ID with 128 bytes. But altogether, that is still less than 4096. I don't know why the receive buffer is 16 Kbytes, but it is plenty big and no changes are needed.
The two frame buffer drivers also use 16 Kbyte receive buffers and set max_pkt_size to 16 Kbytes. Again, this looks to be overkill as the messages received are mostly fixed size. One message returns a variable size list of supported screen resolutions, but each entry in the list is only 4 bytes, and we're looking at a few tens of resolutions at the very most. Again, no changes are needed.
After all this analysis, the balloon driver is the only one that needs changing. It uses a 4 Kbyte receive buffer, and indeed Hyper-V may fill that receive buffer in the case of unballoon messages. And that where the original problem was observed.
Two other aspects for completeness. First, all these drivers do protocol version negotiation with the Hyper-V host. The negotiation includes a variable-size list of supported versions. Each version in the list takes 4 bytes, but there would never be enough different versions to come close to filling a 4 Kbyte buffer. So there's no problem here.
The other lurking issue is the 'offset8' field in the VMbus packet header, which says where the payload starts relative to the VMbus packet header. In practice, this value is always small, so there's no significant additional space to account for. While it's theoretically possible that Hyper-V could use a much larger value, and cause max_pkt_size to be exceeded, there's no real way to fix this problem. Adding an extra page to max_pkt_size, as it done in this patch, certainly provides some extra room, but doesn't guarantee the problem can't happen. But since there's no indication Hyper-V would ever put a big value into offset8, I don't think we need to worry about the possibility.
Thanks for confirming this!
My bottom-line: Let's fix the balloon driver. But now that we know the other drivers are safe "as is", let's leave them unchanged and not waste the additional memory.
Sure, after all I just want a working kernel :)
Michael
From: Yanming Liu yanminglr@gmail.com Sent: Wednesday, January 19, 2022 12:14 PM
On Thu, Jan 20, 2022 at 2:12 AM Michael Kelley (LINUX) mikelley@microsoft.com wrote:
From: Wei Liu wei.liu@kernel.org Sent: Friday, January 14, 2022 11:13 AM
On Mon, Jan 10, 2022 at 01:44:19AM +0100, Andrea Parri wrote:
(Extending Cc: list,)
On Sun, Jan 09, 2022 at 05:55:16PM +0800, Yanming Liu wrote:
The VSS driver in hv_snapshot.c allocates a receive buffer of 8 Kbytes and sets max_pkt_size to 8 Kbytes. But the received messages are all fixed size and small. I don't know why the driver uses an 8 Kbyte receive buffer instead of 4 Kbytes, but the current settings are more than sufficient.
Well, I'm not sure, on August 2021 there was a patch changing max_pkt_size to 8 KiB for VSS driver: https://lore.kernel.org/linux-hyperv/20210825190217.qh2c6yq5qr3ntum5@liuwe-d...
The patch mentioned a 6304 bytes VSS message. Which is part of the reason I tried to address the more "general" problem of potentially mismatching buffer size.
This is certainly interesting. The Linux driver is not processing all those bytes, so I'm not sure what Hyper-V is passing to the guest. I'll check with the Hyper-V team to be sure.
Michael
dri-devel@lists.freedesktop.org
-
Andrea Parri -
Michael Kelley (LINUX) -
Wei Liu -
Yanming Liu