netdev - RE: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <PH7PR21MB311602D700C0FD965AF792F6CAD99@PH7PR21MB3116.namprd21.prod.outlook.com>
Date:   Thu, 9 Feb 2023 19:10:16 +0000
From:   Haiyang Zhang <haiyangz@...rosoft.com>
To:     "Michael Kelley (LINUX)" <mikelley@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT
 completion message



> -----Original Message-----
> From: Michael Kelley (LINUX) <mikelley@...rosoft.com>
> Sent: Thursday, February 9, 2023 12:11 PM
> To: Haiyang Zhang <haiyangz@...rosoft.com>; KY Srinivasan
> <kys@...rosoft.com>; wei.liu@...nel.org; Dexuan Cui
> <decui@...rosoft.com>; davem@...emloft.net; edumazet@...gle.com;
> kuba@...nel.org; pabeni@...hat.com; netdev@...r.kernel.org; linux-
> hyperv@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: RE: [PATCH net-next 1/1] hv_netvsc: Check status in
> SEND_RNDIS_PKT completion message
> 
> From: Haiyang Zhang <haiyangz@...rosoft.com> Sent: Thursday, February 9,
> 2023 5:49 AM
> >
> > > -----Original Message-----
> > > From: Michael Kelley (LINUX) <mikelley@...rosoft.com>
> > > Sent: Wednesday, February 8, 2023 6:50 PM
> > > To: KY Srinivasan <kys@...rosoft.com>; Haiyang Zhang
> > > <haiyangz@...rosoft.com>; wei.liu@...nel.org; Dexuan Cui
> > > <decui@...rosoft.com>; davem@...emloft.net; edumazet@...gle.com;
> > > kuba@...nel.org; pabeni@...hat.com; netdev@...r.kernel.org; linux-
> > > hyperv@...r.kernel.org; linux-kernel@...r.kernel.org
> > > Cc: Michael Kelley (LINUX) <mikelley@...rosoft.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@...rosoft.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