Differential D19578

if_bridge(4): Fix module teardown
ClosedPublic
Actions

Authored by kevans on Mar 14 2019, 1:42 AM.

Details

Reviewers

kp
bz

Commits

rS345180: if_bridge(4): Fix module teardown

Summary

Currently, the following steps will lead to an uma panic:

kldload if_bridge
ifconfig bridge0 create
ifconfig bridge0 addm wlan0 addm em0 up
dhclient em0
kldunload if_bridge

Because bridge_rtnode_zone has outstanding allocations at the time of destruction.

Fix this by explicitly destroying the bridge interfaces at UNINIT. I chose to (attempt) to virtualize the uma zone these allocations come from to simplify things -- trying to reason through how best to destroy the uma zone once all of the bridges were cleaned up across all of the vnets gave me a bit of a headache. I am not completely sure how successful I was, but it looked at a glance like all of the bridge_rt* functions are called in a proper vnet context such that I didn't need to CURVNET_SET (ioctl handlers, IIRC).

Additionally, bridge_rtable_fini now flushes out the routing table to free the memory referenced in the initial panic.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 23076

Event Timeline

kevans created this revision.Mar 14 2019, 1:42 AM

Herald added subscribers: ae, imp. · View Herald TranscriptMar 14 2019, 1:42 AM

Harbormaster completed remote builds in B23076: Diff 55044.Mar 14 2019, 1:42 AM

I guess I should mention that my original example isn't actually accurate to what I'm doing -- I've got an interface like OpenBSD's if_vether that I'm throwing in the bridge rather than em0. That doesn't work for other reasons, but it was clear that the panic was legit.

Sorry, did the dumb there... ripped out the explicit teardown of the clones, since that actually happens. bridge_rtnode_zone remains virtualized, because that's still the cleanest way -- the clones get destroyed upon if_clone_detach, which happens after the MOD_UNLOAD event.

The bridge_rtnode_destroy reference does need CURVNET magic, because bridge_timer -> bridge_rtage -> bridge_rtnode_destroy isn't already setting the proper vnet. bridge_rtupdate is only called from bridge_input/bridge_forward/bridge_ioctl.

Harbormaster completed remote builds in B23077: Diff 55046.Mar 14 2019, 4:36 AM

I'll try to take a more in-depth look later today.

sys/net/if_bridge.c
3039	Do you know where we call into bridge_rtnode_destroy() without a vnet context set? Is it a timeout callback? In that case it might be better to set the context in the callback itself.

kevans added inline comments.Mar 14 2019, 11:41 AM

sys/net/if_bridge.c
3039	Yeah, it's from a timeout callback. I'll push this back up into it and upload a new version soon.

Push vnet context up into the callout callback

Harbormaster completed remote builds in B23084: Diff 55056.Mar 14 2019, 1:11 PM

Do we need to virtualise the bridge_rtnode_zone? Doesn't the cleanup call to bridge_rtflush() take care of all of the allocations already?

sys/net/if_bridge.c
2777	It makes no real difference here, but I"d be inclined to put the restore at the end of the function. Mostly in case someone else comes by later and adds something which does need to be in the correct VNET context.

In D19578#419259, @kristof wrote:

Do we need to virtualise the bridge_rtnode_zone? Doesn't the cleanup call to bridge_rtflush() take care of all of the allocations already?

It would take care of it, but bridge_rtflush (spawned from VNET_SYSUNINIT) happens -after- the destruction of bridge_rtnode_zone (MOD_UNLOAD), so we have an ordering problem.

Oh yes, of course.

I've fixed a couple of other bugs where people forgot or did not know that the vnet uninit flow is a little unusual (i.e. last vnet uninit happens after unload) so I should have realised.

This revision is now accepted and ready to land.Mar 14 2019, 6:00 PM

Closed by commit rS345180: if_bridge(4): Fix module teardown (authored by kevans). · Explain WhyMar 15 2019, 1:20 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

sys/

net/

if_bridge.c

30 lines

Diff 55044

View Options

sys/net/if_bridge.c

	.\" $FreeBSD$			.\" $FreeBSD$
	Set to not build and install the enhanced			Install the traditional FreeBSD
	.Xr bc 1			.Xr bc 1
				jhbUnsubmitted Not Done Inline Actions I would maybe avoid making other changes for this commit than just the "Set to" part. Losing the Xr sections for bc and dc seems rather odd. Granted, I would rewrite this description to be something like "Use the traditional FreeBSD versions of bc(1) and dc(1) rather than the <foo> versions." where <foo> is more descriptive than just "enhanced". jhb: I would maybe avoid making other changes for this commit than just the "Set to" part. Losing…
				emasteAuthorUnsubmitted Done Inline Actions Huh, I think this must have been an accident during a rebase, I did not intend to drop these. When I first did this it was a bit of sed with manual review/touch-ups. In can incorporate feedback before this gets committed and/or we can make further improvements like this one after commit. I am also not sure what <foo> would be off hand. emaste: Huh, I think this must have been an accident during a rebase, I did not intend to drop these.
	and			and
	.Xr dc 1			.Xr dc 1
	programs instead of the traditional FreeBSD versions.			programs instead of the enahnced versions.
				emasteAuthorUnsubmitted Done Inline Actions and of course I have a typo here. corrected locally. emaste: and of course I have a typo here. corrected locally.

if_bridge(4): Fix module teardown ClosedPublicActions