[PATCH net, 0/2] hv_netvsc: fix error handling in netvsc_attach/set_features

[PATCH net, 0/2] hv_netvsc: fix error handling in netvsc_attach/set_features

[Haiyang Zhang]

The error handling code path in these functions are not correct.
This patch set fixes them.

Haiyang Zhang (2):
  hv_netvsc: Fix error handling in netvsc_set_features()
  hv_netvsc: Fix error handling in netvsc_attach()

 drivers/net/hyperv/netvsc_drv.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[PATCH net, 1/2] hv_netvsc: Fix error handling in netvsc_set_features()

[Haiyang Zhang]

When an error is returned by rndis_filter_set_offload_params(), we should
still assign the unaffected features to ndev->features. Otherwise, these
features will be missing.

Fixes: d6792a5a0747 ("hv_netvsc: Add handler for LRO setting change")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/net/hyperv/netvsc_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 39dddcd..734e411 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1807,8 +1807,10 @@ static int netvsc_set_features(struct net_device *ndev,
 
 	ret = rndis_filter_set_offload_params(ndev, nvdev, &offloads);
 
-	if (ret)
+	if (ret) {
 		features ^= NETIF_F_LRO;
+		ndev->features = features;
+	}
 
 syncvf:
 	if (!vf_netdev)
-- 
1.8.3.1

[PATCH net, 2/2] hv_netvsc: Fix error handling in netvsc_attach()

[Haiyang Zhang]

If rndis_filter_open() fails, we need to remove the rndis device created
in earlier steps, before returning an error code. Otherwise, the retry of
netvsc_attach() from its callers will fail and hang.

Fixes: 7b2ee50c0cd5 ("hv_netvsc: common detach logic")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/net/hyperv/netvsc_drv.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 734e411..a14fc8e 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -982,7 +982,7 @@ static int netvsc_attach(struct net_device *ndev,
 	if (netif_running(ndev)) {
 		ret = rndis_filter_open(nvdev);
 		if (ret)
-			return ret;
+			goto err;
 
 		rdev = nvdev->extension;
 		if (!rdev->link_state)
@@ -990,6 +990,13 @@ static int netvsc_attach(struct net_device *ndev,
 	}
 
 	return 0;
+
+err:
+	netif_device_detach(ndev);
+
+	rndis_filter_device_remove(hdev, nvdev);
+
+	return ret;
 }
 
 static int netvsc_set_channels(struct net_device *net,
-- 
1.8.3.1

Re: [PATCH net, 0/2] hv_netvsc: fix error handling in netvsc_attach/set_features

[David Miller]

From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Wed, 30 Oct 2019 15:32:05 +0000

> The error handling code path in these functions are not correct.
> This patch set fixes them.

Series applied, thank you.

Re: [PATCH net, 2/2] hv_netvsc: Fix error handling in netvsc_attach()

[Markus Elfring]

…
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -982,7 +982,7 @@ static int netvsc_attach(struct net_device *ndev,
>  	if (netif_running(ndev)) {
>  		ret = rndis_filter_open(nvdev);
>  		if (ret)
> -			return ret;
> +			goto err;
>
>  		rdev = nvdev->extension;
>  		if (!rdev->link_state)
…

I would prefer to specify the completed exception handling
(addition of two function calls) by a compound statement in
the shown if branch directly.

If you would insist to use a goto statement, I find an other label
more appropriate according to the Linux coding style.

Regards,
Markus