Re: [PATCH v2] hv_netvsc: Fix netvsc_start_xmit's return type

[Nathan Chancellor <natechancellor@gmail.com>]

Hi Michael,

On Thu, Apr 30, 2020 at 12:06:09AM +0000, Michael Kelley wrote:
> From: Nathan Chancellor <natechancellor@gmail.com> Sent: Tuesday, April 28, 2020 10:55 AM
> > 
> > Do note that netvsc_xmit still returns int because netvsc_xmit has a
> > potential return from netvsc_vf_xmit, which does not return netdev_tx_t
> > because of the call to dev_queue_xmit.
> > 
> > I am not sure if that is an oversight that was introduced by
> > commit 0c195567a8f6e ("netvsc: transparent VF management") or if
> > everything works properly as it is now.
> > 
> > My patch is purely concerned with making the definition match the
> > prototype so it should be NFC aside from avoiding the CFI panic.
> > 
> 
> While it probably works correctly now, I'm not too keen on just
> changing the return type for netvsc_start_xmit() and assuming the
> 'int' that is returned from netvsc_xmit() will be correctly mapped to
> the netdev_tx_t enum type.  While that mapping probably happens
> correctly at the moment, this really should have a more holistic fix.

While it might work correctly, I am not sure that the mapping is
correct, hence that comment.

netdev_tx_t is an enum with two acceptable types, 0x00 and 0x10. Up
until commit 0c195567a8f6e ("netvsc: transparent VF management"),
netvsc_xmit was guaranteed to return something of type netdev_tx_t.

However, after said commit, we could return anything from
netvsc_vf_xmit, which in turn calls dev_queue_xmit then
__dev_queue_xmit which will return either an error code (-ENOMEM or
-ENETDOWN) or something from __dev_xmit_skb, which appears to be
NET_XMIT_SUCCESS, NET_XMIT_DROP, or NET_XMIT_CN.

It does not look like netvsc_xmit or netvsc_vf_xmit try to convert those
returns to netdev_tx_t in some way; netvsc_vf_xmit just passes the
return value up to netvsc_xmit, which is the part that I am unsure
about...

> Nathan -- are you willing to look at doing the more holistic fix?  Or
> should we see about asking Haiyang Zhang to do it?

I would be fine trying to look at a more holistic fix but I know
basically nothing about this subsystem. I am unsure if something like
this would be acceptable or if something else needs to happen.
Otherwise, I'd be fine with you guys taking a look and just giving me
reported-by credit.

Cheers,
Nathan

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d8e86bdbfba1e..a39480cfb8fa7 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -520,7 +520,8 @@ static int netvsc_vf_xmit(struct net_device *net, struct net_device *vf_netdev,
 	return rc;
 }
 
-static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
+static netdev_tx_t netvsc_xmit(struct sk_buff *skb, struct net_device *net,
+			       bool xdp_tx)
 {
 	struct net_device_context *net_device_ctx = netdev_priv(net);
 	struct hv_netvsc_packet *packet = NULL;
@@ -537,8 +538,11 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
 	 */
 	vf_netdev = rcu_dereference_bh(net_device_ctx->vf_netdev);
 	if (vf_netdev && netif_running(vf_netdev) &&
-	    !netpoll_tx_running(net))
-		return netvsc_vf_xmit(net, vf_netdev, skb);
+	    !netpoll_tx_running(net)) {
+		if (!netvsc_vf_xmit(net, vf_netdev, skb))
+			return NETDEV_TX_OK;
+		goto drop;
+	}
 
 	/* We will atmost need two pages to describe the rndis
 	 * header. We can only transmit MAX_PAGE_BUFFER_COUNT number
@@ -707,7 +711,8 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
 	goto drop;
 }
 
-static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
+				     struct net_device *ndev)
 {
 	return netvsc_xmit(skb, ndev, false);
 }