From 92c23f6d9c2074f6deb0029d13a8c92b32797059 Mon Sep 17 00:00:00 2001 From: Kristof Provost Date: Fri, 12 May 2023 10:42:48 +0200 Subject: [PATCH] vlan: fix setting flags on a QinQ interface Setting vlan flags needlessly takes the exclusive VLAN_XLOCK(). If we have stacked vlan devices (i.e. QinQ) and we set vlan flags (e.g. IFF_PROMISC) we call rtnl_handle_ifevent() to send a notification about the interface. This ends up calling SIOCGIFMEDIA, which requires the VLAN_SLOCK(). Trying to take that one with the VLAN_XLOCK() held deadlocks us. There's no need for the exclusive lock though, as we're only accessing parent/trunk information, not modifying it, so a shared lock is sufficient. While here also add a test case for this issue. Backtrace: shared lock of (sx) vlan_sx @ /usr/src/sys/net/if_vlan.c:2192 while exclusively locked from /usr/src/sys/net/if_vlan.c:2307 panic: excl->share cpuid = 29 time = 1683873033 KDB: stack backtrace: db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe015d4ad4b0 vpanic() at vpanic+0x152/frame 0xfffffe015d4ad500 panic() at panic+0x43/frame 0xfffffe015d4ad560 witness_checkorder() at witness_checkorder+0xcb5/frame 0xfffffe015d4ad720 _sx_slock_int() at _sx_slock_int+0x67/frame 0xfffffe015d4ad760 vlan_ioctl() at vlan_ioctl+0xf8/frame 0xfffffe015d4ad7c0 dump_iface() at dump_iface+0x12f/frame 0xfffffe015d4ad840 rtnl_handle_ifevent() at rtnl_handle_ifevent+0xab/frame 0xfffffe015d4ad8c0 if_setflag() at if_setflag+0xf6/frame 0xfffffe015d4ad930 ifpromisc() at ifpromisc+0x2a/frame 0xfffffe015d4ad960 vlan_setflags() at vlan_setflags+0x60/frame 0xfffffe015d4ad990 vlan_ioctl() at vlan_ioctl+0x216/frame 0xfffffe015d4ad9f0 if_setflag() at if_setflag+0xe4/frame 0xfffffe015d4ada60 ifpromisc() at ifpromisc+0x2a/frame 0xfffffe015d4ada90 bridge_ioctl_add() at bridge_ioctl_add+0x499/frame 0xfffffe015d4adb10 bridge_ioctl() at bridge_ioctl+0x328/frame 0xfffffe015d4adbc0 ifioctl() at ifioctl+0x972/frame 0xfffffe015d4adcc0 kern_ioctl() at kern_ioctl+0x1fe/frame 0xfffffe015d4add30 sys_ioctl() at sys_ioctl+0x154/frame 0xfffffe015d4ade00 amd64_syscall() at amd64_syscall+0x140/frame 0xfffffe015d4adf30 fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe015d4adf30 --- syscall (54, FreeBSD ELF64, ioctl), rip = 0x22b0f0ef8d8a, rsp = 0x22b0ec63f2c8, rbp = 0x22b0ec63f380 --- KDB: enter: panic [ thread pid 5715 tid 101132 ] Sponsored by: Rubicon Communications, LLC ("Netgate") --- sys/net/if_vlan.c | 4 ++-- tests/sys/net/if_vlan.sh | 29 +++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c index f5b401c446ed..6aa872a19364 100644 --- a/sys/net/if_vlan.c +++ b/sys/net/if_vlan.c @@ -2304,10 +2304,10 @@ vlan_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data) * We should propagate selected flags to the parent, * e.g., promiscuous mode. */ - VLAN_XLOCK(); + VLAN_SLOCK(); if (TRUNK(ifv) != NULL) error = vlan_setflags(ifp, 1); - VLAN_XUNLOCK(); + VLAN_SUNLOCK(); break; case SIOCADDMULTI: diff --git a/tests/sys/net/if_vlan.sh b/tests/sys/net/if_vlan.sh index 96426254bb6d..e4c737121bdb 100755 --- a/tests/sys/net/if_vlan.sh +++ b/tests/sys/net/if_vlan.sh @@ -221,6 +221,34 @@ qinq_dot_cleanup() vnet_cleanup } +atf_test_case "qinq_setflags" "cleanup" +qinq_setflags_head() +{ + atf_set descr 'Test setting flags on a QinQ device' + atf_set require.user root +} + +qinq_setflags_body() +{ + vnet_init + + epair=$(vnet_mkepair) + + ifconfig ${epair}a up + vlan1=$(ifconfig vlan create) + ifconfig $vlan1 vlan 1 vlandev ${epair}a + vlan2=$(ifconfig vlan create) + ifconfig $vlan2 vlan 2 vlandev $vlan1 + + # This panics, incorrect locking + ifconfig $vlan2 promisc +} + +qinq_setflags_cleanup() +{ + vnet_cleanup +} + atf_test_case "bpf_pcp" "cleanup" bpf_pcp_head() { @@ -273,5 +301,6 @@ atf_init_test_cases() atf_add_test_case "qinq_deep" atf_add_test_case "qinq_legacy" atf_add_test_case "qinq_dot" + atf_add_test_case "qinq_setflags" atf_add_test_case "bpf_pcp" }