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

[PATCH net v3 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. The second
patch adds some tests to verify this API.

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

v1 -> v2:
 - add the #2 patch

 net/core/rtnetlink.c                     |  8 +++++++
 tools/testing/selftests/net/rtnetlink.sh | 39 ++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

Regards,
Nicolas

[PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it

[Nicolas Dichtel]

The below commit adds support for:
> ip link set dummy0 down
> ip link set dummy0 master bond0 up

but breaks the opposite:
> ip link set dummy0 up
> ip link set dummy0 master bond0 down

Let's add a workaround to have both commands working.

Cc: stable@vger.kernel.org
Fixes: a4abfa627c38 ("net: rtnetlink: Enslave device before bringing it up")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Acked-by: Phil Sutter <phil@nwl.cc>
Reviewed-by: David Ahern <dsahern@kernel.org>
---
 net/core/rtnetlink.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index e8431c6c8490..dd79693c2d91 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2905,6 +2905,14 @@ static int do_setlink(const struct sk_buff *skb,
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
 	}
 
+	/* Backward compat: enable to set interface down before enslaving it */
+	if (!(ifm->ifi_flags & IFF_UP) && ifm->ifi_change & IFF_UP) {
+		err = dev_change_flags(dev, rtnl_dev_combine_flags(dev, ifm),
+				       extack);
+		if (err < 0)
+			goto errout;
+	}
+
 	if (tb[IFLA_MASTER]) {
 		err = do_set_master(dev, nla_get_u32(tb[IFLA_MASTER]), extack);
 		if (err)
-- 
2.39.2

Re: [PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it

[Jiri Pirko]

Thu, Jan 04, 2024 at 05:42:59PM CET, nicolas.dichtel@6wind.com wrote:
>The below commit adds support for:
>> ip link set dummy0 down
>> ip link set dummy0 master bond0 up
>
>but breaks the opposite:
>> ip link set dummy0 up
>> ip link set dummy0 master bond0 down

It is a bit weird to see these 2 and assume some ordering.
The first one assumes:
dummy0 master bond 0, dummy0 up
The second one assumes:
dummy0 down, dummy0 master bond 0
But why?

What is the practival reason for a4abfa627c38 existence? I mean,
bond/team bring up the device themselfs when needed. Phil?
Wouldn't simple revert do better job here?


>
>Let's add a workaround to have both commands working.
>
>Cc: stable@vger.kernel.org
>Fixes: a4abfa627c38 ("net: rtnetlink: Enslave device before bringing it up")
>Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>Acked-by: Phil Sutter <phil@nwl.cc>
>Reviewed-by: David Ahern <dsahern@kernel.org>
>---
> net/core/rtnetlink.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>index e8431c6c8490..dd79693c2d91 100644
>--- a/net/core/rtnetlink.c
>+++ b/net/core/rtnetlink.c
>@@ -2905,6 +2905,14 @@ static int do_setlink(const struct sk_buff *skb,
> 		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
> 	}
> 
>+	/* Backward compat: enable to set interface down before enslaving it */
>+	if (!(ifm->ifi_flags & IFF_UP) && ifm->ifi_change & IFF_UP) {
>+		err = dev_change_flags(dev, rtnl_dev_combine_flags(dev, ifm),
>+				       extack);
>+		if (err < 0)
>+			goto errout;
>+	}
>+
> 	if (tb[IFLA_MASTER]) {
> 		err = do_set_master(dev, nla_get_u32(tb[IFLA_MASTER]), extack);
> 		if (err)
>-- 
>2.39.2
>
>

Re: [PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it

[Nicolas Dichtel]

Le 05/01/2024 à 12:59, Jiri Pirko a écrit :
> Thu, Jan 04, 2024 at 05:42:59PM CET, nicolas.dichtel@6wind.com wrote:
>> The below commit adds support for:
>>> ip link set dummy0 down
>>> ip link set dummy0 master bond0 up
>>
>> but breaks the opposite:
>>> ip link set dummy0 up
>>> ip link set dummy0 master bond0 down
> 
> It is a bit weird to see these 2 and assume some ordering.
> The first one assumes:
> dummy0 master bond 0, dummy0 up
> The second one assumes:
> dummy0 down, dummy0 master bond 0
> But why?
> 
> What is the practival reason for a4abfa627c38 existence? I mean,
> bond/team bring up the device themselfs when needed. Phil?
> Wouldn't simple revert do better job here?
Yeah, I assumed there was a good reason, but you're right.


> 
> 
>>
>> Let's add a workaround to have both commands working.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: a4abfa627c38 ("net: rtnetlink: Enslave device before bringing it up")
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> Acked-by: Phil Sutter <phil@nwl.cc>
>> Reviewed-by: David Ahern <dsahern@kernel.org>
>> ---
>> net/core/rtnetlink.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index e8431c6c8490..dd79693c2d91 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -2905,6 +2905,14 @@ static int do_setlink(const struct sk_buff *skb,
>> 		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
>> 	}
>>
>> +	/* Backward compat: enable to set interface down before enslaving it */
>> +	if (!(ifm->ifi_flags & IFF_UP) && ifm->ifi_change & IFF_UP) {
>> +		err = dev_change_flags(dev, rtnl_dev_combine_flags(dev, ifm),
>> +				       extack);
>> +		if (err < 0)
>> +			goto errout;
>> +	}
>> +
>> 	if (tb[IFLA_MASTER]) {
>> 		err = do_set_master(dev, nla_get_u32(tb[IFLA_MASTER]), extack);
>> 		if (err)
>> -- 
>> 2.39.2
>>
>>

Re: [PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it

[Phil Sutter]

Hi,

On Fri, Jan 05, 2024 at 12:59:24PM +0100, Jiri Pirko wrote:
> Thu, Jan 04, 2024 at 05:42:59PM CET, nicolas.dichtel@6wind.com wrote:
> >The below commit adds support for:
> >> ip link set dummy0 down
> >> ip link set dummy0 master bond0 up
> >
> >but breaks the opposite:
> >> ip link set dummy0 up
> >> ip link set dummy0 master bond0 down
> 
> It is a bit weird to see these 2 and assume some ordering.
> The first one assumes:
> dummy0 master bond 0, dummy0 up
> The second one assumes:
> dummy0 down, dummy0 master bond 0
> But why?
> 
> What is the practival reason for a4abfa627c38 existence? I mean,
> bond/team bring up the device themselfs when needed. Phil?
> Wouldn't simple revert do better job here?

Ah, I wasn't aware bond master manipulates slaves' links itself and thus
treated all types' link master setting the same by setting the slave up
afterwards. This is basically what a4abfa627c38 is good for: Enabling
'ip link set X master Y up' regardless of Y's link type.

If setting a bond slave up manually is not recommended, the easiest
solution is probbaly indeed to revert a4abfa627c38 and live with the
quirk in bond driver.

Cheers, Phil

Re: [PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it

[Jiri Pirko]

Sat, Jan 06, 2024 at 04:32:12AM CET, phil@nwl.cc wrote:
>Hi,
>
>On Fri, Jan 05, 2024 at 12:59:24PM +0100, Jiri Pirko wrote:
>> Thu, Jan 04, 2024 at 05:42:59PM CET, nicolas.dichtel@6wind.com wrote:
>> >The below commit adds support for:
>> >> ip link set dummy0 down
>> >> ip link set dummy0 master bond0 up
>> >
>> >but breaks the opposite:
>> >> ip link set dummy0 up
>> >> ip link set dummy0 master bond0 down
>> 
>> It is a bit weird to see these 2 and assume some ordering.
>> The first one assumes:
>> dummy0 master bond 0, dummy0 up
>> The second one assumes:
>> dummy0 down, dummy0 master bond 0
>> But why?
>> 
>> What is the practival reason for a4abfa627c38 existence? I mean,
>> bond/team bring up the device themselfs when needed. Phil?
>> Wouldn't simple revert do better job here?
>
>Ah, I wasn't aware bond master manipulates slaves' links itself and thus
>treated all types' link master setting the same by setting the slave up
>afterwards. This is basically what a4abfa627c38 is good for: Enabling
>'ip link set X master Y up' regardless of Y's link type.
>
>If setting a bond slave up manually is not recommended, the easiest
>solution is probbaly indeed to revert a4abfa627c38 and live with the
>quirk in bond driver.

Okay, lets revert then.

>
>Cheers, Phil

[PATCH net v3 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 down
> ip link set dummy0 master bond0 up

and
> ip link set dummy0 up
> ip link set dummy0 master bond0 down

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

diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index 26827ea4e3e5..181c689457e1 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,44 @@ 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 [ $? -ne 0 ]; then
+		end_test "SKIP bonding tests: cannot add net namespace $testns"
+		return $ksft_skip
+	fi
+
+	# test native tunnel
+	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
+
+	run_cmd ip -netns $testns link del dev $dummy
+	run_cmd ip -netns $testns link add dev $dummy type dummy
+	run_cmd ip -netns $testns link set dev $dummy down
+	run_cmd ip -netns $testns link set dev $dummy master $bond up
+	if [ $ret -ne 0 ]; then
+		end_test "FAIL: enslave a down interface in a bonding and set it up"
+		ip netns del "$testns"
+		return 1
+	fi
+
+	end_test "PASS: enslave iface in a bonding"
+	ip netns del "$testns"
+}
+
 kci_test_rtnl()
 {
 	local current_test
-- 
2.39.2

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

[Hangbin Liu]

On Thu, Jan 04, 2024 at 05:43:00PM +0100, Nicolas Dichtel wrote:
> +kci_test_enslave_bonding()
> +{
> +	local testns="testns"
> +	local bond="bond123"
> +	local dummy="dummy123"
> +	local ret=0
> +
> +	run_cmd ip netns add "$testns"
> +	if [ $? -ne 0 ]; then
> +		end_test "SKIP bonding tests: cannot add net namespace $testns"
> +		return $ksft_skip
> +	fi
> +
> +	# test native tunnel
> +	run_cmd ip -netns $testns link add dev $bond type bond mode balance-rr

Hi Nicolas,

If you are going to target the patch to net-next. Please update it in the
subject. And use `setup_ns` when create new netns.

Thanks
Hangbin

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

[Nicolas Dichtel]

Le 05/01/2024 à 03:26, Hangbin Liu a écrit :
> On Thu, Jan 04, 2024 at 05:43:00PM +0100, Nicolas Dichtel wrote:
>> +kci_test_enslave_bonding()
>> +{
>> +	local testns="testns"
>> +	local bond="bond123"
>> +	local dummy="dummy123"
>> +	local ret=0
>> +
>> +	run_cmd ip netns add "$testns"
>> +	if [ $? -ne 0 ]; then
>> +		end_test "SKIP bonding tests: cannot add net namespace $testns"
>> +		return $ksft_skip
>> +	fi
>> +
>> +	# test native tunnel
>> +	run_cmd ip -netns $testns link add dev $bond type bond mode balance-rr
> 
> Hi Nicolas,
> 
> If you are going to target the patch to net-next. Please update it in the
> subject. And use `setup_ns` when create new netns.
As said in the v2 thread, I will send a follow-up once net gets merged into
net-next.


Regards,
Nicolas

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

[Hangbin Liu]

On Fri, Jan 05, 2024 at 11:48:59AM +0100, Nicolas Dichtel wrote:
> > Hi Nicolas,
> > 
> > If you are going to target the patch to net-next. Please update it in the
> > subject. And use `setup_ns` when create new netns.
> As said in the v2 thread, I will send a follow-up once net gets merged into
> net-next.

OK, got it.

Thanks
Hangbin