[PATCH v2] hv: account for packet descriptor in maximum packet size

[PATCH v2] hv: account for packet descriptor in maximum packet size

[Yanming Liu]

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>
---
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

Re: [PATCH v2] hv: account for packet descriptor in maximum packet size

[Andrea Parri]

(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
> 

Re: [PATCH v2] hv: account for packet descriptor in maximum packet size

[Andrea Parri]

(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
> 

Re: [PATCH v2] hv: account for packet descriptor in maximum packet size

[Wei Liu]

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.

Re: [PATCH v2] hv: account for packet descriptor in maximum packet size

[Wei Liu]

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.

RE: [PATCH v2] hv: account for packet descriptor in maximum packet size

[Michael Kelley (LINUX)]

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

RE: [PATCH v2] hv: account for packet descriptor in maximum packet size

[Michael Kelley (LINUX)]

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

Re: [PATCH v2] hv: account for packet descriptor in maximum packet size

[Yanming Liu]

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-devbox-debian-v2/T/

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

Re: [PATCH v2] hv: account for packet descriptor in maximum packet size

[Yanming Liu]

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-devbox-debian-v2/T/

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

RE: [PATCH v2] hv: account for packet descriptor in maximum packet size

[Michael Kelley (LINUX)]

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-devbox-debian-v2/T/
> 
> 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

RE: [PATCH v2] hv: account for packet descriptor in maximum packet size

[Michael Kelley (LINUX)]

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-devbox-debian-v2/T/
> 
> 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