[PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT completion message

[PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT completion message

[Michael Kelley]

Completion responses to SEND_RNDIS_PKT messages are currently processed
regardless of the status in the response, so that resources associated
with the request are freed.  While this is appropriate, code bugs that
cause sending a malformed message, or errors on the Hyper-V host, go
undetected. Fix this by checking the status and outputting a message
if there is an error.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 drivers/net/hyperv/netvsc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 661bbe6..caf22e9 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -813,6 +813,7 @@ static void netvsc_send_completion(struct net_device *ndev,
 	u32 msglen = hv_pkt_datalen(desc);
 	struct nvsp_message *pkt_rqst;
 	u64 cmd_rqst;
+	u32 status;
 
 	/* First check if this is a VMBUS completion without data payload */
 	if (!msglen) {
@@ -884,6 +885,22 @@ static void netvsc_send_completion(struct net_device *ndev,
 		break;
 
 	case NVSP_MSG1_TYPE_SEND_RNDIS_PKT_COMPLETE:
+		if (msglen < sizeof(struct nvsp_message_header) +
+		    sizeof(struct nvsp_1_message_send_rndis_packet_complete)) {
+			netdev_err(ndev, "nvsp_rndis_pkt_complete length too small: %u\n",
+				   msglen);
+			return;
+		}
+
+		/* If status indicates an error, output a message so we know
+		 * there's a problem. But process the completion anyway so the
+		 * resources are released.
+		 */
+		status = nvsp_packet->msg.v1_msg.send_rndis_pkt_complete.status;
+		if (status != NVSP_STAT_SUCCESS)
+			netdev_err(ndev, "nvsp_rndis_pkt_complete error status: %x\n",
+				   status);
+
 		netvsc_send_tx_complete(ndev, net_device, incoming_channel,
 					desc, budget);
 		break;
-- 
1.8.3.1

RE: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT completion message

[Haiyang Zhang]

> -----Original Message-----
> From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> Sent: Wednesday, February 8, 2023 6:50 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-
> hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Michael Kelley (LINUX) <mikelley@microsoft.com>
> Subject: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT
> completion message
> 
> Completion responses to SEND_RNDIS_PKT messages are currently processed
> regardless of the status in the response, so that resources associated
> with the request are freed.  While this is appropriate, code bugs that
> cause sending a malformed message, or errors on the Hyper-V host, go
> undetected. Fix this by checking the status and outputting a message
> if there is an error.
> 
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  drivers/net/hyperv/netvsc.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 661bbe6..caf22e9 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -813,6 +813,7 @@ static void netvsc_send_completion(struct net_device
> *ndev,
>  	u32 msglen = hv_pkt_datalen(desc);
>  	struct nvsp_message *pkt_rqst;
>  	u64 cmd_rqst;
> +	u32 status;
> 
>  	/* First check if this is a VMBUS completion without data payload */
>  	if (!msglen) {
> @@ -884,6 +885,22 @@ static void netvsc_send_completion(struct
> net_device *ndev,
>  		break;
> 
>  	case NVSP_MSG1_TYPE_SEND_RNDIS_PKT_COMPLETE:
> +		if (msglen < sizeof(struct nvsp_message_header) +
> +		    sizeof(struct
> nvsp_1_message_send_rndis_packet_complete)) {
> +			netdev_err(ndev, "nvsp_rndis_pkt_complete length
> too small: %u\n",
> +				   msglen);
> +			return;
> +		}
> +
> +		/* If status indicates an error, output a message so we know
> +		 * there's a problem. But process the completion anyway so
> the
> +		 * resources are released.
> +		 */
> +		status = nvsp_packet-
> >msg.v1_msg.send_rndis_pkt_complete.status;
> +		if (status != NVSP_STAT_SUCCESS)
> +			netdev_err(ndev, "nvsp_rndis_pkt_complete error
> status: %x\n",
> +				   status);
> +

Could you add rate limit to this error, so in case it happens frequently, the 
errors won't fill up the dmesg.

Or even better, add a counter for this.

Thanks,
- Haiyang

RE: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT completion message

[Michael Kelley (LINUX)]

From: Haiyang Zhang <haiyangz@microsoft.com> Sent: Thursday, February 9, 2023 5:49 AM
> 
> > -----Original Message-----
> > From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> > Sent: Wednesday, February 8, 2023 6:50 PM
> > To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> > <decui@microsoft.com>; davem@davemloft.net; edumazet@google.com;
> > kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-
> > hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
> > Cc: Michael Kelley (LINUX) <mikelley@microsoft.com>
> > Subject: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT
> > completion message
> >
> > Completion responses to SEND_RNDIS_PKT messages are currently processed
> > regardless of the status in the response, so that resources associated
> > with the request are freed.  While this is appropriate, code bugs that
> > cause sending a malformed message, or errors on the Hyper-V host, go
> > undetected. Fix this by checking the status and outputting a message
> > if there is an error.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > ---
> >  drivers/net/hyperv/netvsc.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> > index 661bbe6..caf22e9 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -813,6 +813,7 @@ static void netvsc_send_completion(struct net_device *ndev,
> >  	u32 msglen = hv_pkt_datalen(desc);
> >  	struct nvsp_message *pkt_rqst;
> >  	u64 cmd_rqst;
> > +	u32 status;
> >
> >  	/* First check if this is a VMBUS completion without data payload */
> >  	if (!msglen) {
> > @@ -884,6 +885,22 @@ static void netvsc_send_completion(struct net_device *ndev,
> >  		break;
> >
> >  	case NVSP_MSG1_TYPE_SEND_RNDIS_PKT_COMPLETE:
> > +		if (msglen < sizeof(struct nvsp_message_header) +
> > +		    sizeof(struct nvsp_1_message_send_rndis_packet_complete)) {
> > +			netdev_err(ndev, "nvsp_rndis_pkt_complete length too small: %u\n",
> > +				   msglen);
> > +			return;
> > +		}
> > +
> > +		/* If status indicates an error, output a message so we know
> > +		 * there's a problem. But process the completion anyway so the
> > +		 * resources are released.
> > +		 */
> > +		status = nvsp_packet->msg.v1_msg.send_rndis_pkt_complete.status;
> > +		if (status != NVSP_STAT_SUCCESS)
> > +			netdev_err(ndev, "nvsp_rndis_pkt_complete error status: %x\n",
> > +				   status);
> > +
> 
> Could you add rate limit to this error, so in case it happens frequently, the
> errors won't fill up the dmesg.
> 
> Or even better, add a counter for this.

I thought about rate limiting.  But my assumption is that such errors are
very rare, and that it would be better to see all occurrences instead of
potentially filtering some out due to rate limiting.  If that assumption
proves to not be true, then we probably have a bigger problem -- there's
a bug in the Linux guest causing it to submit bad requests, or there's a
bug on the Hyper-V side.

That said, I don't feel strongly about it either way. 

Thoughts?

Michael

RE: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT completion message

[Haiyang Zhang]

> -----Original Message-----
> From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> Sent: Thursday, February 9, 2023 12:11 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>; KY Srinivasan
> <kys@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-
> hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH net-next 1/1] hv_netvsc: Check status in
> SEND_RNDIS_PKT completion message
> 
> From: Haiyang Zhang <haiyangz@microsoft.com> Sent: Thursday, February 9,
> 2023 5:49 AM
> >
> > > -----Original Message-----
> > > From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> > > Sent: Wednesday, February 8, 2023 6:50 PM
> > > To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > > <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> > > <decui@microsoft.com>; davem@davemloft.net; edumazet@google.com;
> > > kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-
> > > hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Cc: Michael Kelley (LINUX) <mikelley@microsoft.com>
> > > Subject: [PATCH net-next 1/1] hv_netvsc: Check status in
> SEND_RNDIS_PKT
> > > completion message
> > >
> > > Completion responses to SEND_RNDIS_PKT messages are currently
> processed
> > > regardless of the status in the response, so that resources associated
> > > with the request are freed.  While this is appropriate, code bugs that
> > > cause sending a malformed message, or errors on the Hyper-V host, go
> > > undetected. Fix this by checking the status and outputting a message
> > > if there is an error.
> > >
> > > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > > ---
> > >  drivers/net/hyperv/netvsc.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > >
> > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> > > index 661bbe6..caf22e9 100644
> > > --- a/drivers/net/hyperv/netvsc.c
> > > +++ b/drivers/net/hyperv/netvsc.c
> > > @@ -813,6 +813,7 @@ static void netvsc_send_completion(struct
> net_device *ndev,
> > >  	u32 msglen = hv_pkt_datalen(desc);
> > >  	struct nvsp_message *pkt_rqst;
> > >  	u64 cmd_rqst;
> > > +	u32 status;
> > >
> > >  	/* First check if this is a VMBUS completion without data payload */
> > >  	if (!msglen) {
> > > @@ -884,6 +885,22 @@ static void netvsc_send_completion(struct
> net_device *ndev,
> > >  		break;
> > >
> > >  	case NVSP_MSG1_TYPE_SEND_RNDIS_PKT_COMPLETE:
> > > +		if (msglen < sizeof(struct nvsp_message_header) +
> > > +		    sizeof(struct
> nvsp_1_message_send_rndis_packet_complete)) {
> > > +			netdev_err(ndev, "nvsp_rndis_pkt_complete length
> too small: %u\n",
> > > +				   msglen);
> > > +			return;
> > > +		}
> > > +
> > > +		/* If status indicates an error, output a message so we know
> > > +		 * there's a problem. But process the completion anyway so
> the
> > > +		 * resources are released.
> > > +		 */
> > > +		status = nvsp_packet-
> >msg.v1_msg.send_rndis_pkt_complete.status;
> > > +		if (status != NVSP_STAT_SUCCESS)
> > > +			netdev_err(ndev, "nvsp_rndis_pkt_complete error
> status: %x\n",
> > > +				   status);
> > > +
> >
> > Could you add rate limit to this error, so in case it happens frequently, the
> > errors won't fill up the dmesg.
> >
> > Or even better, add a counter for this.
> 
> I thought about rate limiting.  But my assumption is that such errors are
> very rare, and that it would be better to see all occurrences instead of
> potentially filtering some out due to rate limiting.  If that assumption
> proves to not be true, then we probably have a bigger problem -- there's
> a bug in the Linux guest causing it to submit bad requests, or there's a
> bug on the Hyper-V side.
> 
> That said, I don't feel strongly about it either way.
> 
> Thoughts?

I haven't seen any cases of large amount of TX errors so far (Our 
existing code doesn't check it).

But I'm just worried about if a VM sending at high speed, and host side is,
for some reason, not able to send them correctly, the log file will become 
really big and difficult to download and read. With rate limit, we still see 
dozens of messages every 5 seconds or so, and it tells you how many 
messages are skipped. And, if the rate is lower, it won't skip anything. 
Isn't this info sufficient to debug?

By the way, guests cannot trust the host -- probably we shouldn't allow the
host to have a way to jam guest's log file?

Thanks,
- Haiyang

Re: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT completion message

[Jakub Kicinski]

On Thu, 9 Feb 2023 19:10:16 +0000 Haiyang Zhang wrote:
> But I'm just worried about if a VM sending at high speed, and host side is,
> for some reason, not able to send them correctly, the log file will become 
> really big and difficult to download and read. With rate limit, we still see 
> dozens of messages every 5 seconds or so, and it tells you how many 
> messages are skipped. And, if the rate is lower, it won't skip anything. 
> Isn't this info sufficient to debug?
> 
> By the way, guests cannot trust the host -- probably we shouldn't allow the
> host to have a way to jam guest's log file?

+1 FWIW, the general guidance is to always rate limit prints
which may be triggered from the datapath (which I'm guessing
this is based on the names of things)

RE: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT completion message

[Michael Kelley (LINUX)]

From: Jakub Kicinski <kuba@kernel.org> Sent: Thursday, February 9, 2023 12:22 PM
> 
> On Thu, 9 Feb 2023 19:10:16 +0000 Haiyang Zhang wrote:
> > But I'm just worried about if a VM sending at high speed, and host side is,
> > for some reason, not able to send them correctly, the log file will become
> > really big and difficult to download and read. With rate limit, we still see
> > dozens of messages every 5 seconds or so, and it tells you how many
> > messages are skipped. And, if the rate is lower, it won't skip anything.
> > Isn't this info sufficient to debug?

Agreed.

> >
> > By the way, guests cannot trust the host -- probably we shouldn't allow the
> > host to have a way to jam guest's log file?

Actually, preventing jamming the guest's log file is not a requirement
in Confidential VMs where the host is not trusted.  Confidential VMs
do not prevent denial-of-service attacks, or similar.  But that's another
topic. :-)

> 
> +1 FWIW, the general guidance is to always rate limit prints
> which may be triggered from the datapath (which I'm guessing
> this is based on the names of things)

Fair enough.  I'll do a v2 with the rate limiting.

Michael