[RFC PATCH 0/6] hv_sock: Hardening changes

[RFC PATCH 0/6] hv_sock: Hardening changes

[Andrea Parri (Microsoft)]

Miscellaneous changes to "harden" the hv_sock driver and enable it
in isolated guests.  The diff in ring_buffer.c, hyperv.h is due to
a consequent refactoring/code elimination (patch #6).

Applies to v5.18-rc2.

Thanks,
  Andrea

Andrea Parri (Microsoft) (6):
  hv_sock: Check hv_pkt_iter_first_raw()'s return value
  hv_sock: Copy packets sent by Hyper-V out of the ring buffer
  hv_sock: Add validation for untrusted Hyper-V values
  hv_sock: Initialize send_buf in hvs_stream_enqueue()
  Drivers: hv: vmbus: Accept hv_sock offers in isolated guests
  Drivers: hv: vmbus: Refactor the ring-buffer iterator functions

 drivers/hv/channel_mgmt.c        |  9 ++++--
 drivers/hv/ring_buffer.c         | 11 ++++----
 include/linux/hyperv.h           | 48 ++++++++++----------------------
 net/vmw_vsock/hyperv_transport.c | 24 ++++++++++++----
 4 files changed, 46 insertions(+), 46 deletions(-)

-- 
2.25.1

[RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value

[Andrea Parri (Microsoft)]

The function returns NULL if the ring buffer has no enough space
available for a packet descriptor.  The ring buffer's write_index
is in memory which is shared with the Hyper-V host, its value is
thus subject to being changed at any time.

Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 net/vmw_vsock/hyperv_transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index e111e13b66604..943352530936e 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -603,6 +603,8 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, struct msghdr *msg,
 
 	if (need_refill) {
 		hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan);
+		if (!hvs->recv_desc)
+			return -ENOBUFS;
 		ret = hvs_update_recv_data(hvs);
 		if (ret)
 			return ret;
-- 
2.25.1

RE: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value

[Michael Kelley (LINUX) via Virtualization]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> 
> The function returns NULL if the ring buffer has no enough space
> available for a packet descriptor.  The ring buffer's write_index

The first sentence wording is a bit scrambled.  I think you mean the
ring buffer doesn't contain enough readable bytes to constitute a
packet descriptor.

> is in memory which is shared with the Hyper-V host, its value is
> thus subject to being changed at any time.

This second sentence is true, but I'm not making the connection
with the code change below.   Evidently, there is some previous
check made to ensure that enough bytes are available to be
received when hvs_stream_dequeue() is called, so we assumed that
NULL could never be returned?  I looked but didn't find such a check, 
so maybe I didn't look carefully enough.  But now we are assuming
that Hyper-V might have invalidated that previous check by 
subsequently changing the write_index in a bogus way?  So now, NULL
could be returned when previously we assumed it couldn't.

Michael

> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  net/vmw_vsock/hyperv_transport.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index e111e13b66604..943352530936e 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -603,6 +603,8 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk,
> struct msghdr *msg,
> 
>  	if (need_refill) {
>  		hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan);
> +		if (!hvs->recv_desc)
> +			return -ENOBUFS;
>  		ret = hvs_update_recv_data(hvs);
>  		if (ret)
>  			return ret;
> --
> 2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value

[Michael Kelley (LINUX)]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> 
> The function returns NULL if the ring buffer has no enough space
> available for a packet descriptor.  The ring buffer's write_index

The first sentence wording is a bit scrambled.  I think you mean the
ring buffer doesn't contain enough readable bytes to constitute a
packet descriptor.

> is in memory which is shared with the Hyper-V host, its value is
> thus subject to being changed at any time.

This second sentence is true, but I'm not making the connection
with the code change below.   Evidently, there is some previous
check made to ensure that enough bytes are available to be
received when hvs_stream_dequeue() is called, so we assumed that
NULL could never be returned?  I looked but didn't find such a check, 
so maybe I didn't look carefully enough.  But now we are assuming
that Hyper-V might have invalidated that previous check by 
subsequently changing the write_index in a bogus way?  So now, NULL
could be returned when previously we assumed it couldn't.

Michael

> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  net/vmw_vsock/hyperv_transport.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index e111e13b66604..943352530936e 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -603,6 +603,8 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk,
> struct msghdr *msg,
> 
>  	if (need_refill) {
>  		hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan);
> +		if (!hvs->recv_desc)
> +			return -ENOBUFS;
>  		ret = hvs_update_recv_data(hvs);
>  		if (ret)
>  			return ret;
> --
> 2.25.1

Re: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value

[Andrea Parri]

On Fri, Apr 15, 2022 at 03:33:23AM +0000, Michael Kelley (LINUX) wrote:
> From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> > 
> > The function returns NULL if the ring buffer has no enough space
> > available for a packet descriptor.  The ring buffer's write_index
> 
> The first sentence wording is a bit scrambled.  I think you mean the
> ring buffer doesn't contain enough readable bytes to constitute a
> packet descriptor.

Indeed, replaced with your working.


> > is in memory which is shared with the Hyper-V host, its value is
> > thus subject to being changed at any time.
> 
> This second sentence is true, but I'm not making the connection
> with the code change below.   Evidently, there is some previous
> check made to ensure that enough bytes are available to be
> received when hvs_stream_dequeue() is called, so we assumed that
> NULL could never be returned?  I looked but didn't find such a check, 
> so maybe I didn't look carefully enough.  But now we are assuming
> that Hyper-V might have invalidated that previous check by 
> subsequently changing the write_index in a bogus way?  So now, NULL
> could be returned when previously we assumed it couldn't.

I think you're looking for hvs_stream_has_data().  (Previous checks
apart, hvs_stream_dequeue() will "dereference" the pointer so...)

Thanks,
  Andrea

RE: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value

[Michael Kelley (LINUX) via Virtualization]

From: Andrea Parri <parri.andrea@gmail.com> Sent: Thursday, April 14, 2022 11:42 PM
> 
> On Fri, Apr 15, 2022 at 03:33:23AM +0000, Michael Kelley (LINUX) wrote:
> > From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13,
> 2022 1:48 PM
> > >
> > > The function returns NULL if the ring buffer has no enough space
> > > available for a packet descriptor.  The ring buffer's write_index
> >
> > The first sentence wording is a bit scrambled.  I think you mean the
> > ring buffer doesn't contain enough readable bytes to constitute a
> > packet descriptor.
> 
> Indeed, replaced with your working.
> 
> 
> > > is in memory which is shared with the Hyper-V host, its value is
> > > thus subject to being changed at any time.
> >
> > This second sentence is true, but I'm not making the connection
> > with the code change below.   Evidently, there is some previous
> > check made to ensure that enough bytes are available to be
> > received when hvs_stream_dequeue() is called, so we assumed that
> > NULL could never be returned?  I looked but didn't find such a check,
> > so maybe I didn't look carefully enough.  But now we are assuming
> > that Hyper-V might have invalidated that previous check by
> > subsequently changing the write_index in a bogus way?  So now, NULL
> > could be returned when previously we assumed it couldn't.
> 
> I think you're looking for hvs_stream_has_data().  (Previous checks
> apart, hvs_stream_dequeue() will "dereference" the pointer so...)

Agreed.  I didn't say this explicitly, but I was wondering about the risk
in the current code (without these hardening patches) of getting a
NULL pointer from hv_pkt_iter_first_raw(), and then dereferencing it.

Michael



_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value

[Michael Kelley (LINUX)]

From: Andrea Parri <parri.andrea@gmail.com> Sent: Thursday, April 14, 2022 11:42 PM
> 
> On Fri, Apr 15, 2022 at 03:33:23AM +0000, Michael Kelley (LINUX) wrote:
> > From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13,
> 2022 1:48 PM
> > >
> > > The function returns NULL if the ring buffer has no enough space
> > > available for a packet descriptor.  The ring buffer's write_index
> >
> > The first sentence wording is a bit scrambled.  I think you mean the
> > ring buffer doesn't contain enough readable bytes to constitute a
> > packet descriptor.
> 
> Indeed, replaced with your working.
> 
> 
> > > is in memory which is shared with the Hyper-V host, its value is
> > > thus subject to being changed at any time.
> >
> > This second sentence is true, but I'm not making the connection
> > with the code change below.   Evidently, there is some previous
> > check made to ensure that enough bytes are available to be
> > received when hvs_stream_dequeue() is called, so we assumed that
> > NULL could never be returned?  I looked but didn't find such a check,
> > so maybe I didn't look carefully enough.  But now we are assuming
> > that Hyper-V might have invalidated that previous check by
> > subsequently changing the write_index in a bogus way?  So now, NULL
> > could be returned when previously we assumed it couldn't.
> 
> I think you're looking for hvs_stream_has_data().  (Previous checks
> apart, hvs_stream_dequeue() will "dereference" the pointer so...)

Agreed.  I didn't say this explicitly, but I was wondering about the risk
in the current code (without these hardening patches) of getting a
NULL pointer from hv_pkt_iter_first_raw(), and then dereferencing it.

Michael

Re: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value

[Andrea Parri]

On Fri, Apr 15, 2022 at 02:27:37PM +0000, Michael Kelley (LINUX) wrote:
> From: Andrea Parri <parri.andrea@gmail.com> Sent: Thursday, April 14, 2022 11:42 PM
> > 
> > On Fri, Apr 15, 2022 at 03:33:23AM +0000, Michael Kelley (LINUX) wrote:
> > > From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13,
> > 2022 1:48 PM
> > > >
> > > > The function returns NULL if the ring buffer has no enough space
> > > > available for a packet descriptor.  The ring buffer's write_index
> > >
> > > The first sentence wording is a bit scrambled.  I think you mean the
> > > ring buffer doesn't contain enough readable bytes to constitute a
> > > packet descriptor.
> > 
> > Indeed, replaced with your working.
> > 
> > 
> > > > is in memory which is shared with the Hyper-V host, its value is
> > > > thus subject to being changed at any time.
> > >
> > > This second sentence is true, but I'm not making the connection
> > > with the code change below.   Evidently, there is some previous
> > > check made to ensure that enough bytes are available to be
> > > received when hvs_stream_dequeue() is called, so we assumed that
> > > NULL could never be returned?  I looked but didn't find such a check,
> > > so maybe I didn't look carefully enough.  But now we are assuming
> > > that Hyper-V might have invalidated that previous check by
> > > subsequently changing the write_index in a bogus way?  So now, NULL
> > > could be returned when previously we assumed it couldn't.
> > 
> > I think you're looking for hvs_stream_has_data().  (Previous checks
> > apart, hvs_stream_dequeue() will "dereference" the pointer so...)
> 
> Agreed.  I didn't say this explicitly, but I was wondering about the risk
> in the current code (without these hardening patches) of getting a
> NULL pointer from hv_pkt_iter_first_raw(), and then dereferencing it.

Got it.  Updated the changelog to:

  "The ring buffer's write_index is in memory which is shared with the
   Hyper-V host, an erroneous or malicious host could thus change its
   value and overturn the result of hvs_stream_has_data()."

Hopefully this can clarify the issue (without introducing other typos).

Thanks,
  Andrea

[RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer

[Andrea Parri (Microsoft)]

Pointers to VMbus packets sent by Hyper-V are used by the hv_sock driver
within the gues VM.  Hyper-V can send packets with erroneous values or
modify packet fields after they are processed by the guest.  To defend
against these scenarios, copy the incoming packet after validating its
length and offset fields using hv_pkt_iter_{first,next}().  In this way,
the packet can no longer be modified by the host.

Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 net/vmw_vsock/hyperv_transport.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index 943352530936e..8c37d07017fc4 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -78,6 +78,9 @@ struct hvs_send_buf {
 					 ALIGN((payload_len), 8) + \
 					 VMBUS_PKT_TRAILER_SIZE)
 
+/* Upper bound on the size of a VMbus packet for hv_sock */
+#define HVS_MAX_PKT_SIZE	HVS_PKT_LEN(HVS_MTU_SIZE)
+
 union hvs_service_id {
 	guid_t	srv_id;
 
@@ -378,6 +381,8 @@ static void hvs_open_connection(struct vmbus_channel *chan)
 		rcvbuf = ALIGN(rcvbuf, HV_HYP_PAGE_SIZE);
 	}
 
+	chan->max_pkt_size = HVS_MAX_PKT_SIZE;
+
 	ret = vmbus_open(chan, sndbuf, rcvbuf, NULL, 0, hvs_channel_cb,
 			 conn_from_host ? new : sk);
 	if (ret != 0) {
@@ -602,7 +607,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, struct msghdr *msg,
 		return -EOPNOTSUPP;
 
 	if (need_refill) {
-		hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan);
+		hvs->recv_desc = hv_pkt_iter_first(hvs->chan);
 		if (!hvs->recv_desc)
 			return -ENOBUFS;
 		ret = hvs_update_recv_data(hvs);
@@ -618,7 +623,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, struct msghdr *msg,
 
 	hvs->recv_data_len -= to_read;
 	if (hvs->recv_data_len == 0) {
-		hvs->recv_desc = hv_pkt_iter_next_raw(hvs->chan, hvs->recv_desc);
+		hvs->recv_desc = hv_pkt_iter_next(hvs->chan, hvs->recv_desc);
 		if (hvs->recv_desc) {
 			ret = hvs_update_recv_data(hvs);
 			if (ret)
-- 
2.25.1

RE: [RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer

[Michael Kelley (LINUX) via Virtualization]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> 
> Pointers to VMbus packets sent by Hyper-V are used by the hv_sock driver
> within the gues VM.  Hyper-V can send packets with erroneous values or

s/gues/guest/

> modify packet fields after they are processed by the guest.  To defend
> against these scenarios, copy the incoming packet after validating its
> length and offset fields using hv_pkt_iter_{first,next}().  In this way,
> the packet can no longer be modified by the host.
> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  net/vmw_vsock/hyperv_transport.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index 943352530936e..8c37d07017fc4 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -78,6 +78,9 @@ struct hvs_send_buf {
>  					 ALIGN((payload_len), 8) + \
>  					 VMBUS_PKT_TRAILER_SIZE)
> 
> +/* Upper bound on the size of a VMbus packet for hv_sock */
> +#define HVS_MAX_PKT_SIZE	HVS_PKT_LEN(HVS_MTU_SIZE)
> +
>  union hvs_service_id {
>  	guid_t	srv_id;
> 
> @@ -378,6 +381,8 @@ static void hvs_open_connection(struct vmbus_channel *chan)
>  		rcvbuf = ALIGN(rcvbuf, HV_HYP_PAGE_SIZE);
>  	}
> 
> +	chan->max_pkt_size = HVS_MAX_PKT_SIZE;
> +
>  	ret = vmbus_open(chan, sndbuf, rcvbuf, NULL, 0, hvs_channel_cb,
>  			 conn_from_host ? new : sk);
>  	if (ret != 0) {
> @@ -602,7 +607,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk,
> struct msghdr *msg,
>  		return -EOPNOTSUPP;
> 
>  	if (need_refill) {
> -		hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan);
> +		hvs->recv_desc = hv_pkt_iter_first(hvs->chan);
>  		if (!hvs->recv_desc)
>  			return -ENOBUFS;
>  		ret = hvs_update_recv_data(hvs);
> @@ -618,7 +623,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk,
> struct msghdr *msg,
> 
>  	hvs->recv_data_len -= to_read;
>  	if (hvs->recv_data_len == 0) {
> -		hvs->recv_desc = hv_pkt_iter_next_raw(hvs->chan, hvs->recv_desc);
> +		hvs->recv_desc = hv_pkt_iter_next(hvs->chan, hvs->recv_desc);
>  		if (hvs->recv_desc) {
>  			ret = hvs_update_recv_data(hvs);
>  			if (ret)
> --
> 2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer

[Michael Kelley (LINUX)]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> 
> Pointers to VMbus packets sent by Hyper-V are used by the hv_sock driver
> within the gues VM.  Hyper-V can send packets with erroneous values or

s/gues/guest/

> modify packet fields after they are processed by the guest.  To defend
> against these scenarios, copy the incoming packet after validating its
> length and offset fields using hv_pkt_iter_{first,next}().  In this way,
> the packet can no longer be modified by the host.
> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  net/vmw_vsock/hyperv_transport.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index 943352530936e..8c37d07017fc4 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -78,6 +78,9 @@ struct hvs_send_buf {
>  					 ALIGN((payload_len), 8) + \
>  					 VMBUS_PKT_TRAILER_SIZE)
> 
> +/* Upper bound on the size of a VMbus packet for hv_sock */
> +#define HVS_MAX_PKT_SIZE	HVS_PKT_LEN(HVS_MTU_SIZE)
> +
>  union hvs_service_id {
>  	guid_t	srv_id;
> 
> @@ -378,6 +381,8 @@ static void hvs_open_connection(struct vmbus_channel *chan)
>  		rcvbuf = ALIGN(rcvbuf, HV_HYP_PAGE_SIZE);
>  	}
> 
> +	chan->max_pkt_size = HVS_MAX_PKT_SIZE;
> +
>  	ret = vmbus_open(chan, sndbuf, rcvbuf, NULL, 0, hvs_channel_cb,
>  			 conn_from_host ? new : sk);
>  	if (ret != 0) {
> @@ -602,7 +607,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk,
> struct msghdr *msg,
>  		return -EOPNOTSUPP;
> 
>  	if (need_refill) {
> -		hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan);
> +		hvs->recv_desc = hv_pkt_iter_first(hvs->chan);
>  		if (!hvs->recv_desc)
>  			return -ENOBUFS;
>  		ret = hvs_update_recv_data(hvs);
> @@ -618,7 +623,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk,
> struct msghdr *msg,
> 
>  	hvs->recv_data_len -= to_read;
>  	if (hvs->recv_data_len == 0) {
> -		hvs->recv_desc = hv_pkt_iter_next_raw(hvs->chan, hvs->recv_desc);
> +		hvs->recv_desc = hv_pkt_iter_next(hvs->chan, hvs->recv_desc);
>  		if (hvs->recv_desc) {
>  			ret = hvs_update_recv_data(hvs);
>  			if (ret)
> --
> 2.25.1

Re: [RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer

[Andrea Parri]

On Fri, Apr 15, 2022 at 03:33:31AM +0000, Michael Kelley (LINUX) wrote:
> From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> > 
> > Pointers to VMbus packets sent by Hyper-V are used by the hv_sock driver
> > within the gues VM.  Hyper-V can send packets with erroneous values or
> 
> s/gues/guest/

Fixed.

Thanks,
  Andrea

[RFC PATCH 3/6] hv_sock: Add validation for untrusted Hyper-V values

[Andrea Parri (Microsoft)]

For additional robustness in the face of Hyper-V errors or malicious
behavior, validate all values that originate from packets that Hyper-V
has sent to the guest in the host-to-guest ring buffer.  Ensure that
invalid values cannot cause data being copied out of the bounds of the
source buffer in hvs_stream_dequeue().

Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 include/linux/hyperv.h           |  5 +++++
 net/vmw_vsock/hyperv_transport.c | 11 +++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index fe2e0179ed51e..55478a6810b60 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1663,6 +1663,11 @@ static inline u32 hv_pkt_datalen(const struct vmpacket_descriptor *desc)
 	return (desc->len8 << 3) - (desc->offset8 << 3);
 }
 
+/* Get packet length associated with descriptor */
+static inline u32 hv_pkt_len(const struct vmpacket_descriptor *desc)
+{
+	return desc->len8 << 3;
+}
 
 struct vmpacket_descriptor *
 hv_pkt_iter_first_raw(struct vmbus_channel *channel);
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index 8c37d07017fc4..092cadc2c866d 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -577,12 +577,19 @@ static bool hvs_dgram_allow(u32 cid, u32 port)
 static int hvs_update_recv_data(struct hvsock *hvs)
 {
 	struct hvs_recv_buf *recv_buf;
-	u32 payload_len;
+	u32 pkt_len, payload_len;
+
+	pkt_len = hv_pkt_len(hvs->recv_desc);
+
+	/* Ensure the packet is big enough to read its header */
+	if (pkt_len < HVS_HEADER_LEN)
+		return -EIO;
 
 	recv_buf = (struct hvs_recv_buf *)(hvs->recv_desc + 1);
 	payload_len = recv_buf->hdr.data_size;
 
-	if (payload_len > HVS_MTU_SIZE)
+	/* Ensure the packet is big enough to read its payload */
+	if (payload_len > pkt_len - HVS_HEADER_LEN || payload_len > HVS_MTU_SIZE)
 		return -EIO;
 
 	if (payload_len == 0)
-- 
2.25.1

[RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()

[Andrea Parri (Microsoft)]

So that padding or uninitialized bytes can't leak guest memory contents.

Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 net/vmw_vsock/hyperv_transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index 092cadc2c866d..72ce00928c8e7 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -655,7 +655,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
 
 	BUILD_BUG_ON(sizeof(*send_buf) != HV_HYP_PAGE_SIZE);
 
-	send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
+	send_buf = kzalloc(sizeof(*send_buf), GFP_KERNEL);
 	if (!send_buf)
 		return -ENOMEM;
 
-- 
2.25.1

RE: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()

[Michael Kelley (LINUX) via Virtualization]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> 
> So that padding or uninitialized bytes can't leak guest memory contents.
> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  net/vmw_vsock/hyperv_transport.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index 092cadc2c866d..72ce00928c8e7 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -655,7 +655,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk,
> struct msghdr *msg,
> 
>  	BUILD_BUG_ON(sizeof(*send_buf) != HV_HYP_PAGE_SIZE);
> 
> -	send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
> +	send_buf = kzalloc(sizeof(*send_buf), GFP_KERNEL);

Is this change really needed?   All fields are explicitly initialized, and in the data
array, only the populated bytes are copied to the ring buffer.  There should not
be any uninitialized values sent to the host.   Zeroing the memory ahead of
time certainly provides an extra protection (particularly against padding bytes,
but there can't be any since the layout of the data is part of the protocol with
Hyper-V).  It is expensive protection to zero out 16K+ bytes every time we send
out a small message.

Michael

>  	if (!send_buf)
>  		return -ENOMEM;
> 
> --
> 2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()

[Michael Kelley (LINUX)]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> 
> So that padding or uninitialized bytes can't leak guest memory contents.
> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  net/vmw_vsock/hyperv_transport.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index 092cadc2c866d..72ce00928c8e7 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -655,7 +655,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk,
> struct msghdr *msg,
> 
>  	BUILD_BUG_ON(sizeof(*send_buf) != HV_HYP_PAGE_SIZE);
> 
> -	send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
> +	send_buf = kzalloc(sizeof(*send_buf), GFP_KERNEL);

Is this change really needed?   All fields are explicitly initialized, and in the data
array, only the populated bytes are copied to the ring buffer.  There should not
be any uninitialized values sent to the host.   Zeroing the memory ahead of
time certainly provides an extra protection (particularly against padding bytes,
but there can't be any since the layout of the data is part of the protocol with
Hyper-V).  It is expensive protection to zero out 16K+ bytes every time we send
out a small message.

Michael

>  	if (!send_buf)
>  		return -ENOMEM;
> 
> --
> 2.25.1

Re: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()

[Andrea Parri]

> > @@ -655,7 +655,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk,
> > struct msghdr *msg,
> > 
> >  	BUILD_BUG_ON(sizeof(*send_buf) != HV_HYP_PAGE_SIZE);
> > 
> > -	send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
> > +	send_buf = kzalloc(sizeof(*send_buf), GFP_KERNEL);
> 
> Is this change really needed?

The idea was...


> All fields are explicitly initialized, and in the data
> array, only the populated bytes are copied to the ring buffer.  There should not
> be any uninitialized values sent to the host.   Zeroing the memory ahead of
> time certainly provides an extra protection (particularly against padding bytes,
> but there can't be any since the layout of the data is part of the protocol with
> Hyper-V).

Rather than keeping checking that...


> It is expensive protection to zero out 16K+ bytes every time we send
> out a small message.

Do this.  ;-)

Will drop the patch.

Thanks,
  Andrea

RE: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()

[Michael Kelley (LINUX) via Virtualization]

From: Andrea Parri <parri.andrea@gmail.com> Sent: Thursday, April 14, 2022 11:51 PM
> 
> > > @@ -655,7 +655,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk,
> > > struct msghdr *msg,
> > >
> > >  	BUILD_BUG_ON(sizeof(*send_buf) != HV_HYP_PAGE_SIZE);
> > >
> > > -	send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
> > > +	send_buf = kzalloc(sizeof(*send_buf), GFP_KERNEL);
> >
> > Is this change really needed?
> 
> The idea was...
> 
> 
> > All fields are explicitly initialized, and in the data
> > array, only the populated bytes are copied to the ring buffer.  There should not
> > be any uninitialized values sent to the host.   Zeroing the memory ahead of
> > time certainly provides an extra protection (particularly against padding bytes,
> > but there can't be any since the layout of the data is part of the protocol with
> > Hyper-V).
> 
> Rather than keeping checking that...

The extra protection might be obtained by just zero'ing the header (i.e., the
bytes up to the 16 Kbyte data array).   I don't have a strong preference either
way, so up to you.

Michael

> 
> 
> > It is expensive protection to zero out 16K+ bytes every time we send
> > out a small message.
> 
> Do this.  ;-)
> 
> Will drop the patch.
> 
> Thanks,
>   Andrea
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()

[Michael Kelley (LINUX)]

From: Andrea Parri <parri.andrea@gmail.com> Sent: Thursday, April 14, 2022 11:51 PM
> 
> > > @@ -655,7 +655,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk,
> > > struct msghdr *msg,
> > >
> > >  	BUILD_BUG_ON(sizeof(*send_buf) != HV_HYP_PAGE_SIZE);
> > >
> > > -	send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
> > > +	send_buf = kzalloc(sizeof(*send_buf), GFP_KERNEL);
> >
> > Is this change really needed?
> 
> The idea was...
> 
> 
> > All fields are explicitly initialized, and in the data
> > array, only the populated bytes are copied to the ring buffer.  There should not
> > be any uninitialized values sent to the host.   Zeroing the memory ahead of
> > time certainly provides an extra protection (particularly against padding bytes,
> > but there can't be any since the layout of the data is part of the protocol with
> > Hyper-V).
> 
> Rather than keeping checking that...

The extra protection might be obtained by just zero'ing the header (i.e., the
bytes up to the 16 Kbyte data array).   I don't have a strong preference either
way, so up to you.

Michael

> 
> 
> > It is expensive protection to zero out 16K+ bytes every time we send
> > out a small message.
> 
> Do this.  ;-)
> 
> Will drop the patch.
> 
> Thanks,
>   Andrea

Re: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()

[Andrea Parri]

> > > All fields are explicitly initialized, and in the data
> > > array, only the populated bytes are copied to the ring buffer.  There should not
> > > be any uninitialized values sent to the host.   Zeroing the memory ahead of
> > > time certainly provides an extra protection (particularly against padding bytes,
> > > but there can't be any since the layout of the data is part of the protocol with
> > > Hyper-V).
> > 
> > Rather than keeping checking that...
> 
> The extra protection might be obtained by just zero'ing the header (i.e., the
> bytes up to the 16 Kbyte data array).   I don't have a strong preference either
> way, so up to you.

A main reason behind this RFC is that I don't have either.  IIUC, you're
suggesting something like (the compiled only):


diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index 092cadc2c866d..200f12c432863 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -234,7 +234,8 @@ static int __hvs_send_data(struct vmbus_channel *chan,
 {
 	hdr->pkt_type = 1;
 	hdr->data_size = to_write;
-	return vmbus_sendpacket(chan, hdr, sizeof(*hdr) + to_write,
+	return vmbus_sendpacket(chan, hdr,
+				offsetof(struct hvs_send_buf, data) + to_write,
 				0, VM_PKT_DATA_INBAND, 0);
 }
 
@@ -658,6 +659,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
 	send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
 	if (!send_buf)
 		return -ENOMEM;
+	memset(send_buf, 0, offsetof(struct hvs_send_buf, data));
 
 	/* Reader(s) could be draining data from the channel as we write.
 	 * Maximize bandwidth, by iterating until the channel is found to be
-- 

Let me queue this for further testing/review...

Thanks,
  Andrea

[RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests

[Andrea Parri (Microsoft)]

So that isolated guests can communicate with the host via hv_sock
channels.

Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 drivers/hv/channel_mgmt.c | 9 +++++++--
 include/linux/hyperv.h    | 8 ++++++--
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 67be81208a2d9..83d7ab90b7305 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -976,17 +976,22 @@ find_primary_channel_by_offer(const struct vmbus_channel_offer_channel *offer)
 	return channel;
 }
 
-static bool vmbus_is_valid_device(const guid_t *guid)
+static bool vmbus_is_valid_offer(const struct vmbus_channel_offer_channel *offer)
 {
+	const guid_t *guid = &offer->offer.if_type;
 	u16 i;
 
 	if (!hv_is_isolation_supported())
 		return true;
 
+	if (is_hvsock_offer(offer))
+		return true;
+
 	for (i = 0; i < ARRAY_SIZE(vmbus_devs); i++) {
 		if (guid_equal(guid, &vmbus_devs[i].guid))
 			return vmbus_devs[i].allowed_in_isolated;
 	}
+
 	return false;
 }
 
@@ -1004,7 +1009,7 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
 
 	trace_vmbus_onoffer(offer);
 
-	if (!vmbus_is_valid_device(&offer->offer.if_type)) {
+	if (!vmbus_is_valid_offer(offer)) {
 		pr_err_ratelimited("Invalid offer %d from the host supporting isolation\n",
 				   offer->child_relid);
 		atomic_dec(&vmbus_connection.offer_in_progress);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 55478a6810b60..1112c5cf894e6 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1044,10 +1044,14 @@ struct vmbus_channel {
 u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr);
 u64 vmbus_request_addr(struct vmbus_channel *channel, u64 trans_id);
 
+static inline bool is_hvsock_offer(const struct vmbus_channel_offer_channel *o)
+{
+	return !!(o->offer.chn_flags & VMBUS_CHANNEL_TLNPI_PROVIDER_OFFER);
+}
+
 static inline bool is_hvsock_channel(const struct vmbus_channel *c)
 {
-	return !!(c->offermsg.offer.chn_flags &
-		  VMBUS_CHANNEL_TLNPI_PROVIDER_OFFER);
+	return is_hvsock_offer(&c->offermsg);
 }
 
 static inline bool is_sub_channel(const struct vmbus_channel *c)
-- 
2.25.1

RE: [RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests

[Michael Kelley (LINUX) via Virtualization]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> 
> So that isolated guests can communicate with the host via hv_sock
> channels.
> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  drivers/hv/channel_mgmt.c | 9 +++++++--
>  include/linux/hyperv.h    | 8 ++++++--
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 67be81208a2d9..83d7ab90b7305 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -976,17 +976,22 @@ find_primary_channel_by_offer(const struct
> vmbus_channel_offer_channel *offer)
>  	return channel;
>  }
> 
> -static bool vmbus_is_valid_device(const guid_t *guid)
> +static bool vmbus_is_valid_offer(const struct vmbus_channel_offer_channel *offer)
>  {
> +	const guid_t *guid = &offer->offer.if_type;
>  	u16 i;
> 
>  	if (!hv_is_isolation_supported())
>  		return true;
> 
> +	if (is_hvsock_offer(offer))
> +		return true;
> +
>  	for (i = 0; i < ARRAY_SIZE(vmbus_devs); i++) {
>  		if (guid_equal(guid, &vmbus_devs[i].guid))
>  			return vmbus_devs[i].allowed_in_isolated;
>  	}
> +

Spurious newline added?

>  	return false;
>  }
> 
> @@ -1004,7 +1009,7 @@ static void vmbus_onoffer(struct
> vmbus_channel_message_header *hdr)
> 
>  	trace_vmbus_onoffer(offer);
> 
> -	if (!vmbus_is_valid_device(&offer->offer.if_type)) {
> +	if (!vmbus_is_valid_offer(offer)) {
>  		pr_err_ratelimited("Invalid offer %d from the host supporting isolation\n",
>  				   offer->child_relid);
>  		atomic_dec(&vmbus_connection.offer_in_progress);
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 55478a6810b60..1112c5cf894e6 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1044,10 +1044,14 @@ struct vmbus_channel {
>  u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr);
>  u64 vmbus_request_addr(struct vmbus_channel *channel, u64 trans_id);
> 
> +static inline bool is_hvsock_offer(const struct vmbus_channel_offer_channel *o)
> +{
> +	return !!(o->offer.chn_flags & VMBUS_CHANNEL_TLNPI_PROVIDER_OFFER);
> +}
> +
>  static inline bool is_hvsock_channel(const struct vmbus_channel *c)
>  {
> -	return !!(c->offermsg.offer.chn_flags &
> -		  VMBUS_CHANNEL_TLNPI_PROVIDER_OFFER);
> +	return is_hvsock_offer(&c->offermsg);
>  }
> 
>  static inline bool is_sub_channel(const struct vmbus_channel *c)
> --
> 2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests

[Michael Kelley (LINUX)]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> 
> So that isolated guests can communicate with the host via hv_sock
> channels.
> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  drivers/hv/channel_mgmt.c | 9 +++++++--
>  include/linux/hyperv.h    | 8 ++++++--
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 67be81208a2d9..83d7ab90b7305 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -976,17 +976,22 @@ find_primary_channel_by_offer(const struct
> vmbus_channel_offer_channel *offer)
>  	return channel;
>  }
> 
> -static bool vmbus_is_valid_device(const guid_t *guid)
> +static bool vmbus_is_valid_offer(const struct vmbus_channel_offer_channel *offer)
>  {
> +	const guid_t *guid = &offer->offer.if_type;
>  	u16 i;
> 
>  	if (!hv_is_isolation_supported())
>  		return true;
> 
> +	if (is_hvsock_offer(offer))
> +		return true;
> +
>  	for (i = 0; i < ARRAY_SIZE(vmbus_devs); i++) {
>  		if (guid_equal(guid, &vmbus_devs[i].guid))
>  			return vmbus_devs[i].allowed_in_isolated;
>  	}
> +

Spurious newline added?

>  	return false;
>  }
> 
> @@ -1004,7 +1009,7 @@ static void vmbus_onoffer(struct
> vmbus_channel_message_header *hdr)
> 
>  	trace_vmbus_onoffer(offer);
> 
> -	if (!vmbus_is_valid_device(&offer->offer.if_type)) {
> +	if (!vmbus_is_valid_offer(offer)) {
>  		pr_err_ratelimited("Invalid offer %d from the host supporting isolation\n",
>  				   offer->child_relid);
>  		atomic_dec(&vmbus_connection.offer_in_progress);
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 55478a6810b60..1112c5cf894e6 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1044,10 +1044,14 @@ struct vmbus_channel {
>  u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr);
>  u64 vmbus_request_addr(struct vmbus_channel *channel, u64 trans_id);
> 
> +static inline bool is_hvsock_offer(const struct vmbus_channel_offer_channel *o)
> +{
> +	return !!(o->offer.chn_flags & VMBUS_CHANNEL_TLNPI_PROVIDER_OFFER);
> +}
> +
>  static inline bool is_hvsock_channel(const struct vmbus_channel *c)
>  {
> -	return !!(c->offermsg.offer.chn_flags &
> -		  VMBUS_CHANNEL_TLNPI_PROVIDER_OFFER);
> +	return is_hvsock_offer(&c->offermsg);
>  }
> 
>  static inline bool is_sub_channel(const struct vmbus_channel *c)
> --
> 2.25.1

Re: [RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests

[Andrea Parri]

> > @@ -976,17 +976,22 @@ find_primary_channel_by_offer(const struct
> > vmbus_channel_offer_channel *offer)
> >  	return channel;
> >  }
> > 
> > -static bool vmbus_is_valid_device(const guid_t *guid)
> > +static bool vmbus_is_valid_offer(const struct vmbus_channel_offer_channel *offer)
> >  {
> > +	const guid_t *guid = &offer->offer.if_type;
> >  	u16 i;
> > 
> >  	if (!hv_is_isolation_supported())
> >  		return true;
> > 
> > +	if (is_hvsock_offer(offer))
> > +		return true;
> > +
> >  	for (i = 0; i < ARRAY_SIZE(vmbus_devs); i++) {
> >  		if (guid_equal(guid, &vmbus_devs[i].guid))
> >  			return vmbus_devs[i].allowed_in_isolated;
> >  	}
> > +
> 
> Spurious newline added?
> 
> >  	return false;

Intentionally added to visually separate the "hvsock", "vmbus_dev" and
"default" blocks, patch seemed simple enough to try to merge in "style
material" without incurring in the question.  ;-)

Newline removed.

Thanks,
  Andrea

[RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions

[Andrea Parri (Microsoft)]

With no users of hv_pkt_iter_next_raw() and no "external" users of
hv_pkt_iter_first_raw(), the iterator functions can be refactored
and simplified to remove some indirection/code.

Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 drivers/hv/ring_buffer.c | 11 +++++------
 include/linux/hyperv.h   | 35 ++++-------------------------------
 2 files changed, 9 insertions(+), 37 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 3d215d9dec433..c9357dae2a2c8 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -421,7 +421,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
 	memcpy(buffer, (const char *)desc + offset, packetlen);
 
 	/* Advance ring index to next packet descriptor */
-	__hv_pkt_iter_next(channel, desc, true);
+	__hv_pkt_iter_next(channel, desc);
 
 	/* Notify host of update */
 	hv_pkt_iter_close(channel);
@@ -459,7 +459,8 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info *rbi)
 /*
  * Get first vmbus packet without copying it out of the ring buffer
  */
-struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel)
+static struct vmpacket_descriptor *
+hv_pkt_iter_first_raw(struct vmbus_channel *channel)
 {
 	struct hv_ring_buffer_info *rbi = &channel->inbound;
 
@@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel)
 
 	return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index);
 }
-EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
 
 /*
  * Get first vmbus packet from ring buffer after read_index
@@ -534,8 +534,7 @@ EXPORT_SYMBOL_GPL(hv_pkt_iter_first);
  */
 struct vmpacket_descriptor *
 __hv_pkt_iter_next(struct vmbus_channel *channel,
-		   const struct vmpacket_descriptor *desc,
-		   bool copy)
+		   const struct vmpacket_descriptor *desc)
 {
 	struct hv_ring_buffer_info *rbi = &channel->inbound;
 	u32 packetlen = desc->len8 << 3;
@@ -548,7 +547,7 @@ __hv_pkt_iter_next(struct vmbus_channel *channel,
 		rbi->priv_read_index -= dsize;
 
 	/* more data? */
-	return copy ? hv_pkt_iter_first(channel) : hv_pkt_iter_first_raw(channel);
+	return hv_pkt_iter_first(channel);
 }
 EXPORT_SYMBOL_GPL(__hv_pkt_iter_next);
 
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 1112c5cf894e6..370adc9971d3e 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1673,55 +1673,28 @@ static inline u32 hv_pkt_len(const struct vmpacket_descriptor *desc)
 	return desc->len8 << 3;
 }
 
-struct vmpacket_descriptor *
-hv_pkt_iter_first_raw(struct vmbus_channel *channel);
-
 struct vmpacket_descriptor *
 hv_pkt_iter_first(struct vmbus_channel *channel);
 
 struct vmpacket_descriptor *
 __hv_pkt_iter_next(struct vmbus_channel *channel,
-		   const struct vmpacket_descriptor *pkt,
-		   bool copy);
+		   const struct vmpacket_descriptor *pkt);
 
 void hv_pkt_iter_close(struct vmbus_channel *channel);
 
 static inline struct vmpacket_descriptor *
-hv_pkt_iter_next_pkt(struct vmbus_channel *channel,
-		     const struct vmpacket_descriptor *pkt,
-		     bool copy)
+hv_pkt_iter_next(struct vmbus_channel *channel,
+		 const struct vmpacket_descriptor *pkt)
 {
 	struct vmpacket_descriptor *nxt;
 
-	nxt = __hv_pkt_iter_next(channel, pkt, copy);
+	nxt = __hv_pkt_iter_next(channel, pkt);
 	if (!nxt)
 		hv_pkt_iter_close(channel);
 
 	return nxt;
 }
 
-/*
- * Get next packet descriptor without copying it out of the ring buffer
- * If at end of list, return NULL and update host.
- */
-static inline struct vmpacket_descriptor *
-hv_pkt_iter_next_raw(struct vmbus_channel *channel,
-		     const struct vmpacket_descriptor *pkt)
-{
-	return hv_pkt_iter_next_pkt(channel, pkt, false);
-}
-
-/*
- * Get next packet descriptor from iterator
- * If at end of list, return NULL and update host.
- */
-static inline struct vmpacket_descriptor *
-hv_pkt_iter_next(struct vmbus_channel *channel,
-		 const struct vmpacket_descriptor *pkt)
-{
-	return hv_pkt_iter_next_pkt(channel, pkt, true);
-}
-
 #define foreach_vmbus_pkt(pkt, channel) \
 	for (pkt = hv_pkt_iter_first(channel); pkt; \
 	    pkt = hv_pkt_iter_next(channel, pkt))
-- 
2.25.1

RE: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions

[Michael Kelley (LINUX) via Virtualization]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> 
> With no users of hv_pkt_iter_next_raw() and no "external" users of
> hv_pkt_iter_first_raw(), the iterator functions can be refactored
> and simplified to remove some indirection/code.
> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  drivers/hv/ring_buffer.c | 11 +++++------
>  include/linux/hyperv.h   | 35 ++++-------------------------------
>  2 files changed, 9 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 3d215d9dec433..c9357dae2a2c8 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -421,7 +421,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
>  	memcpy(buffer, (const char *)desc + offset, packetlen);
> 
>  	/* Advance ring index to next packet descriptor */
> -	__hv_pkt_iter_next(channel, desc, true);
> +	__hv_pkt_iter_next(channel, desc);
> 
>  	/* Notify host of update */
>  	hv_pkt_iter_close(channel);
> @@ -459,7 +459,8 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info
> *rbi)
>  /*
>   * Get first vmbus packet without copying it out of the ring buffer
>   */
> -struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel)
> +static struct vmpacket_descriptor *
> +hv_pkt_iter_first_raw(struct vmbus_channel *channel)
>  {
>  	struct hv_ring_buffer_info *rbi = &channel->inbound;
> 
> @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> vmbus_channel *channel)
> 
>  	return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> >priv_read_index);
>  }
> -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);

Does hv_pkt_iter_first_raw() need to be retained at all as a
separate function?  I think after these changes, the only caller
is hv_pkt_iter_first(), in which case the code could just go
inline in hv_pkt_iter_first().  Doing that combining would
also allow the elimination of the duplicate call to 
hv_pkt_iter_avail().

Michael

> 
>  /*
>   * Get first vmbus packet from ring buffer after read_index
> @@ -534,8 +534,7 @@ EXPORT_SYMBOL_GPL(hv_pkt_iter_first);
>   */
>  struct vmpacket_descriptor *
>  __hv_pkt_iter_next(struct vmbus_channel *channel,
> -		   const struct vmpacket_descriptor *desc,
> -		   bool copy)
> +		   const struct vmpacket_descriptor *desc)
>  {
>  	struct hv_ring_buffer_info *rbi = &channel->inbound;
>  	u32 packetlen = desc->len8 << 3;
> @@ -548,7 +547,7 @@ __hv_pkt_iter_next(struct vmbus_channel *channel,
>  		rbi->priv_read_index -= dsize;
> 
>  	/* more data? */
> -	return copy ? hv_pkt_iter_first(channel) : hv_pkt_iter_first_raw(channel);
> +	return hv_pkt_iter_first(channel);
>  }
>  EXPORT_SYMBOL_GPL(__hv_pkt_iter_next);
> 
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 1112c5cf894e6..370adc9971d3e 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1673,55 +1673,28 @@ static inline u32 hv_pkt_len(const struct
> vmpacket_descriptor *desc)
>  	return desc->len8 << 3;
>  }
> 
> -struct vmpacket_descriptor *
> -hv_pkt_iter_first_raw(struct vmbus_channel *channel);
> -
>  struct vmpacket_descriptor *
>  hv_pkt_iter_first(struct vmbus_channel *channel);
> 
>  struct vmpacket_descriptor *
>  __hv_pkt_iter_next(struct vmbus_channel *channel,
> -		   const struct vmpacket_descriptor *pkt,
> -		   bool copy);
> +		   const struct vmpacket_descriptor *pkt);
> 
>  void hv_pkt_iter_close(struct vmbus_channel *channel);
> 
>  static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next_pkt(struct vmbus_channel *channel,
> -		     const struct vmpacket_descriptor *pkt,
> -		     bool copy)
> +hv_pkt_iter_next(struct vmbus_channel *channel,
> +		 const struct vmpacket_descriptor *pkt)
>  {
>  	struct vmpacket_descriptor *nxt;
> 
> -	nxt = __hv_pkt_iter_next(channel, pkt, copy);
> +	nxt = __hv_pkt_iter_next(channel, pkt);
>  	if (!nxt)
>  		hv_pkt_iter_close(channel);
> 
>  	return nxt;
>  }
> 
> -/*
> - * Get next packet descriptor without copying it out of the ring buffer
> - * If at end of list, return NULL and update host.
> - */
> -static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next_raw(struct vmbus_channel *channel,
> -		     const struct vmpacket_descriptor *pkt)
> -{
> -	return hv_pkt_iter_next_pkt(channel, pkt, false);
> -}
> -
> -/*
> - * Get next packet descriptor from iterator
> - * If at end of list, return NULL and update host.
> - */
> -static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next(struct vmbus_channel *channel,
> -		 const struct vmpacket_descriptor *pkt)
> -{
> -	return hv_pkt_iter_next_pkt(channel, pkt, true);
> -}
> -
>  #define foreach_vmbus_pkt(pkt, channel) \
>  	for (pkt = hv_pkt_iter_first(channel); pkt; \
>  	    pkt = hv_pkt_iter_next(channel, pkt))
> --
> 2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions

[Michael Kelley (LINUX)]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM
> 
> With no users of hv_pkt_iter_next_raw() and no "external" users of
> hv_pkt_iter_first_raw(), the iterator functions can be refactored
> and simplified to remove some indirection/code.
> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  drivers/hv/ring_buffer.c | 11 +++++------
>  include/linux/hyperv.h   | 35 ++++-------------------------------
>  2 files changed, 9 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 3d215d9dec433..c9357dae2a2c8 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -421,7 +421,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
>  	memcpy(buffer, (const char *)desc + offset, packetlen);
> 
>  	/* Advance ring index to next packet descriptor */
> -	__hv_pkt_iter_next(channel, desc, true);
> +	__hv_pkt_iter_next(channel, desc);
> 
>  	/* Notify host of update */
>  	hv_pkt_iter_close(channel);
> @@ -459,7 +459,8 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info
> *rbi)
>  /*
>   * Get first vmbus packet without copying it out of the ring buffer
>   */
> -struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel)
> +static struct vmpacket_descriptor *
> +hv_pkt_iter_first_raw(struct vmbus_channel *channel)
>  {
>  	struct hv_ring_buffer_info *rbi = &channel->inbound;
> 
> @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> vmbus_channel *channel)
> 
>  	return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> >priv_read_index);
>  }
> -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);

Does hv_pkt_iter_first_raw() need to be retained at all as a
separate function?  I think after these changes, the only caller
is hv_pkt_iter_first(), in which case the code could just go
inline in hv_pkt_iter_first().  Doing that combining would
also allow the elimination of the duplicate call to 
hv_pkt_iter_avail().

Michael

> 
>  /*
>   * Get first vmbus packet from ring buffer after read_index
> @@ -534,8 +534,7 @@ EXPORT_SYMBOL_GPL(hv_pkt_iter_first);
>   */
>  struct vmpacket_descriptor *
>  __hv_pkt_iter_next(struct vmbus_channel *channel,
> -		   const struct vmpacket_descriptor *desc,
> -		   bool copy)
> +		   const struct vmpacket_descriptor *desc)
>  {
>  	struct hv_ring_buffer_info *rbi = &channel->inbound;
>  	u32 packetlen = desc->len8 << 3;
> @@ -548,7 +547,7 @@ __hv_pkt_iter_next(struct vmbus_channel *channel,
>  		rbi->priv_read_index -= dsize;
> 
>  	/* more data? */
> -	return copy ? hv_pkt_iter_first(channel) : hv_pkt_iter_first_raw(channel);
> +	return hv_pkt_iter_first(channel);
>  }
>  EXPORT_SYMBOL_GPL(__hv_pkt_iter_next);
> 
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 1112c5cf894e6..370adc9971d3e 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1673,55 +1673,28 @@ static inline u32 hv_pkt_len(const struct
> vmpacket_descriptor *desc)
>  	return desc->len8 << 3;
>  }
> 
> -struct vmpacket_descriptor *
> -hv_pkt_iter_first_raw(struct vmbus_channel *channel);
> -
>  struct vmpacket_descriptor *
>  hv_pkt_iter_first(struct vmbus_channel *channel);
> 
>  struct vmpacket_descriptor *
>  __hv_pkt_iter_next(struct vmbus_channel *channel,
> -		   const struct vmpacket_descriptor *pkt,
> -		   bool copy);
> +		   const struct vmpacket_descriptor *pkt);
> 
>  void hv_pkt_iter_close(struct vmbus_channel *channel);
> 
>  static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next_pkt(struct vmbus_channel *channel,
> -		     const struct vmpacket_descriptor *pkt,
> -		     bool copy)
> +hv_pkt_iter_next(struct vmbus_channel *channel,
> +		 const struct vmpacket_descriptor *pkt)
>  {
>  	struct vmpacket_descriptor *nxt;
> 
> -	nxt = __hv_pkt_iter_next(channel, pkt, copy);
> +	nxt = __hv_pkt_iter_next(channel, pkt);
>  	if (!nxt)
>  		hv_pkt_iter_close(channel);
> 
>  	return nxt;
>  }
> 
> -/*
> - * Get next packet descriptor without copying it out of the ring buffer
> - * If at end of list, return NULL and update host.
> - */
> -static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next_raw(struct vmbus_channel *channel,
> -		     const struct vmpacket_descriptor *pkt)
> -{
> -	return hv_pkt_iter_next_pkt(channel, pkt, false);
> -}
> -
> -/*
> - * Get next packet descriptor from iterator
> - * If at end of list, return NULL and update host.
> - */
> -static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next(struct vmbus_channel *channel,
> -		 const struct vmpacket_descriptor *pkt)
> -{
> -	return hv_pkt_iter_next_pkt(channel, pkt, true);
> -}
> -
>  #define foreach_vmbus_pkt(pkt, channel) \
>  	for (pkt = hv_pkt_iter_first(channel); pkt; \
>  	    pkt = hv_pkt_iter_next(channel, pkt))
> --
> 2.25.1

Re: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions

[Andrea Parri]

> > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> > vmbus_channel *channel)
> > 
> >  	return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> > >priv_read_index);
> >  }
> > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
> 
> Does hv_pkt_iter_first_raw() need to be retained at all as a
> separate function?  I think after these changes, the only caller
> is hv_pkt_iter_first(), in which case the code could just go
> inline in hv_pkt_iter_first().  Doing that combining would
> also allow the elimination of the duplicate call to 
> hv_pkt_iter_avail().

Good point.  Will do.

Thanks,
  Andrea

Re: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions

[Andrea Parri]

On Fri, Apr 15, 2022 at 09:00:31AM +0200, Andrea Parri wrote:
> > > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> > > vmbus_channel *channel)
> > > 
> > >  	return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> > > >priv_read_index);
> > >  }
> > > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
> > 
> > Does hv_pkt_iter_first_raw() need to be retained at all as a
> > separate function?  I think after these changes, the only caller
> > is hv_pkt_iter_first(), in which case the code could just go
> > inline in hv_pkt_iter_first().  Doing that combining would
> > also allow the elimination of the duplicate call to 
> > hv_pkt_iter_avail().

Back to this, can you clarify what you mean by "the elimination of..."?
After moving the function "inline", hv_pkt_iter_avail() would be called
in to check for a non-NULL descriptor (in the inline function) and later
in the computation of bytes_avail.

Thanks,
  Andrea


> 
> Good point.  Will do.
> 
> Thanks,
>   Andrea

RE: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions

[Michael Kelley (LINUX)]

From: Andrea Parri <parri.andrea@gmail.com> Sent: Friday, April 15, 2022 9:28 AM
> 
> On Fri, Apr 15, 2022 at 09:00:31AM +0200, Andrea Parri wrote:
> > > > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> > > > vmbus_channel *channel)
> > > >
> > > >  	return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> > > > >priv_read_index);
> > > >  }
> > > > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
> > >
> > > Does hv_pkt_iter_first_raw() need to be retained at all as a
> > > separate function?  I think after these changes, the only caller
> > > is hv_pkt_iter_first(), in which case the code could just go
> > > inline in hv_pkt_iter_first().  Doing that combining would
> > > also allow the elimination of the duplicate call to
> > > hv_pkt_iter_avail().
> 
> Back to this, can you clarify what you mean by "the elimination of..."?
> After moving the function "inline", hv_pkt_iter_avail() would be called
> in to check for a non-NULL descriptor (in the inline function) and later
> in the computation of bytes_avail.

I was thinking something like this:

bytes_avail = hv_pkt_iter_avail(rbi);
if (bytes_avail < sizeof(struct vmpacket_descriptor))
	return NULL;
bytes_avail = min(rbi->pkt_buffer_size, bytes_avail);

desc = (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index);

And for that matter, hv_pkt_iter_avail() is now only called in one place.
It's a judgment call whether to keep it as a separate helper function vs.
inlining it in hv_pkt_iter_first() as well.  I'm OK either way.


Michael

RE: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions

[Michael Kelley (LINUX) via Virtualization]

From: Andrea Parri <parri.andrea@gmail.com> Sent: Friday, April 15, 2022 9:28 AM
> 
> On Fri, Apr 15, 2022 at 09:00:31AM +0200, Andrea Parri wrote:
> > > > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> > > > vmbus_channel *channel)
> > > >
> > > >  	return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> > > > >priv_read_index);
> > > >  }
> > > > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
> > >
> > > Does hv_pkt_iter_first_raw() need to be retained at all as a
> > > separate function?  I think after these changes, the only caller
> > > is hv_pkt_iter_first(), in which case the code could just go
> > > inline in hv_pkt_iter_first().  Doing that combining would
> > > also allow the elimination of the duplicate call to
> > > hv_pkt_iter_avail().
> 
> Back to this, can you clarify what you mean by "the elimination of..."?
> After moving the function "inline", hv_pkt_iter_avail() would be called
> in to check for a non-NULL descriptor (in the inline function) and later
> in the computation of bytes_avail.

I was thinking something like this:

bytes_avail = hv_pkt_iter_avail(rbi);
if (bytes_avail < sizeof(struct vmpacket_descriptor))
	return NULL;
bytes_avail = min(rbi->pkt_buffer_size, bytes_avail);

desc = (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index);

And for that matter, hv_pkt_iter_avail() is now only called in one place.
It's a judgment call whether to keep it as a separate helper function vs.
inlining it in hv_pkt_iter_first() as well.  I'm OK either way.


Michael


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions

[Andrea Parri]

On Fri, Apr 15, 2022 at 04:44:50PM +0000, Michael Kelley (LINUX) wrote:
> From: Andrea Parri <parri.andrea@gmail.com> Sent: Friday, April 15, 2022 9:28 AM
> > 
> > On Fri, Apr 15, 2022 at 09:00:31AM +0200, Andrea Parri wrote:
> > > > > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct
> > > > > vmbus_channel *channel)
> > > > >
> > > > >  	return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> > > > > >priv_read_index);
> > > > >  }
> > > > > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
> > > >
> > > > Does hv_pkt_iter_first_raw() need to be retained at all as a
> > > > separate function?  I think after these changes, the only caller
> > > > is hv_pkt_iter_first(), in which case the code could just go
> > > > inline in hv_pkt_iter_first().  Doing that combining would
> > > > also allow the elimination of the duplicate call to
> > > > hv_pkt_iter_avail().
> > 
> > Back to this, can you clarify what you mean by "the elimination of..."?
> > After moving the function "inline", hv_pkt_iter_avail() would be called
> > in to check for a non-NULL descriptor (in the inline function) and later
> > in the computation of bytes_avail.
> 
> I was thinking something like this:
> 
> bytes_avail = hv_pkt_iter_avail(rbi);
> if (bytes_avail < sizeof(struct vmpacket_descriptor))
> 	return NULL;
> bytes_avail = min(rbi->pkt_buffer_size, bytes_avail);
> 
> desc = (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index);

Thanks for the clarification, I've applied it.

  Andrea


> And for that matter, hv_pkt_iter_avail() is now only called in one place.
> It's a judgment call whether to keep it as a separate helper function vs.
> inlining it in hv_pkt_iter_first() as well.  I'm OK either way.
> 
> 
> Michael
> 
>