[PATCH net v4 0/2] rtnetlink: allow to enslave with one msg an up interface

[PATCH net v4 0/2] rtnetlink: allow to enslave with one msg an up interface

[Nicolas Dichtel]

The first patch fixes a regression, introduced in linux v6.1, by reverting
a patch. The second patch adds a test to verify this API.

v3 -> v4:
 - replace patch #1 by a revert of the original patch
 - patch #2: keep only one test

v2 -> v3:
 - reword a failure message in patch #2

v1 -> v2:
 - add the #2 patch

 net/core/rtnetlink.c                     | 14 +++++++-------
 tools/testing/selftests/net/rtnetlink.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 7 deletions(-)

Regards,
Nicolas

[PATCH net v4 1/2] Revert "net: rtnetlink: Enslave device before bringing it up"

[Nicolas Dichtel]

This reverts commit a4abfa627c3865c37e036bccb681619a50d3d93c.

The patch broke:
> ip link set dummy0 up
> ip link set dummy0 master bond0 down

This last command is useful to be able to enslave an interface with only
one netlink message.

After discussion, there is no good reason to support:
> ip link set dummy0 down
> ip link set dummy0 master bond0 up
because the bond interface already set the slave up when it is up.

Cc: stable@vger.kernel.org
Fixes: a4abfa627c38 ("net: rtnetlink: Enslave device before bringing it up")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/rtnetlink.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index e8431c6c8490..bf4c3f65ad99 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2905,13 +2905,6 @@ static int do_setlink(const struct sk_buff *skb,
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
 	}
 
-	if (tb[IFLA_MASTER]) {
-		err = do_set_master(dev, nla_get_u32(tb[IFLA_MASTER]), extack);
-		if (err)
-			goto errout;
-		status |= DO_SETLINK_MODIFIED;
-	}
-
 	if (ifm->ifi_flags || ifm->ifi_change) {
 		err = dev_change_flags(dev, rtnl_dev_combine_flags(dev, ifm),
 				       extack);
@@ -2919,6 +2912,13 @@ static int do_setlink(const struct sk_buff *skb,
 			goto errout;
 	}
 
+	if (tb[IFLA_MASTER]) {
+		err = do_set_master(dev, nla_get_u32(tb[IFLA_MASTER]), extack);
+		if (err)
+			goto errout;
+		status |= DO_SETLINK_MODIFIED;
+	}
+
 	if (tb[IFLA_CARRIER]) {
 		err = dev_change_carrier(dev, nla_get_u8(tb[IFLA_CARRIER]));
 		if (err)
-- 
2.39.2

Re: [PATCH net v4 1/2] Revert "net: rtnetlink: Enslave device before bringing it up"

[Jiri Pirko]

Mon, Jan 08, 2024 at 10:41:02AM CET, nicolas.dichtel@6wind.com wrote:
>This reverts commit a4abfa627c3865c37e036bccb681619a50d3d93c.
>
>The patch broke:
>> ip link set dummy0 up
>> ip link set dummy0 master bond0 down
>
>This last command is useful to be able to enslave an interface with only
>one netlink message.
>
>After discussion, there is no good reason to support:
>> ip link set dummy0 down
>> ip link set dummy0 master bond0 up
>because the bond interface already set the slave up when it is up.
>
>Cc: stable@vger.kernel.org
>Fixes: a4abfa627c38 ("net: rtnetlink: Enslave device before bringing it up")
>Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Thanks!

Re: [PATCH net v4 1/2] Revert "net: rtnetlink: Enslave device before bringing it up"

[Hangbin Liu]

On Mon, Jan 08, 2024 at 10:41:02AM +0100, Nicolas Dichtel wrote:
> This reverts commit a4abfa627c3865c37e036bccb681619a50d3d93c.
> 
> The patch broke:
> > ip link set dummy0 up
> > ip link set dummy0 master bond0 down
> 
> This last command is useful to be able to enslave an interface with only
> one netlink message.
> 
> After discussion, there is no good reason to support:
> > ip link set dummy0 down
> > ip link set dummy0 master bond0 up
> because the bond interface already set the slave up when it is up.
> 
> Cc: stable@vger.kernel.org
> Fixes: a4abfa627c38 ("net: rtnetlink: Enslave device before bringing it up")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>

[PATCH net v4 2/2] selftests: rtnetlink: check enslaving iface in a bond

[Nicolas Dichtel]

The goal is to check the following two sequences:
> ip link set dummy0 up
> ip link set dummy0 master bond0 down

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 tools/testing/selftests/net/rtnetlink.sh | 28 ++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index 26827ea4e3e5..bbf9d2bd3d7b 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -28,6 +28,7 @@ ALL_TESTS="
 	kci_test_neigh_get
 	kci_test_bridge_parent_id
 	kci_test_address_proto
+	kci_test_enslave_bonding
 "
 
 devdummy="test-dummy0"
@@ -1239,6 +1240,33 @@ kci_test_address_proto()
 	return $ret
 }
 
+kci_test_enslave_bonding()
+{
+	local testns="testns"
+	local bond="bond123"
+	local dummy="dummy123"
+	local ret=0
+
+	run_cmd ip netns add "$testns"
+	if [ $ret -ne 0 ]; then
+		end_test "SKIP bonding tests: cannot add net namespace $testns"
+		return $ksft_skip
+	fi
+
+	run_cmd ip -netns $testns link add dev $bond type bond mode balance-rr
+	run_cmd ip -netns $testns link add dev $dummy type dummy
+	run_cmd ip -netns $testns link set dev $dummy up
+	run_cmd ip -netns $testns link set dev $dummy master $bond down
+	if [ $ret -ne 0 ]; then
+		end_test "FAIL: initially up interface added to a bond and set down"
+		ip netns del "$testns"
+		return 1
+	fi
+
+	end_test "PASS: enslave interface in a bond"
+	ip netns del "$testns"
+}
+
 kci_test_rtnl()
 {
 	local current_test
-- 
2.39.2

Re: [PATCH net v4 2/2] selftests: rtnetlink: check enslaving iface in a bond

[Jiri Pirko]

Mon, Jan 08, 2024 at 10:41:03AM CET, nicolas.dichtel@6wind.com wrote:
>The goal is to check the following two sequences:
>> ip link set dummy0 up
>> ip link set dummy0 master bond0 down
>
>Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH net v4 2/2] selftests: rtnetlink: check enslaving iface in a bond

[Hangbin Liu]

On Mon, Jan 08, 2024 at 10:41:03AM +0100, Nicolas Dichtel wrote:
> The goal is to check the following two sequences:
> > ip link set dummy0 up
> > ip link set dummy0 master bond0 down
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  tools/testing/selftests/net/rtnetlink.sh | 28 ++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
> index 26827ea4e3e5..bbf9d2bd3d7b 100755
> --- a/tools/testing/selftests/net/rtnetlink.sh
> +++ b/tools/testing/selftests/net/rtnetlink.sh
> @@ -28,6 +28,7 @@ ALL_TESTS="
>  	kci_test_neigh_get
>  	kci_test_bridge_parent_id
>  	kci_test_address_proto
> +	kci_test_enslave_bonding
>  "
>  
>  devdummy="test-dummy0"
> @@ -1239,6 +1240,33 @@ kci_test_address_proto()
>  	return $ret
>  }
>  
> +kci_test_enslave_bonding()
> +{
> +	local testns="testns"
> +	local bond="bond123"
> +	local dummy="dummy123"
> +	local ret=0
> +
> +	run_cmd ip netns add "$testns"
> +	if [ $ret -ne 0 ]; then
> +		end_test "SKIP bonding tests: cannot add net namespace $testns"
> +		return $ksft_skip
> +	fi
> +
> +	run_cmd ip -netns $testns link add dev $bond type bond mode balance-rr

Hi Nicolas,

FYI, the selftests/net/lib.sh has been merged to net tree. Please remember
send a following up update to create the netns with setup_ns.

Thanks
Hangbin

Re: [PATCH net v4 2/2] selftests: rtnetlink: check enslaving iface in a bond

[Nicolas Dichtel]

Le 12/01/2024 à 02:15, Hangbin Liu a écrit :
[snip]
> Hi Nicolas,
> 
> FYI, the selftests/net/lib.sh has been merged to net tree. Please remember
> send a following up update to create the netns with setup_ns.
Please be patient and don't worry.
I said I will send an update, and thus I will send an update.


Regards,
Nicolas

Re: [PATCH net v4 2/2] selftests: rtnetlink: check enslaving iface in a bond

[Hangbin Liu]

On Fri, Jan 12, 2024 at 09:06:56AM +0100, Nicolas Dichtel wrote:
> Le 12/01/2024 à 02:15, Hangbin Liu a écrit :
> [snip]
> > Hi Nicolas,
> > 
> > FYI, the selftests/net/lib.sh has been merged to net tree. Please remember
> > send a following up update to create the netns with setup_ns.
> Please be patient and don't worry.
> I said I will send an update, and thus I will send an update.

It's just a reminder. Not pushing you. Sorry if this makes you feel disturbed.

Regards
Hangbin

Re: [PATCH net v4 0/2] rtnetlink: allow to enslave with one msg an up interface

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  8 Jan 2024 10:41:01 +0100 you wrote:
> The first patch fixes a regression, introduced in linux v6.1, by reverting
> a patch. The second patch adds a test to verify this API.
> 
> v3 -> v4:
>  - replace patch #1 by a revert of the original patch
>  - patch #2: keep only one test
> 
> [...]

Here is the summary with links:
  - [net,v4,1/2] Revert "net: rtnetlink: Enslave device before bringing it up"
    https://git.kernel.org/netdev/net/c/ec4ffd100ffb
  - [net,v4,2/2] selftests: rtnetlink: check enslaving iface in a bond
    https://git.kernel.org/netdev/net/c/a159cbe81d3b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html