mbuf: do not restore dying interfaces
ClosedPublic
Actions

Authored by kp on Jan 28 2022, 10:34 AM.

Details

Reviewers

Group Reviewers

network
pfsense

Commits

rG613acc64834b: mbuf: do not restore dying interfaces
rG703e533da5e2: mbuf: do not restore dying interfaces

Summary

When we remove an interface it is first removed from the interface list
V_ifnet (by if_unlink_ifnet()) and marked as IFF_DYING. We then wait for
any possible references to stop being used (i.e.
epoch_wait/epoch_drain_callbacks) before we tear it fully down.

However, the index in ifindex_table is not removed, so m_rcvif_restore()
can still find the (now dying) interface.

This results in panics, for example when dummynet restores the rcvif
pointer and passes a packet to ip6_input() we can panic because the
AF_INET6 domain has already been removed (so we end up dereferencing a
NULL pointer there).

Check that the interface is not dying before we restore it, which is
equivalent to checking its presence in V_ifnet, and thus ensures that
future accesses (while in NET_EPOCH) are safe.

Sponsored by: Rubicon Communications, LLC ("Netgate")

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 44139
Build 41027: arc lint + arc unit

Event Timeline

kp created this revision.Jan 28 2022, 10:34 AM

Herald added subscribers: donner, melifaro, ae, imp. · View Herald TranscriptJan 28 2022, 10:34 AM

kp requested review of this revision.Jan 28 2022, 10:34 AM

Harbormaster completed remote builds in B44138: Diff 102038.Jan 28 2022, 10:34 AM

Remove mostly unrelated dummynet fix

Harbormaster completed remote builds in B44139: Diff 102039.Jan 28 2022, 10:35 AM

Why can't we remove it from the ifindex, too? That would be a normal delayed free practice: remove all references to a structure, mark it as being deleted and wait for threads that hold a reference to finish.

Note: we set this flag twice: if_unlink_vnet() and if_free(). This doesn't look consistent.

Ok, this second IFF_DYING was added in a716ad4a35627bfe68d6681e1a4bb0c1e922325d

In D34076#770623, @glebius wrote:

Why can't we remove it from the ifindex, too? That would be a normal delayed free practice: remove all references to a structure, mark it as being deleted and wait for threads that hold a reference to finish.

That might be better. I'll take a closer look at that tomorrow, when I'm sure I'm not going to break if_vmove().

I'm also digging that. I think your patch is fine as temporary measure.

This revision is now accepted and ready to land.Jan 28 2022, 5:52 PM

Closed by commit rG703e533da5e2: mbuf: do not restore dying interfaces (authored by kp). · Explain WhyJan 28 2022, 11:10 PM

This revision was automatically updated to reflect the committed changes.

kp added a commit: rG703e533da5e2: mbuf: do not restore dying interfaces.

zec added a reverting change: rG6c741ffbfa7f: Revert "mbuf: do not restore dying interfaces".May 3 2022, 5:29 PM

emaste added a commit: rG613acc64834b: mbuf: do not restore dying interfaces.May 5 2022, 7:16 PM