All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/6] hv_sock: Hardening changes
@ 2022-04-13 20:47 Andrea Parri (Microsoft)
  2022-04-13 20:47 ` [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value Andrea Parri (Microsoft)
                   ` (5 more replies)
  0 siblings, 6 replies; 32+ messages in thread
From: Andrea Parri (Microsoft) @ 2022-04-13 20:47 UTC (permalink / raw)
  To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Michael Kelley, Stefano Garzarella, David Miller,
	Jakub Kicinski, Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel,
	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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value
  2022-04-13 20:47 [RFC PATCH 0/6] hv_sock: Hardening changes Andrea Parri (Microsoft)
@ 2022-04-13 20:47 ` Andrea Parri (Microsoft)
  2022-04-15  3:33     ` Michael Kelley (LINUX)
  2022-04-13 20:47 ` [RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer Andrea Parri (Microsoft)
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 32+ messages in thread
From: Andrea Parri (Microsoft) @ 2022-04-13 20:47 UTC (permalink / raw)
  To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Michael Kelley, Stefano Garzarella, David Miller,
	Jakub Kicinski, Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel,
	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


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer
  2022-04-13 20:47 [RFC PATCH 0/6] hv_sock: Hardening changes Andrea Parri (Microsoft)
  2022-04-13 20:47 ` [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value Andrea Parri (Microsoft)
@ 2022-04-13 20:47 ` Andrea Parri (Microsoft)
  2022-04-15  3:33     ` Michael Kelley (LINUX)
  2022-04-13 20:47 ` [RFC PATCH 3/6] hv_sock: Add validation for untrusted Hyper-V values Andrea Parri (Microsoft)
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 32+ messages in thread
From: Andrea Parri (Microsoft) @ 2022-04-13 20:47 UTC (permalink / raw)
  To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Michael Kelley, Stefano Garzarella, David Miller,
	Jakub Kicinski, Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel,
	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


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 3/6] hv_sock: Add validation for untrusted Hyper-V values
  2022-04-13 20:47 [RFC PATCH 0/6] hv_sock: Hardening changes Andrea Parri (Microsoft)
  2022-04-13 20:47 ` [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value Andrea Parri (Microsoft)
  2022-04-13 20:47 ` [RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer Andrea Parri (Microsoft)
@ 2022-04-13 20:47 ` Andrea Parri (Microsoft)
  2022-04-13 20:47 ` [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue() Andrea Parri (Microsoft)
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 32+ messages in thread
From: Andrea Parri (Microsoft) @ 2022-04-13 20:47 UTC (permalink / raw)
  To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Michael Kelley, Stefano Garzarella, David Miller,
	Jakub Kicinski, Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel,
	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


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()
  2022-04-13 20:47 [RFC PATCH 0/6] hv_sock: Hardening changes Andrea Parri (Microsoft)
                   ` (2 preceding siblings ...)
  2022-04-13 20:47 ` [RFC PATCH 3/6] hv_sock: Add validation for untrusted Hyper-V values Andrea Parri (Microsoft)
@ 2022-04-13 20:47 ` Andrea Parri (Microsoft)
  2022-04-15  3:33     ` Michael Kelley (LINUX)
  2022-04-13 20:47 ` [RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests Andrea Parri (Microsoft)
  2022-04-13 20:47 ` [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions Andrea Parri (Microsoft)
  5 siblings, 1 reply; 32+ messages in thread
From: Andrea Parri (Microsoft) @ 2022-04-13 20:47 UTC (permalink / raw)
  To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Michael Kelley, Stefano Garzarella, David Miller,
	Jakub Kicinski, Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel,
	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


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests
  2022-04-13 20:47 [RFC PATCH 0/6] hv_sock: Hardening changes Andrea Parri (Microsoft)
                   ` (3 preceding siblings ...)
  2022-04-13 20:47 ` [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue() Andrea Parri (Microsoft)
@ 2022-04-13 20:47 ` Andrea Parri (Microsoft)
  2022-04-15  3:33     ` Michael Kelley (LINUX)
  2022-04-13 20:47 ` [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions Andrea Parri (Microsoft)
  5 siblings, 1 reply; 32+ messages in thread
From: Andrea Parri (Microsoft) @ 2022-04-13 20:47 UTC (permalink / raw)
  To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Michael Kelley, Stefano Garzarella, David Miller,
	Jakub Kicinski, Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel,
	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


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions
  2022-04-13 20:47 [RFC PATCH 0/6] hv_sock: Hardening changes Andrea Parri (Microsoft)
                   ` (4 preceding siblings ...)
  2022-04-13 20:47 ` [RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests Andrea Parri (Microsoft)
@ 2022-04-13 20:47 ` Andrea Parri (Microsoft)
  2022-04-15  3:34     ` Michael Kelley (LINUX)
  5 siblings, 1 reply; 32+ messages in thread
From: Andrea Parri (Microsoft) @ 2022-04-13 20:47 UTC (permalink / raw)
  To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Michael Kelley, Stefano Garzarella, David Miller,
	Jakub Kicinski, Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel,
	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


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value
  2022-04-13 20:47 ` [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value Andrea Parri (Microsoft)
@ 2022-04-15  3:33     ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) via Virtualization @ 2022-04-15  3:33 UTC (permalink / raw)
  To: Andrea Parri (Microsoft),
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni
  Cc: netdev, linux-hyperv, linux-kernel, 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value
@ 2022-04-15  3:33     ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) @ 2022-04-15  3:33 UTC (permalink / raw)
  To: Andrea Parri (Microsoft),
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer
  2022-04-13 20:47 ` [RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer Andrea Parri (Microsoft)
@ 2022-04-15  3:33     ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) via Virtualization @ 2022-04-15  3:33 UTC (permalink / raw)
  To: Andrea Parri (Microsoft),
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni
  Cc: netdev, linux-hyperv, linux-kernel, 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer
@ 2022-04-15  3:33     ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) @ 2022-04-15  3:33 UTC (permalink / raw)
  To: Andrea Parri (Microsoft),
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()
  2022-04-13 20:47 ` [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue() Andrea Parri (Microsoft)
@ 2022-04-15  3:33     ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) via Virtualization @ 2022-04-15  3:33 UTC (permalink / raw)
  To: Andrea Parri (Microsoft),
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni
  Cc: netdev, linux-hyperv, linux-kernel, 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()
@ 2022-04-15  3:33     ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) @ 2022-04-15  3:33 UTC (permalink / raw)
  To: Andrea Parri (Microsoft),
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests
  2022-04-13 20:47 ` [RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests Andrea Parri (Microsoft)
@ 2022-04-15  3:33     ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) via Virtualization @ 2022-04-15  3:33 UTC (permalink / raw)
  To: Andrea Parri (Microsoft),
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni
  Cc: netdev, linux-hyperv, linux-kernel, 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests
@ 2022-04-15  3:33     ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) @ 2022-04-15  3:33 UTC (permalink / raw)
  To: Andrea Parri (Microsoft),
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions
  2022-04-13 20:47 ` [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions Andrea Parri (Microsoft)
@ 2022-04-15  3:34     ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) via Virtualization @ 2022-04-15  3:34 UTC (permalink / raw)
  To: Andrea Parri (Microsoft),
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni
  Cc: netdev, linux-hyperv, linux-kernel, 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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions
@ 2022-04-15  3:34     ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) @ 2022-04-15  3:34 UTC (permalink / raw)
  To: Andrea Parri (Microsoft),
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni
  Cc: linux-hyperv, virtualization, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value
  2022-04-15  3:33     ` Michael Kelley (LINUX)
  (?)
@ 2022-04-15  6:41     ` Andrea Parri
  2022-04-15 14:27         ` Michael Kelley (LINUX)
  -1 siblings, 1 reply; 32+ messages in thread
From: Andrea Parri @ 2022-04-15  6:41 UTC (permalink / raw)
  To: Michael Kelley (LINUX)
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer
  2022-04-15  3:33     ` Michael Kelley (LINUX)
  (?)
@ 2022-04-15  6:42     ` Andrea Parri
  -1 siblings, 0 replies; 32+ messages in thread
From: Andrea Parri @ 2022-04-15  6:42 UTC (permalink / raw)
  To: Michael Kelley (LINUX)
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()
  2022-04-15  3:33     ` Michael Kelley (LINUX)
  (?)
@ 2022-04-15  6:50     ` Andrea Parri
  2022-04-15 14:30         ` Michael Kelley (LINUX)
  -1 siblings, 1 reply; 32+ messages in thread
From: Andrea Parri @ 2022-04-15  6:50 UTC (permalink / raw)
  To: Michael Kelley (LINUX)
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests
  2022-04-15  3:33     ` Michael Kelley (LINUX)
  (?)
@ 2022-04-15  6:58     ` Andrea Parri
  -1 siblings, 0 replies; 32+ messages in thread
From: Andrea Parri @ 2022-04-15  6:58 UTC (permalink / raw)
  To: Michael Kelley (LINUX)
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions
  2022-04-15  3:34     ` Michael Kelley (LINUX)
  (?)
@ 2022-04-15  7:00     ` Andrea Parri
  2022-04-15 16:28       ` Andrea Parri
  -1 siblings, 1 reply; 32+ messages in thread
From: Andrea Parri @ 2022-04-15  7:00 UTC (permalink / raw)
  To: Michael Kelley (LINUX)
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value
  2022-04-15  6:41     ` Andrea Parri
@ 2022-04-15 14:27         ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) via Virtualization @ 2022-04-15 14:27 UTC (permalink / raw)
  To: Andrea Parri
  Cc: Wei Liu, Paolo Abeni, Stephen Hemminger, netdev, Haiyang Zhang,
	Dexuan Cui, linux-hyperv, virtualization, Jakub Kicinski,
	David Miller, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value
@ 2022-04-15 14:27         ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) @ 2022-04-15 14:27 UTC (permalink / raw)
  To: Andrea Parri
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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




^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()
  2022-04-15  6:50     ` Andrea Parri
@ 2022-04-15 14:30         ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) via Virtualization @ 2022-04-15 14:30 UTC (permalink / raw)
  To: Andrea Parri
  Cc: Wei Liu, Paolo Abeni, Stephen Hemminger, netdev, Haiyang Zhang,
	Dexuan Cui, linux-hyperv, virtualization, Jakub Kicinski,
	David Miller, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()
@ 2022-04-15 14:30         ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) @ 2022-04-15 14:30 UTC (permalink / raw)
  To: Andrea Parri
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value
  2022-04-15 14:27         ` Michael Kelley (LINUX)
  (?)
@ 2022-04-15 16:09         ` Andrea Parri
  -1 siblings, 0 replies; 32+ messages in thread
From: Andrea Parri @ 2022-04-15 16:09 UTC (permalink / raw)
  To: Michael Kelley (LINUX)
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue()
  2022-04-15 14:30         ` Michael Kelley (LINUX)
  (?)
@ 2022-04-15 16:16         ` Andrea Parri
  -1 siblings, 0 replies; 32+ messages in thread
From: Andrea Parri @ 2022-04-15 16:16 UTC (permalink / raw)
  To: Michael Kelley (LINUX)
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions
  2022-04-15  7:00     ` Andrea Parri
@ 2022-04-15 16:28       ` Andrea Parri
  2022-04-15 16:44           ` Michael Kelley (LINUX) via Virtualization
  0 siblings, 1 reply; 32+ messages in thread
From: Andrea Parri @ 2022-04-15 16:28 UTC (permalink / raw)
  To: Michael Kelley (LINUX)
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions
  2022-04-15 16:28       ` Andrea Parri
@ 2022-04-15 16:44           ` Michael Kelley (LINUX) via Virtualization
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) @ 2022-04-15 16:44 UTC (permalink / raw)
  To: Andrea Parri
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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



^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions
@ 2022-04-15 16:44           ` Michael Kelley (LINUX) via Virtualization
  0 siblings, 0 replies; 32+ messages in thread
From: Michael Kelley (LINUX) via Virtualization @ 2022-04-15 16:44 UTC (permalink / raw)
  To: Andrea Parri
  Cc: Wei Liu, Paolo Abeni, Stephen Hemminger, netdev, Haiyang Zhang,
	Dexuan Cui, linux-hyperv, virtualization, Jakub Kicinski,
	David Miller, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions
  2022-04-15 16:44           ` Michael Kelley (LINUX) via Virtualization
  (?)
@ 2022-04-15 17:05           ` Andrea Parri
  -1 siblings, 0 replies; 32+ messages in thread
From: Andrea Parri @ 2022-04-15 17:05 UTC (permalink / raw)
  To: Michael Kelley (LINUX)
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Stefano Garzarella, David Miller, Jakub Kicinski,
	Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2022-04-15 17:06 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-13 20:47 [RFC PATCH 0/6] hv_sock: Hardening changes Andrea Parri (Microsoft)
2022-04-13 20:47 ` [RFC PATCH 1/6] hv_sock: Check hv_pkt_iter_first_raw()'s return value Andrea Parri (Microsoft)
2022-04-15  3:33   ` Michael Kelley (LINUX) via Virtualization
2022-04-15  3:33     ` Michael Kelley (LINUX)
2022-04-15  6:41     ` Andrea Parri
2022-04-15 14:27       ` Michael Kelley (LINUX) via Virtualization
2022-04-15 14:27         ` Michael Kelley (LINUX)
2022-04-15 16:09         ` Andrea Parri
2022-04-13 20:47 ` [RFC PATCH 2/6] hv_sock: Copy packets sent by Hyper-V out of the ring buffer Andrea Parri (Microsoft)
2022-04-15  3:33   ` Michael Kelley (LINUX) via Virtualization
2022-04-15  3:33     ` Michael Kelley (LINUX)
2022-04-15  6:42     ` Andrea Parri
2022-04-13 20:47 ` [RFC PATCH 3/6] hv_sock: Add validation for untrusted Hyper-V values Andrea Parri (Microsoft)
2022-04-13 20:47 ` [RFC PATCH 4/6] hv_sock: Initialize send_buf in hvs_stream_enqueue() Andrea Parri (Microsoft)
2022-04-15  3:33   ` Michael Kelley (LINUX) via Virtualization
2022-04-15  3:33     ` Michael Kelley (LINUX)
2022-04-15  6:50     ` Andrea Parri
2022-04-15 14:30       ` Michael Kelley (LINUX) via Virtualization
2022-04-15 14:30         ` Michael Kelley (LINUX)
2022-04-15 16:16         ` Andrea Parri
2022-04-13 20:47 ` [RFC PATCH 5/6] Drivers: hv: vmbus: Accept hv_sock offers in isolated guests Andrea Parri (Microsoft)
2022-04-15  3:33   ` Michael Kelley (LINUX) via Virtualization
2022-04-15  3:33     ` Michael Kelley (LINUX)
2022-04-15  6:58     ` Andrea Parri
2022-04-13 20:47 ` [RFC PATCH 6/6] Drivers: hv: vmbus: Refactor the ring-buffer iterator functions Andrea Parri (Microsoft)
2022-04-15  3:34   ` Michael Kelley (LINUX) via Virtualization
2022-04-15  3:34     ` Michael Kelley (LINUX)
2022-04-15  7:00     ` Andrea Parri
2022-04-15 16:28       ` Andrea Parri
2022-04-15 16:44         ` Michael Kelley (LINUX)
2022-04-15 16:44           ` Michael Kelley (LINUX) via Virtualization
2022-04-15 17:05           ` Andrea Parri

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.