Hi Michael,
On Thu, Apr 30, 2020 at 12:06:09AM +0000, Michael Kelley wrote:
> From: Nathan Chancellor <natechancellor@xxxxxxxxx> 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);
}
