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

[PATCH net v2 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.

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 v2 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>
---
 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 v2 1/2] rtnetlink: allow to set iface down before enslaving it

[David Ahern]

On 1/3/24 2:48 AM, Nicolas Dichtel 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
> 
> 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>
> ---
>  net/core/rtnetlink.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

[PATCH net v2 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>
---
 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..130be7de76af 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: enslave an up interface in a bonding"
+		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 an 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 v2 2/2] selftests: rtnetlink: check enslaving iface in a bond

[Hangbin Liu]

On Wed, Jan 03, 2024 at 10:48:46AM +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,

The net-next added new function setup_ns in lib.sh and converted all hard code
netns setup. I think It may be good to post this patch set to net-next
to reduce future merge conflicts.

Jakub, Paolo, please correct me if we can't post fixes to net-next.

Thanks
Hangbin

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

[Nicolas Dichtel]

Le 03/01/2024 à 14:00, Hangbin Liu a écrit :
> On Wed, Jan 03, 2024 at 10:48:46AM +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,
> 
> The net-next added new function setup_ns in lib.sh and converted all hard code
> netns setup. I think It may be good to post this patch set to net-next
> to reduce future merge conflicts.
The first patch is for net. I can post the second one to net-next if it eases
the merge.

> 
> Jakub, Paolo, please correct me if we can't post fixes to net-next.
Please, let me know if I should target net-next for the second patch.


Regards,
Nicolas

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

[Jakub Kicinski]

On Wed, 3 Jan 2024 15:15:33 +0100 Nicolas Dichtel wrote:
> > The net-next added new function setup_ns in lib.sh and converted all hard code
> > netns setup. I think It may be good to post this patch set to net-next
> > to reduce future merge conflicts.  
> 
> The first patch is for net. I can post the second one to net-next if it eases
> the merge.
> 
> > Jakub, Paolo, please correct me if we can't post fixes to net-next.  
>
> Please, let me know if I should target net-next for the second patch.

Looks like the patch applies to net-next, so hopefully there won't 
be any actual conflicts. But it'd be good to follow up and refactor
it in net-next once net gets merged in. As long as I'm not missing
anything - up to you - I'm fine with either sending the test to
net-next like Hangbin suggests, or following up in net-next to use
setup_ns.

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

[Nicolas Dichtel]

Le 03/01/2024 à 22:21, Jakub Kicinski a écrit :
> On Wed, 3 Jan 2024 15:15:33 +0100 Nicolas Dichtel wrote:
>>> The net-next added new function setup_ns in lib.sh and converted all hard code
>>> netns setup. I think It may be good to post this patch set to net-next
>>> to reduce future merge conflicts.  
>>
>> The first patch is for net. I can post the second one to net-next if it eases
>> the merge.
>>
>>> Jakub, Paolo, please correct me if we can't post fixes to net-next.  
>>
>> Please, let me know if I should target net-next for the second patch.
> 
> Looks like the patch applies to net-next, so hopefully there won't 
> be any actual conflicts. But it'd be good to follow up and refactor
> it in net-next once net gets merged in. As long as I'm not missing
> anything - up to you - I'm fine with either sending the test to
> net-next like Hangbin suggests, or following up in net-next to use
> setup_ns.
I will send a follow-up once net gets merged in net-next.


Thank you,
Nicolas

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

[Jakub Kicinski]

On Thu, 4 Jan 2024 14:51:08 +0100 Nicolas Dichtel wrote:
> > Looks like the patch applies to net-next, so hopefully there won't 
> > be any actual conflicts. But it'd be good to follow up and refactor
> > it in net-next once net gets merged in. As long as I'm not missing
> > anything - up to you - I'm fine with either sending the test to
> > net-next like Hangbin suggests, or following up in net-next to use
> > setup_ns.  
> I will send a follow-up once net gets merged in net-next.

Ack, there's still going to be a v3 to update the error message, 
tho, right?

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

[Nicolas Dichtel]

Le 04/01/2024 à 16:17, Jakub Kicinski a écrit :
> On Thu, 4 Jan 2024 14:51:08 +0100 Nicolas Dichtel wrote:
>>> Looks like the patch applies to net-next, so hopefully there won't 
>>> be any actual conflicts. But it'd be good to follow up and refactor
>>> it in net-next once net gets merged in. As long as I'm not missing
>>> anything - up to you - I'm fine with either sending the test to
>>> net-next like Hangbin suggests, or following up in net-next to use
>>> setup_ns.  
>> I will send a follow-up once net gets merged in net-next.
> 
> Ack, there's still going to be a v3 to update the error message, 
> tho, right?
Right, I was waiting for the conclusion about this patch ;-)

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

[David Ahern]

On 1/3/24 2:48 AM, Nicolas Dichtel wrote:
> @@ -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: enslave an up interface in a bonding"

interface is up, being put as a port on a bond and taken down at the
same time. That does not match this error message.


Thanks for adding test cases. Besides the error message:

Reviewed-by: David Ahern <dsahern@kernel.org>

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

[Nicolas Dichtel]

Le 03/01/2024 à 16:42, David Ahern a écrit :
> On 1/3/24 2:48 AM, Nicolas Dichtel wrote:
>> @@ -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: enslave an up interface in a bonding"
> 
> interface is up, being put as a port on a bond and taken down at the
> same time. That does not match this error message.
The idea was "the command used to enslave an up interface fails".
What about: "FAIL: set down and enslave an up interface in a bonding"

> 
> 
> Thanks for adding test cases. Besides the error message:
> 
> Reviewed-by: David Ahern <dsahern@kernel.org
Thank you,
Nicolas

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

[David Ahern]

On 1/3/24 9:17 AM, Nicolas Dichtel wrote:
> Le 03/01/2024 à 16:42, David Ahern a écrit :
>> On 1/3/24 2:48 AM, Nicolas Dichtel wrote:
>>> @@ -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: enslave an up interface in a bonding"
>>
>> interface is up, being put as a port on a bond and taken down at the
>> same time. That does not match this error message.
> The idea was "the command used to enslave an up interface fails".
> What about: "FAIL: set down and enslave an up interface in a bonding"

The 'set down' is part of the adding to the bond command. how about:

FAIL: Initially up interface added to a bond and set down.