[PATCH net 1/4] ieee802154: Restore initial state on failed device_rename() in cfg802154_switch_netns()

Ivan Abramov posted 4 patches 8 months, 3 weeks ago
Only 3 patches received!
[PATCH net 1/4] ieee802154: Restore initial state on failed device_rename() in cfg802154_switch_netns()
Posted by Ivan Abramov 8 months, 3 weeks ago
Currently, the return value of device_rename() is not checked or acted
upon. There is also a pointless WARN_ON() call in case of an allocation
failure, since it only leads to useless splats caused by deliberate fault
injections.

Since it's possible to roll back the changes made before the
device_rename() call in case of failure, do it.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Fixes: 66e5c2672cd1 ("ieee802154: add netns support")
Signed-off-by: Ivan Abramov <i.abramov@mt-integration.ru>
---
 net/ieee802154/core.c | 44 +++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
index 88adb04e4072..f9865eb2c7cf 100644
--- a/net/ieee802154/core.c
+++ b/net/ieee802154/core.c
@@ -233,31 +233,35 @@ int cfg802154_switch_netns(struct cfg802154_registered_device *rdev,
 		wpan_dev->netdev->netns_local = true;
 	}
 
-	if (err) {
-		/* failed -- clean up to old netns */
-		net = wpan_phy_net(&rdev->wpan_phy);
-
-		list_for_each_entry_continue_reverse(wpan_dev,
-						     &rdev->wpan_dev_list,
-						     list) {
-			if (!wpan_dev->netdev)
-				continue;
-			wpan_dev->netdev->netns_local = false;
-			err = dev_change_net_namespace(wpan_dev->netdev, net,
-						       "wpan%d");
-			WARN_ON(err);
-			wpan_dev->netdev->netns_local = true;
-		}
+	if (err)
+		goto errout;
 
-		return err;
-	}
+	err = device_rename(&rdev->wpan_phy.dev, dev_name(&rdev->wpan_phy.dev));
 
-	wpan_phy_net_set(&rdev->wpan_phy, net);
+	if (err)
+		goto errout;
 
-	err = device_rename(&rdev->wpan_phy.dev, dev_name(&rdev->wpan_phy.dev));
-	WARN_ON(err);
+	wpan_phy_net_set(&rdev->wpan_phy, net);
 
 	return 0;
+
+errout:
+	/* failed -- clean up to old netns */
+	net = wpan_phy_net(&rdev->wpan_phy);
+
+	list_for_each_entry_continue_reverse(wpan_dev,
+					     &rdev->wpan_dev_list,
+					     list) {
+		if (!wpan_dev->netdev)
+			continue;
+		wpan_dev->netdev->netns_local = false;
+		err = dev_change_net_namespace(wpan_dev->netdev, net,
+					       "wpan%d");
+		WARN_ON(err);
+		wpan_dev->netdev->netns_local = true;
+	}
+
+	return err;
 }
 
 void cfg802154_dev_free(struct cfg802154_registered_device *rdev)
-- 
2.39.5
Re: [PATCH net 1/4] ieee802154: Restore initial state on failed device_rename() in cfg802154_switch_netns()
Posted by Kuniyuki Iwashima 8 months, 3 weeks ago
From: Ivan Abramov <i.abramov@mt-integration.ru>
Date: Tue, 25 Mar 2025 17:17:20 +0300
> Currently, the return value of device_rename() is not checked or acted
> upon. There is also a pointless WARN_ON() call in case of an allocation
> failure, since it only leads to useless splats caused by deliberate fault
> injections.
> 
> Since it's possible to roll back the changes made before the
> device_rename() call in case of failure, do it.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: 66e5c2672cd1 ("ieee802154: add netns support")
> Signed-off-by: Ivan Abramov <i.abramov@mt-integration.ru>
> ---
>  net/ieee802154/core.c | 44 +++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
> index 88adb04e4072..f9865eb2c7cf 100644
> --- a/net/ieee802154/core.c
> +++ b/net/ieee802154/core.c
> @@ -233,31 +233,35 @@ int cfg802154_switch_netns(struct cfg802154_registered_device *rdev,
>  		wpan_dev->netdev->netns_local = true;
>  	}
>  
> -	if (err) {
> -		/* failed -- clean up to old netns */
> -		net = wpan_phy_net(&rdev->wpan_phy);
> -
> -		list_for_each_entry_continue_reverse(wpan_dev,
> -						     &rdev->wpan_dev_list,
> -						     list) {
> -			if (!wpan_dev->netdev)
> -				continue;
> -			wpan_dev->netdev->netns_local = false;
> -			err = dev_change_net_namespace(wpan_dev->netdev, net,
> -						       "wpan%d");
> -			WARN_ON(err);
> -			wpan_dev->netdev->netns_local = true;
> -		}
> +	if (err)
> +		goto errout;
>  
> -		return err;
> -	}
> +	err = device_rename(&rdev->wpan_phy.dev, dev_name(&rdev->wpan_phy.dev));
>  
> -	wpan_phy_net_set(&rdev->wpan_phy, net);
> +	if (err)
> +		goto errout;
>  
> -	err = device_rename(&rdev->wpan_phy.dev, dev_name(&rdev->wpan_phy.dev));
> -	WARN_ON(err);
> +	wpan_phy_net_set(&rdev->wpan_phy, net);
>  
>  	return 0;
> +
> +errout:
> +	/* failed -- clean up to old netns */
> +	net = wpan_phy_net(&rdev->wpan_phy);
> +
> +	list_for_each_entry_continue_reverse(wpan_dev,
> +					     &rdev->wpan_dev_list,
> +					     list) {
> +		if (!wpan_dev->netdev)
> +			continue;
> +		wpan_dev->netdev->netns_local = false;
> +		err = dev_change_net_namespace(wpan_dev->netdev, net,
> +					       "wpan%d");
> +		WARN_ON(err);

It's still possible to trigger this with -ENOMEM.

For example, see bitmap_zalloc() in __dev_alloc_name().

Perhaps simply use pr_warn() or net_warn_ratelimited() as do_setlink().

I guess the stack trace from here is not so interesting as it doens't
show where it actually failed.


> +		wpan_dev->netdev->netns_local = true;
> +	}
> +
> +	return err;
>  }
>  
>  void cfg802154_dev_free(struct cfg802154_registered_device *rdev)
> -- 
> 2.39.5