| Message ID | 20230313132834.946360-3-razor@blackwall.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bonding: properly restore flags when non-eth dev enslave fails | expand |
On 13/03/2023 15:28, Nikolay Aleksandrov wrote: [snip] > +kci_test_enslaved_bond_non_eth() > +{ > + local ret=0 > + > + ip link add name test-nlmon0 type nlmon > + ip link add name test-bond0 type bond > + ip link add name test-bond1 type bond > + ip link set dev test-bond0 master test-bond1 > + ip link set dev test-nlmon0 master test-bond0 1>/dev/null 2>/dev/null > + > + ip -d l sh dev test-bond0 | grep -q "SLAVE" > + if [ $? -ne 0 ]; then > + echo "FAIL: IFF_SLAVE flag is missing from the bond device" > + check_err 1 > + fi > + ip -d l sh dev test-bond0 | grep -q "MASTER" > + if [ $? -ne 0 ]; then > + echo "FAIL: IFF_MASTER flag is missing from the bond device" > + check_err 1 > + fi > + > + # on error we return before cleaning up as that may hang the system I wasn't sure if this part was ok, let me know if you prefer to always attempt cleaning up and I'll send v2 moving the return after the cleanup attempt. > + if [ $ret -ne 0 ]; then > + return 1 > + fi > + > + # clean up any leftovers > + ip link del dev test-bond0 > + ip link del dev test-bond1 > + ip link del dev test-nlmon0 > + > + echo "PASS: enslaved bond device has flags restored properly" > +} > + > kci_test_rtnl() > { > local ret=0 > @@ -1276,6 +1310,8 @@ kci_test_rtnl() > check_err $? > kci_test_bridge_parent_id > check_err $? > + kci_test_enslaved_bond_non_eth > + check_err $? > > kci_del_dummy > return $ret
On 13/03/2023 15:32, Nikolay Aleksandrov wrote: > On 13/03/2023 15:28, Nikolay Aleksandrov wrote: > [snip] >> +kci_test_enslaved_bond_non_eth() >> +{ >> + local ret=0 >> + >> + ip link add name test-nlmon0 type nlmon >> + ip link add name test-bond0 type bond >> + ip link add name test-bond1 type bond >> + ip link set dev test-bond0 master test-bond1 >> + ip link set dev test-nlmon0 master test-bond0 1>/dev/null 2>/dev/null >> + >> + ip -d l sh dev test-bond0 | grep -q "SLAVE" >> + if [ $? -ne 0 ]; then >> + echo "FAIL: IFF_SLAVE flag is missing from the bond device" >> + check_err 1 >> + fi >> + ip -d l sh dev test-bond0 | grep -q "MASTER" >> + if [ $? -ne 0 ]; then >> + echo "FAIL: IFF_MASTER flag is missing from the bond device" >> + check_err 1 >> + fi >> + >> + # on error we return before cleaning up as that may hang the system > > I wasn't sure if this part was ok, let me know if you prefer to always attempt cleaning up > and I'll send v2 moving the return after the cleanup attempt. > Actually sorry for the noise, I'll just send v2 instead with that change. I don't have a good argument why we shouldn't attempt a cleanup after printing the error messages even if the system hangs.
diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh index 275491be3da2..02964b2afd8d 100755 --- a/tools/testing/selftests/net/rtnetlink.sh +++ b/tools/testing/selftests/net/rtnetlink.sh @@ -1225,6 +1225,40 @@ kci_test_bridge_parent_id() echo "PASS: bridge_parent_id" } +kci_test_enslaved_bond_non_eth() +{ + local ret=0 + + ip link add name test-nlmon0 type nlmon + ip link add name test-bond0 type bond + ip link add name test-bond1 type bond + ip link set dev test-bond0 master test-bond1 + ip link set dev test-nlmon0 master test-bond0 1>/dev/null 2>/dev/null + + ip -d l sh dev test-bond0 | grep -q "SLAVE" + if [ $? -ne 0 ]; then + echo "FAIL: IFF_SLAVE flag is missing from the bond device" + check_err 1 + fi + ip -d l sh dev test-bond0 | grep -q "MASTER" + if [ $? -ne 0 ]; then + echo "FAIL: IFF_MASTER flag is missing from the bond device" + check_err 1 + fi + + # on error we return before cleaning up as that may hang the system + if [ $ret -ne 0 ]; then + return 1 + fi + + # clean up any leftovers + ip link del dev test-bond0 + ip link del dev test-bond1 + ip link del dev test-nlmon0 + + echo "PASS: enslaved bond device has flags restored properly" +} + kci_test_rtnl() { local ret=0 @@ -1276,6 +1310,8 @@ kci_test_rtnl() check_err $? kci_test_bridge_parent_id check_err $? + kci_test_enslaved_bond_non_eth + check_err $? kci_del_dummy return $ret
Add a new selftest for the recent bonding bug hit by syzbot[1] which causes a warning and results in wrong device flags (IFF_SLAVE missing). The test adds two bond devices and a nlmon device, enslaves one of the bond devices to the other and then tries to enslave the nlmon device to the enslaved bond testing the bond_enslave() error path when trying to enslave a non-eth device. It checks that both MASTER and SLAVE flags are properly restored. If the flags are properly restored we get: PASS: enslaved bond device has flags restored properly [1] https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef Cc: Shigeru Yoshida <syoshida@redhat.com> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> --- tools/testing/selftests/net/rtnetlink.sh | 36 ++++++++++++++++++++++++ 1 file changed, 36 insertions(+)