RE: [PATCH v2] hv: account for packet descriptor in maximum packet size
From: Yanming Liu Sent: Wednesday, January 19, 2022 12:14 PM > > On Thu, Jan 20, 2022 at 2:12 AM Michael Kelley (LINUX) > wrote: > > > > From: Wei Liu 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
On Thu, Jan 20, 2022 at 2:12 AM Michael Kelley (LINUX)
wrote:
>
> From: Wei Liu 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: " 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)
> > > > Signed-off-by: Yanming Liu
> > >
> > > Thanks for sorting this out; the patch looks good to me:
> > >
> > > Reviewed-by: Andrea Parri (Microsoft)
> > >
> >
> > 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
RE: [PATCH v2] hv: account for packet descriptor in maximum packet size
From: Wei Liu 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: " 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)
> > > Signed-off-by: Yanming Liu
> >
> > Thanks for sorting this out; the patch looks good to me:
> >
> > Reviewed-by: Andrea Parri (Microsoft)
> >
>
> 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
Re: [PATCH v2] hv: account for packet descriptor in maximum packet size
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: " 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)
> > Signed-off-by: Yanming Liu
>
> Thanks for sorting this out; the patch looks good to me:
>
> Reviewed-by: Andrea Parri (Microsoft)
>
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
(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: " 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)
> Signed-off-by: Yanming Liu
Thanks for sorting this out; the patch looks good to me:
Reviewed-by: Andrea Parri (Microsoft)
In future submissions (if any), please include LKML as well as subsystem
lists 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 = >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
> ---
