netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Drivers: hv: vmbus: Restrict devices and configurations on 'isolated' guests
@ 2021-01-19 17:58 Andrea Parri (Microsoft)
  2021-01-19 17:58 ` [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests Andrea Parri (Microsoft)
  0 siblings, 1 reply; 7+ messages in thread
From: Andrea Parri (Microsoft) @ 2021-01-19 17:58 UTC (permalink / raw)
  To: linux-kernel
  Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Michael Kelley, Tianyu Lan, Saruhan Karademir, Juan Vazquez,
	linux-hyperv, Andrea Parri (Microsoft), Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, Arnd Bergmann,
	David S. Miller, Jakub Kicinski, x86, linux-arch, netdev

Hi all,

To reduce the footprint of the code that will be exercised, and hence
the exposure to bugs and vulnerabilities, restrict configurations and
devices on 'isolated' VMs.

Specs of the Isolation Configuration leaf (cf. patch #1) were derived
from internal discussions with the Hyper-V team and, AFAICT, they are
not publicly available yet.

The series has some minor/naming conflict with on-going work aimed at
enabling SNP VMs on Hyper-V[1]; such conflicts can be addressed later
at the right time.

Applies to hyperv-next.

Thanks,
  Andrea

[1] https://github.com/lantianyu/linux # cvm

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: x86@kernel.org
Cc: linux-arch@vger.kernel.org
Cc: netdev@vger.kernel.org

Andrea Parri (Microsoft) (4):
  x86/hyperv: Load/save the Isolation Configuration leaf
  Drivers: hv: vmbus: Restrict vmbus_devices on isolated guests
  Drivers: hv: vmbus: Enforce 'VMBus version >= 5.2' on isolated guests
  hv_netvsc: Restrict configurations on isolated guests

 arch/x86/hyperv/hv_init.c          | 15 +++++++++++++
 arch/x86/include/asm/hyperv-tlfs.h | 15 +++++++++++++
 arch/x86/kernel/cpu/mshyperv.c     |  9 ++++++++
 drivers/hv/channel_mgmt.c          | 36 ++++++++++++++++++++++++++++++
 drivers/hv/connection.c            | 13 +++++++++++
 drivers/net/hyperv/netvsc.c        | 21 ++++++++++++++---
 include/asm-generic/hyperv-tlfs.h  |  1 +
 include/asm-generic/mshyperv.h     |  5 +++++
 include/linux/hyperv.h             |  1 +
 9 files changed, 113 insertions(+), 3 deletions(-)

-- 
2.25.1


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

* [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests
  2021-01-19 17:58 [PATCH 0/4] Drivers: hv: vmbus: Restrict devices and configurations on 'isolated' guests Andrea Parri (Microsoft)
@ 2021-01-19 17:58 ` Andrea Parri (Microsoft)
  2021-01-20 19:24   ` Haiyang Zhang
  2021-01-21  1:26   ` Jakub Kicinski
  0 siblings, 2 replies; 7+ messages in thread
From: Andrea Parri (Microsoft) @ 2021-01-19 17:58 UTC (permalink / raw)
  To: linux-kernel
  Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Michael Kelley, Tianyu Lan, Saruhan Karademir, Juan Vazquez,
	linux-hyperv, Andrea Parri (Microsoft), David S. Miller,
	Jakub Kicinski, netdev

Restrict the NVSP protocol version(s) that will be negotiated with the
host to be NVSP_PROTOCOL_VERSION_61 or greater if the guest is running
isolated.  Moreover, do not advertise the SR-IOV capability and ignore
NVSP_MSG_4_TYPE_SEND_VF_ASSOCIATION messages in isolated guests, which
are not supposed to support SR-IOV.  This reduces the footprint of the
code that will be exercised by Confidential VMs and hence the exposure
to bugs and vulnerabilities.

Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org
---
 drivers/net/hyperv/netvsc.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 1510a236aa341..8027d553cb67d 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -22,6 +22,7 @@
 #include <linux/prefetch.h>
 
 #include <asm/sync_bitops.h>
+#include <asm/mshyperv.h>
 
 #include "hyperv_net.h"
 #include "netvsc_trace.h"
@@ -544,7 +545,8 @@ static int negotiate_nvsp_ver(struct hv_device *device,
 	init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q = 1;
 
 	if (nvsp_ver >= NVSP_PROTOCOL_VERSION_5) {
-		init_packet->msg.v2_msg.send_ndis_config.capability.sriov = 1;
+		if (!hv_is_isolation_supported())
+			init_packet->msg.v2_msg.send_ndis_config.capability.sriov = 1;
 
 		/* Teaming bit is needed to receive link speed updates */
 		init_packet->msg.v2_msg.send_ndis_config.capability.teaming = 1;
@@ -563,6 +565,13 @@ static int negotiate_nvsp_ver(struct hv_device *device,
 	return ret;
 }
 
+static bool nvsp_is_valid_version(u32 version)
+{
+       if (hv_is_isolation_supported())
+               return version >= NVSP_PROTOCOL_VERSION_61;
+       return true;
+}
+
 static int netvsc_connect_vsp(struct hv_device *device,
 			      struct netvsc_device *net_device,
 			      const struct netvsc_device_info *device_info)
@@ -579,12 +588,17 @@ static int netvsc_connect_vsp(struct hv_device *device,
 	init_packet = &net_device->channel_init_pkt;
 
 	/* Negotiate the latest NVSP protocol supported */
-	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--)
+	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--) {
+		if (!nvsp_is_valid_version(ver_list[i])) {
+			ret = -EPROTO;
+			goto cleanup;
+		}
 		if (negotiate_nvsp_ver(device, net_device, init_packet,
 				       ver_list[i])  == 0) {
 			net_device->nvsp_version = ver_list[i];
 			break;
 		}
+	}
 
 	if (i < 0) {
 		ret = -EPROTO;
@@ -1357,7 +1371,8 @@ static void netvsc_receive_inband(struct net_device *ndev,
 		break;
 
 	case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
-		netvsc_send_vf(ndev, nvmsg, msglen);
+		if (!hv_is_isolation_supported())
+			netvsc_send_vf(ndev, nvmsg, msglen);
 		break;
 	}
 }
-- 
2.25.1


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

* RE: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests
  2021-01-19 17:58 ` [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests Andrea Parri (Microsoft)
@ 2021-01-20 19:24   ` Haiyang Zhang
  2021-01-21  4:05     ` Andrea Parri
  2021-01-21  1:26   ` Jakub Kicinski
  1 sibling, 1 reply; 7+ messages in thread
From: Haiyang Zhang @ 2021-01-20 19:24 UTC (permalink / raw)
  To: Andrea Parri (Microsoft), linux-kernel@vger.kernel.org
  Cc: KY Srinivasan, Stephen Hemminger, Wei Liu, Michael Kelley,
	Tianyu Lan, Saruhan Karademir, Juan Vazquez,
	linux-hyperv@vger.kernel.org, David S. Miller, Jakub Kicinski,
	netdev@vger.kernel.org



> -----Original Message-----
> From: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Sent: Tuesday, January 19, 2021 12:59 PM
> To: linux-kernel@vger.kernel.org
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; Wei Liu <wei.liu@kernel.org>; Michael Kelley
> <mikelley@microsoft.com>; Tianyu Lan <Tianyu.Lan@microsoft.com>;
> Saruhan Karademir <skarade@microsoft.com>; Juan Vazquez
> <juvazq@microsoft.com>; linux-hyperv@vger.kernel.org; Andrea Parri
> (Microsoft) <parri.andrea@gmail.com>; David S. Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
> netdev@vger.kernel.org
> Subject: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests
> 
> Restrict the NVSP protocol version(s) that will be negotiated with the
> host to be NVSP_PROTOCOL_VERSION_61 or greater if the guest is running
> isolated.  Moreover, do not advertise the SR-IOV capability and ignore
> NVSP_MSG_4_TYPE_SEND_VF_ASSOCIATION messages in isolated guests,
> which
> are not supposed to support SR-IOV.  This reduces the footprint of the
> code that will be exercised by Confidential VMs and hence the exposure
> to bugs and vulnerabilities.
> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: netdev@vger.kernel.org
> ---
>  drivers/net/hyperv/netvsc.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 1510a236aa341..8027d553cb67d 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -22,6 +22,7 @@
>  #include <linux/prefetch.h>
> 
>  #include <asm/sync_bitops.h>
> +#include <asm/mshyperv.h>
> 
>  #include "hyperv_net.h"
>  #include "netvsc_trace.h"
> @@ -544,7 +545,8 @@ static int negotiate_nvsp_ver(struct hv_device
> *device,
>  	init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q = 1;
> 
>  	if (nvsp_ver >= NVSP_PROTOCOL_VERSION_5) {
> -		init_packet->msg.v2_msg.send_ndis_config.capability.sriov =
> 1;
> +		if (!hv_is_isolation_supported())
> +			init_packet-
> >msg.v2_msg.send_ndis_config.capability.sriov = 1;

Please also add a log there stating we don't support sriov in this case. Otherwise,
customers will ask why vf not showing up.

> 
>  		/* Teaming bit is needed to receive link speed updates */
>  		init_packet-
> >msg.v2_msg.send_ndis_config.capability.teaming = 1;
> @@ -563,6 +565,13 @@ static int negotiate_nvsp_ver(struct hv_device
> *device,
>  	return ret;
>  }
> 
> +static bool nvsp_is_valid_version(u32 version)
> +{
> +       if (hv_is_isolation_supported())
> +               return version >= NVSP_PROTOCOL_VERSION_61;
> +       return true;
Hosts support isolation should run nvsp 6.1+. This error is not expected.
Instead of fail silently, we should log an error to explain why it's failed, and the current version and expected version.


> +}
> +
>  static int netvsc_connect_vsp(struct hv_device *device,
>  			      struct netvsc_device *net_device,
>  			      const struct netvsc_device_info *device_info)
> @@ -579,12 +588,17 @@ static int netvsc_connect_vsp(struct hv_device
> *device,
>  	init_packet = &net_device->channel_init_pkt;
> 
>  	/* Negotiate the latest NVSP protocol supported */
> -	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--)
> +	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--) {
> +		if (!nvsp_is_valid_version(ver_list[i])) {
> +			ret = -EPROTO;
> +			goto cleanup;
> +		}

This code can catch the invalid, but cannot get the current host nvsp version.
I'd suggest move this check after version negotiation is done. So we can log what's
the current host nvsp version, and why we fail it (the expected nvsp ver).

>  		if (negotiate_nvsp_ver(device, net_device, init_packet,
>  				       ver_list[i])  == 0) {
>  			net_device->nvsp_version = ver_list[i];
>  			break;
>  		}
> +	}
> 
>  	if (i < 0) {
>  		ret = -EPROTO;
> @@ -1357,7 +1371,8 @@ static void netvsc_receive_inband(struct
> net_device *ndev,
>  		break;
> 
>  	case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
> -		netvsc_send_vf(ndev, nvmsg, msglen);
> +		if (!hv_is_isolation_supported())
> +			netvsc_send_vf(ndev, nvmsg, msglen);

When the driver doesn't advertise SRIOV, this message is not expected.
Instead of ignore silently, we should log an error.

Thanks,
- Haiyang


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

* Re: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests
  2021-01-19 17:58 ` [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests Andrea Parri (Microsoft)
  2021-01-20 19:24   ` Haiyang Zhang
@ 2021-01-21  1:26   ` Jakub Kicinski
  1 sibling, 0 replies; 7+ messages in thread
From: Jakub Kicinski @ 2021-01-21  1:26 UTC (permalink / raw)
  To: Andrea Parri (Microsoft)
  Cc: linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Michael Kelley, Tianyu Lan,
	Saruhan Karademir, Juan Vazquez, linux-hyperv, David S. Miller,
	netdev

On Tue, 19 Jan 2021 18:58:41 +0100 Andrea Parri (Microsoft) wrote:
> Restrict the NVSP protocol version(s) that will be negotiated with the
> host to be NVSP_PROTOCOL_VERSION_61 or greater if the guest is running
> isolated.  Moreover, do not advertise the SR-IOV capability and ignore
> NVSP_MSG_4_TYPE_SEND_VF_ASSOCIATION messages in isolated guests, which
> are not supposed to support SR-IOV.  This reduces the footprint of the
> code that will be exercised by Confidential VMs and hence the exposure
> to bugs and vulnerabilities.
> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: netdev@vger.kernel.org

Nothing exciting here from networking perspective, so:

Acked-by: Jakub Kicinski <kuba@kernel.org>

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

* Re: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests
  2021-01-20 19:24   ` Haiyang Zhang
@ 2021-01-21  4:05     ` Andrea Parri
  2021-01-21 16:02       ` Haiyang Zhang
  0 siblings, 1 reply; 7+ messages in thread
From: Andrea Parri @ 2021-01-21  4:05 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: linux-kernel@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
	Wei Liu, Michael Kelley, Tianyu Lan, Saruhan Karademir,
	Juan Vazquez, linux-hyperv@vger.kernel.org, David S. Miller,
	Jakub Kicinski, netdev@vger.kernel.org

> > @@ -544,7 +545,8 @@ static int negotiate_nvsp_ver(struct hv_device
> > *device,
> >  	init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q = 1;
> > 
> >  	if (nvsp_ver >= NVSP_PROTOCOL_VERSION_5) {
> > -		init_packet->msg.v2_msg.send_ndis_config.capability.sriov =
> > 1;
> > +		if (!hv_is_isolation_supported())
> > +			init_packet-
> > >msg.v2_msg.send_ndis_config.capability.sriov = 1;
> 
> Please also add a log there stating we don't support sriov in this case. Otherwise,
> customers will ask why vf not showing up.

IIUC, you're suggesting that I append something like:

+		else
+			netdev_info(ndev, "SR-IOV not advertised: isolation supported\n");

I've added this locally; please let me know if you had something else
/better in mind.


> > @@ -563,6 +565,13 @@ static int negotiate_nvsp_ver(struct hv_device
> > *device,
> >  	return ret;
> >  }
> > 
> > +static bool nvsp_is_valid_version(u32 version)
> > +{
> > +       if (hv_is_isolation_supported())
> > +               return version >= NVSP_PROTOCOL_VERSION_61;
> > +       return true;
> Hosts support isolation should run nvsp 6.1+. This error is not expected.
> Instead of fail silently, we should log an error to explain why it's failed, and the current version and expected version.

Please see my next comment below.


> > +}
> > +
> >  static int netvsc_connect_vsp(struct hv_device *device,
> >  			      struct netvsc_device *net_device,
> >  			      const struct netvsc_device_info *device_info)
> > @@ -579,12 +588,17 @@ static int netvsc_connect_vsp(struct hv_device
> > *device,
> >  	init_packet = &net_device->channel_init_pkt;
> > 
> >  	/* Negotiate the latest NVSP protocol supported */
> > -	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--)
> > +	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--) {
> > +		if (!nvsp_is_valid_version(ver_list[i])) {
> > +			ret = -EPROTO;
> > +			goto cleanup;
> > +		}
> 
> This code can catch the invalid, but cannot get the current host nvsp version.
> I'd suggest move this check after version negotiation is done. So we can log what's
> the current host nvsp version, and why we fail it (the expected nvsp ver).

Mmh, invalid versions are not negotiated.  How about I simply add the
following logging right before the above 'ret = -EPROTO' say?

+			netdev_err(ndev, "Invalid NVSP version %x (expected >= %x): isolation supported\n",
+				   ver_list[i], NVSP_PROTOCOL_VERSION_61);

(or something along these lines)


> > @@ -1357,7 +1371,8 @@ static void netvsc_receive_inband(struct
> > net_device *ndev,
> >  		break;
> > 
> >  	case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
> > -		netvsc_send_vf(ndev, nvmsg, msglen);
> > +		if (!hv_is_isolation_supported())
> > +			netvsc_send_vf(ndev, nvmsg, msglen);
> 
> When the driver doesn't advertise SRIOV, this message is not expected.
> Instead of ignore silently, we should log an error.

I've appended:

+		else
+			netdev_err(ndev, "Unexpected VF message: isolation supported\n");

Please let me know if I got this wrong.

Thanks,
  Andrea

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

* RE: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests
  2021-01-21  4:05     ` Andrea Parri
@ 2021-01-21 16:02       ` Haiyang Zhang
  2021-01-21 16:53         ` Andrea Parri
  0 siblings, 1 reply; 7+ messages in thread
From: Haiyang Zhang @ 2021-01-21 16:02 UTC (permalink / raw)
  To: Andrea Parri
  Cc: linux-kernel@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
	Wei Liu, Michael Kelley, Tianyu Lan, Saruhan Karademir,
	Juan Vazquez, linux-hyperv@vger.kernel.org, David S. Miller,
	Jakub Kicinski, netdev@vger.kernel.org



> -----Original Message-----
> From: Andrea Parri <parri.andrea@gmail.com>
> Sent: Wednesday, January 20, 2021 11:05 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
> Stephen Hemminger <sthemmin@microsoft.com>; Wei Liu
> <wei.liu@kernel.org>; Michael Kelley <mikelley@microsoft.com>; Tianyu Lan
> <Tianyu.Lan@microsoft.com>; Saruhan Karademir
> <skarade@microsoft.com>; Juan Vazquez <juvazq@microsoft.com>; linux-
> hyperv@vger.kernel.org; David S. Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; netdev@vger.kernel.org
> Subject: Re: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated
> guests
> 
> > > @@ -544,7 +545,8 @@ static int negotiate_nvsp_ver(struct hv_device
> > > *device,
> > >  	init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q = 1;
> > >
> > >  	if (nvsp_ver >= NVSP_PROTOCOL_VERSION_5) {
> > > -		init_packet->msg.v2_msg.send_ndis_config.capability.sriov =
> > > 1;
> > > +		if (!hv_is_isolation_supported())
> > > +			init_packet-
> > > >msg.v2_msg.send_ndis_config.capability.sriov = 1;
> >
> > Please also add a log there stating we don't support sriov in this case.
> Otherwise,
> > customers will ask why vf not showing up.
> 
> IIUC, you're suggesting that I append something like:
> 
> +		else
> +			netdev_info(ndev, "SR-IOV not advertised: isolation
> supported\n");
> 
> I've added this locally; please let me know if you had something else
> /better in mind.

This message explains the failure reason better:
  "SR-IOV not advertised by guests on the host supporting isolation"

> 
> 
> > > @@ -563,6 +565,13 @@ static int negotiate_nvsp_ver(struct hv_device
> > > *device,
> > >  	return ret;
> > >  }
> > >
> > > +static bool nvsp_is_valid_version(u32 version)
> > > +{
> > > +       if (hv_is_isolation_supported())
> > > +               return version >= NVSP_PROTOCOL_VERSION_61;
> > > +       return true;
> > Hosts support isolation should run nvsp 6.1+. This error is not expected.
> > Instead of fail silently, we should log an error to explain why it's failed, and
> the current version and expected version.
> 
> Please see my next comment below.
> 
> 
> > > +}
> > > +
> > >  static int netvsc_connect_vsp(struct hv_device *device,
> > >  			      struct netvsc_device *net_device,
> > >  			      const struct netvsc_device_info *device_info)
> > > @@ -579,12 +588,17 @@ static int netvsc_connect_vsp(struct hv_device
> > > *device,
> > >  	init_packet = &net_device->channel_init_pkt;
> > >
> > >  	/* Negotiate the latest NVSP protocol supported */
> > > -	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--)
> > > +	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--) {
> > > +		if (!nvsp_is_valid_version(ver_list[i])) {
> > > +			ret = -EPROTO;
> > > +			goto cleanup;
> > > +		}
> >
> > This code can catch the invalid, but cannot get the current host nvsp
> version.
> > I'd suggest move this check after version negotiation is done. So we can log
> what's
> > the current host nvsp version, and why we fail it (the expected nvsp ver).
> 
> Mmh, invalid versions are not negotiated.  How about I simply add the
> following logging right before the above 'ret = -EPROTO' say?
> 
> +			netdev_err(ndev, "Invalid NVSP version %x
> (expected >= %x): isolation supported\n",
> +				   ver_list[i], NVSP_PROTOCOL_VERSION_61);
> 
> (or something along these lines)

The negotiation process runs from the latest to oldest. If the host is 5, your code 
will fail before trying v6.0, and log:
	"Invalid NVSP version 60000  (expected >= 60001): isolation supported"
This will make user think the NVSP version is 6.0.

Since you will let the NIC fail and cleanup, there is no harm to check the version 
after negotiation. And this case is unexpected from a "normal" host. So I suggest 
move the check after negotiation is done, then we know the actual host nvsp 
version that causing this issue. And we can bring the accurate info to host team 
for better diagnosability.

Please point out this invalid version is caused by the host side, like this:
	"Invalid NVSP version 0x50000  (expected >= 0x60001) from the host with isolation support"
Also please use "0x%x" for hexadecimal numbers.

> 
> 
> > > @@ -1357,7 +1371,8 @@ static void netvsc_receive_inband(struct
> > > net_device *ndev,
> > >  		break;
> > >
> > >  	case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
> > > -		netvsc_send_vf(ndev, nvmsg, msglen);
> > > +		if (!hv_is_isolation_supported())
> > > +			netvsc_send_vf(ndev, nvmsg, msglen);
> >
> > When the driver doesn't advertise SRIOV, this message is not expected.
> > Instead of ignore silently, we should log an error.
> 
> I've appended:
> 
> +		else
> +			netdev_err(ndev, "Unexpected VF message:
> isolation supported\n");

Please log the msg type:
  "Ignore VF_ASSOCIATION msg from the host supporting isolation"

Thanks,
- Haiyang

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

* Re: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests
  2021-01-21 16:02       ` Haiyang Zhang
@ 2021-01-21 16:53         ` Andrea Parri
  0 siblings, 0 replies; 7+ messages in thread
From: Andrea Parri @ 2021-01-21 16:53 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: linux-kernel@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
	Wei Liu, Michael Kelley, Tianyu Lan, Saruhan Karademir,
	Juan Vazquez, linux-hyperv@vger.kernel.org, David S. Miller,
	Jakub Kicinski, netdev@vger.kernel.org

> > > > @@ -544,7 +545,8 @@ static int negotiate_nvsp_ver(struct hv_device
> > > > *device,
> > > >  	init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q = 1;
> > > >
> > > >  	if (nvsp_ver >= NVSP_PROTOCOL_VERSION_5) {
> > > > -		init_packet->msg.v2_msg.send_ndis_config.capability.sriov =
> > > > 1;
> > > > +		if (!hv_is_isolation_supported())
> > > > +			init_packet-
> > > > >msg.v2_msg.send_ndis_config.capability.sriov = 1;
> > >
> > > Please also add a log there stating we don't support sriov in this case.
> > Otherwise,
> > > customers will ask why vf not showing up.
> > 
> > IIUC, you're suggesting that I append something like:
> > 
> > +		else
> > +			netdev_info(ndev, "SR-IOV not advertised: isolation
> > supported\n");
> > 
> > I've added this locally; please let me know if you had something else
> > /better in mind.
> 
> This message explains the failure reason better:
>   "SR-IOV not advertised by guests on the host supporting isolation"

Applied.


> > > > @@ -579,12 +588,17 @@ static int netvsc_connect_vsp(struct hv_device
> > > > *device,
> > > >  	init_packet = &net_device->channel_init_pkt;
> > > >
> > > >  	/* Negotiate the latest NVSP protocol supported */
> > > > -	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--)
> > > > +	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--) {
> > > > +		if (!nvsp_is_valid_version(ver_list[i])) {
> > > > +			ret = -EPROTO;
> > > > +			goto cleanup;
> > > > +		}
> > >
> > > This code can catch the invalid, but cannot get the current host nvsp
> > version.
> > > I'd suggest move this check after version negotiation is done. So we can log
> > what's
> > > the current host nvsp version, and why we fail it (the expected nvsp ver).
> > 
> > Mmh, invalid versions are not negotiated.  How about I simply add the
> > following logging right before the above 'ret = -EPROTO' say?
> > 
> > +			netdev_err(ndev, "Invalid NVSP version %x
> > (expected >= %x): isolation supported\n",
> > +				   ver_list[i], NVSP_PROTOCOL_VERSION_61);
> > 
> > (or something along these lines)
> 
> The negotiation process runs from the latest to oldest. If the host is 5, your code 
> will fail before trying v6.0, and log:
> 	"Invalid NVSP version 60000  (expected >= 60001): isolation supported"
> This will make user think the NVSP version is 6.0.
> 
> Since you will let the NIC fail and cleanup, there is no harm to check the version 
> after negotiation. And this case is unexpected from a "normal" host. So I suggest 
> move the check after negotiation is done, then we know the actual host nvsp 
> version that causing this issue. And we can bring the accurate info to host team 
> for better diagnosability.

Fair enough, will do.


> Please point out this invalid version is caused by the host side, like this:
> 	"Invalid NVSP version 0x50000  (expected >= 0x60001) from the host with isolation support"
> Also please use "0x%x" for hexadecimal numbers.

Sure.


> > > > @@ -1357,7 +1371,8 @@ static void netvsc_receive_inband(struct
> > > > net_device *ndev,
> > > >  		break;
> > > >
> > > >  	case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
> > > > -		netvsc_send_vf(ndev, nvmsg, msglen);
> > > > +		if (!hv_is_isolation_supported())
> > > > +			netvsc_send_vf(ndev, nvmsg, msglen);
> > >
> > > When the driver doesn't advertise SRIOV, this message is not expected.
> > > Instead of ignore silently, we should log an error.
> > 
> > I've appended:
> > 
> > +		else
> > +			netdev_err(ndev, "Unexpected VF message:
> > isolation supported\n");
> 
> Please log the msg type:
>   "Ignore VF_ASSOCIATION msg from the host supporting isolation"

Applied.

Thanks,
  Andrea

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

end of thread, other threads:[~2021-01-21 16:55 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-19 17:58 [PATCH 0/4] Drivers: hv: vmbus: Restrict devices and configurations on 'isolated' guests Andrea Parri (Microsoft)
2021-01-19 17:58 ` [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests Andrea Parri (Microsoft)
2021-01-20 19:24   ` Haiyang Zhang
2021-01-21  4:05     ` Andrea Parri
2021-01-21 16:02       ` Haiyang Zhang
2021-01-21 16:53         ` Andrea Parri
2021-01-21  1:26   ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).