Make sure the multicast release tasks are properly drained when destroying VNET and IFNET
ClosedPublic
Actions

Authored by • hselasky on May 19 2020, 10:33 AM.

Details

Reviewers

bz
jmg

Commits

rS364388: MFC r364072, r364073 and r364102:
rS364073: Make sure the multicast release tasks are properly drained when

Summary

Make sure the multicast release tasks are properly drained when
destroying a VNET or a network interface.

Else the inm release tasks, both IPv4 and IPv6 may cause a panic
accessing a freed VNET or network interface.

MFC after: 1 week
Sponsored by: Mellanox Technologies

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• hselasky created this revision.May 19 2020, 10:33 AM

Herald added subscribers: melifaro, ae, imp. · View Herald TranscriptMay 19 2020, 10:33 AM

• hselasky requested review of this revision.May 19 2020, 10:33 AM

Harbormaster completed remote builds in B31175: Diff 71986.May 19 2020, 10:33 AM

kib added inline comments.May 19 2020, 3:34 PM

sys/netinet/in_mcast.c
254 ↗	(On Diff #71986)	Is this ordered 'enough' to ensure that the drain happens after all other stuff ?
sys/netinet6/in6_mcast.c
523 ↗	(On Diff #71986)	This is an unrelated fix, right ?

• hselasky added inline comments.May 20 2020, 11:54 AM

sys/netinet/in_mcast.c
254 ↗	(On Diff #71986)	Testing is ongoing. From what I can see this is good enough.
sys/netinet6/in6_mcast.c
523 ↗	(On Diff #71986)	Yes, minor nit while at it. SYSINITs should take a pointer argument.

kp added a subscriber: kp.Jul 23 2020, 8:42 AM

freqlabs added a subscriber: freqlabs.Jul 23 2020, 2:38 PM

emaste added a subscriber: emaste.Jul 23 2020, 11:46 PM

• hselasky marked an inline comment as done.Aug 10 2020, 8:37 AM

• hselasky added a reviewer: jmg.Aug 10 2020, 8:55 AM

I had left comments on the mailing list discussions; I'll just add pointers here.

https://lists.freebsd.org/pipermail/freebsd-net/2020-July/056460.html
https://lists.freebsd.org/pipermail/freebsd-net/2020-July/056478.html

It would help a lot to understand why the teardown work has to be deferred on vnet shutdown to understand why we have to add an extra task on top to block and wait for it to finish rather than just rip it down?

@bz: I didn't see those comments yet. Thanks for the pointer.

@bz wrote:

Wow, did I miss that back then? Did I review a change and not notice?
Sorry if that was the case.

Vnet teardown is blocking and forceful.
Doing deferred cleanup work isn’t a good idea at all.
I think that is the real problem here.

I’d rather have us fix this than putting more bandaids into the code.

@bz wrote:

That’ll probably work; still, the deferred teardown work seems wrong
to me; I haven’t investigated; the patch kind-of says exactly that
as well: if “wait until deferred stuff is done” is all we are doing,
why can we not do it on the spot then?

@bz: The answer to your question is this commit:

https://svnweb.freebsd.org/base/head/sys/netinet/in_mcast.c?revision=333175&view=markup

--HPS

• hselasky edited the summary of this revision. (Show Details)Aug 10 2020, 9:40 AM

• hselasky removed a reviewer: kib.Aug 10 2020, 10:19 AM

• hselasky marked an inline comment as done.

In D24914#577073, @hselasky wrote:

@bz: The answer to your question is this commit:

https://svnweb.freebsd.org/base/head/sys/netinet/in_mcast.c?revision=333175&view=markup

Not really; the change (once you remove all the noise of renaming things which all should have been committed separately) doesn't justify the taskqueue anywhere; the only (extra!) locks are for the free list management; apart from that there seem to be no locking implications on freeing at the end, so that could always be done after unlock in inline if I am not wrong but it is hard to tell now about 30 commits later.

Your chnage seems to be the right thing for the current state of code; the current state of code however I believe is wrong. Go ahead and fix the immediate panic but could you do me a favour and leave an "XXX-BZ FIXME, see D24914" somewhere next to the two "VNET_SYSUNINIT" introductions; or rather, let me know once you commit this and I'll add them myself; probably cleaner that way.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 10 2020, 10:46 AM

Closed by commit rS364073: Make sure the multicast release tasks are properly drained when (authored by • hselasky). · Explain Why

This revision was automatically updated to reflect the committed changes.

• hselasky added a commit: rS364073: Make sure the multicast release tasks are properly drained when.