Skip to content

Commit

Permalink
mroute: avoid calling if_allmulti with the lock held
Browse files Browse the repository at this point in the history
Avoid locking issues when if_allmulti() calls the driver's if_ioctl,
because that may acquire sleepable locks (while we hold a non-sleepable
rwlock).

Fortunately there's no pressing need to hold the mroute lock while we
do this, so we can postpone the call slightly, until after we've
released the lock.

This avoids the following WITNESS warning (with iflib drivers):

	lock order reversal: (sleepable after non-sleepable)
	 1st 0xffffffff82f64960 IPv4 multicast forwarding (IPv4 multicast forwarding, rw) @ /usr/src/sys/netinet/ip_mroute.c:1050
	 2nd 0xfffff8000480f180 iflib ctx lock (iflib ctx lock, sx) @ /usr/src/sys/net/iflib.c:4525
	lock order IPv4 multicast forwarding -> iflib ctx lock attempted at:
	#0 0xffffffff80bbd6ce at witness_checkorder+0xbbe
	#1 0xffffffff80b56d10 at _sx_xlock+0x60
	#2 0xffffffff80c9ce5c at iflib_if_ioctl+0x2dc
	#3 0xffffffff80c7c395 at if_setflag+0xe5
	#4 0xffffffff82f60a0e at del_vif_locked+0x9e
	#5 0xffffffff82f5f0d5 at X_ip_mrouter_set+0x265
	#6 0xffffffff80bfd402 at sosetopt+0xc2
	#7 0xffffffff80c02105 at kern_setsockopt+0xa5
	#8 0xffffffff80c02054 at sys_setsockopt+0x24
	#9 0xffffffff81046be8 at amd64_syscall+0x138
	#10 0xffffffff8101930b at fast_syscall_common+0xf8

See also:	https://redmine.pfsense.org/issues/12079
Reviewed by:	mjg
Sponsored by:	Rubicon Communications, LLC ("Netgate")
Differential Revision:	https://reviews.freebsd.org/D41209

(cherry picked from commit 680ad06)
  • Loading branch information
kprovost committed Jul 28, 2023
1 parent 0b72d3e commit f10efe9
Showing 1 changed file with 19 additions and 8 deletions.
27 changes: 19 additions & 8 deletions sys/netinet/ip_mroute.c
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ static void bw_upcalls_send(void);
static int del_bw_upcall(struct bw_upcall *);
static int del_mfc(struct mfcctl2 *);
static int del_vif(vifi_t);
static int del_vif_locked(vifi_t, struct ifnet **);
static int del_vif_locked(vifi_t, struct ifnet **, struct ifnet **);
static void expire_bw_upcalls_send(void *);
static void expire_mfc(struct mfc *);
static void expire_upcalls(void *);
Expand Down Expand Up @@ -618,7 +618,7 @@ if_detached_event(void *arg __unused, struct ifnet *ifp)
{
vifi_t vifi;
u_long i, vifi_cnt = 0;
struct ifnet *free_ptr;
struct ifnet *free_ptr, *multi_leave;

MRW_WLOCK();

Expand All @@ -635,6 +635,7 @@ if_detached_event(void *arg __unused, struct ifnet *ifp)
* 3. Expire any matching multicast forwarding cache entries.
* 4. Free vif state. This should disable ALLMULTI on the interface.
*/
restart:
for (vifi = 0; vifi < V_numvifs; vifi++) {
if (V_viftable[vifi].v_ifp != ifp)
continue;
Expand All @@ -647,9 +648,15 @@ if_detached_event(void *arg __unused, struct ifnet *ifp)
}
}
}
del_vif_locked(vifi, &free_ptr);
del_vif_locked(vifi, &multi_leave, &free_ptr);
if (free_ptr != NULL)
vifi_cnt++;
if (multi_leave) {
MRW_WUNLOCK();
if_allmulti(multi_leave, 0);
MRW_WLOCK();
goto restart;
}
}

MRW_WUNLOCK();
Expand Down Expand Up @@ -998,11 +1005,12 @@ add_vif(struct vifctl *vifcp)
* Delete a vif from the vif table
*/
static int
del_vif_locked(vifi_t vifi, struct ifnet **ifp_free)
del_vif_locked(vifi_t vifi, struct ifnet **ifp_multi_leave, struct ifnet **ifp_free)
{
struct vif *vifp;

*ifp_free = NULL;
*ifp_multi_leave = NULL;

MRW_WLOCK_ASSERT();

Expand All @@ -1015,7 +1023,7 @@ del_vif_locked(vifi_t vifi, struct ifnet **ifp_free)
}

if (!(vifp->v_flags & (VIFF_TUNNEL | VIFF_REGISTER)))
if_allmulti(vifp->v_ifp, 0);
*ifp_multi_leave = vifp->v_ifp;

if (vifp->v_flags & VIFF_REGISTER) {
V_reg_vif_num = VIFI_INVALID;
Expand Down Expand Up @@ -1045,14 +1053,17 @@ static int
del_vif(vifi_t vifi)
{
int cc;
struct ifnet *free_ptr;
struct ifnet *free_ptr, *multi_leave;

MRW_WLOCK();
cc = del_vif_locked(vifi, &free_ptr);
cc = del_vif_locked(vifi, &multi_leave, &free_ptr);
MRW_WUNLOCK();

if (free_ptr)
if (multi_leave)
if_allmulti(multi_leave, 0);
if (free_ptr) {
if_free(free_ptr);
}

return cc;
}
Expand Down

0 comments on commit f10efe9

Please sign in to comment.