iflib: Fix handling of mbuf cluster allocation failures.
ClosedPublic
Actions

Authored by markj on Jun 27 2020, 3:36 PM.

Details

Reviewers

shurd
• hselasky
gallatin

Group Reviewers

iflib

Commits

rS362962: iflib: Fix handling of mbuf cluster allocation failures.

Summary

- When refilling an rx freelist, make sure we only update the hw
producer index if at least one cluster was allocated. Also make sure
that we don't update the fragment index cursor if the last allocation
attempt didn't succeed. For Intel drivers, iflib basically assumes
that the consumer index and fragment index cursor stay in lock step,
but this assumption was getting violated, resulting in use-after-frees
and NULL pointer dereferences.

Test Plan

Peter Holm was reporting occasional mbuf cluster use-after-frees
that were tracked back to these bugs. He verified that they are no
longer reproducible with this patch.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 32012
Build 29546: arc lint + arc unit

Event Timeline

markj created this revision.Jun 27 2020, 3:36 PM

Herald added a reviewer: shurd. · View Herald TranscriptJun 27 2020, 3:36 PM

Herald added a reviewer: iflib. · View Herald Transcript

Herald added subscribers: melifaro, ae. · View Herald Transcript

markj requested review of this revision.Jun 27 2020, 3:36 PM

Harbormaster completed remote builds in B32012: Diff 73772.Jun 27 2020, 3:36 PM

markj edited the test plan for this revision. (Show Details)Jun 27 2020, 3:45 PM

markj edited the test plan for this revision. (Show Details)

Don't attempt to handle m_gethdr() failures. Once clusters are placed
in the ring, we don't need to update the bitmap right away, so there's
no harm in simply leaving it there.

Harbormaster completed remote builds in B32025: Diff 73817.Jun 28 2020, 7:27 PM

markj edited the summary of this revision. (Show Details)Jun 28 2020, 7:28 PM

markj edited the test plan for this revision. (Show Details)Jun 30 2020, 3:15 PM

gallatin added reviewers: • hselasky, gallatin.Jun 30 2020, 4:58 PM

I added Hans, as he's also got an attempt to fix the same problem (https://reviews.freebsd.org/D24236)

sys/net/iflib.c
1987–1988	while you're adding __predict_false(), this seems like a good place for one..

This revision is now accepted and ready to land.Jun 30 2020, 5:01 PM

Hi,

I think a custom test to provoke the failure and make sure it is really gone is required. I simply added a sysctl which triggered allocation failures at a random point in the loop you are patching.

--HPS

In D25489#564279, @hselasky wrote:

Hi,

I think a custom test to provoke the failure and make sure it is really gone is required. I simply added a sysctl which triggered allocation failures at a random point in the loop you are patching.

--HPS

@pho has been able to trigger this bug reliably with stress2 since we discovered that it was triggered by cluster allocation failures. I used fail(9) when working on the patch but do not want to leave it in since this is a performance-sensitive path.

Sprinkle more __predict_false().

This revision now requires review to proceed.Jul 1 2020, 2:02 PM

Harbormaster completed remote builds in B32090: Diff 73964.Jul 1 2020, 2:02 PM

markj marked an inline comment as done.Jul 1 2020, 2:02 PM

emaste added a subscriber: emaste.Jul 2 2020, 7:45 PM

Any objection to committing this? It has been tested extensively by Peter.

No objections from me. I'll give it a spin at Mellanox once it hits the tree.

• hselasky accepted this revision.Jul 6 2020, 2:22 PM

This revision is now accepted and ready to land.Jul 6 2020, 2:22 PM

gallatin accepted this revision.Jul 6 2020, 2:43 PM

Closed by commit rS362962: iflib: Fix handling of mbuf cluster allocation failures. (authored by markj). · Explain WhyJul 6 2020, 2:52 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rS362962: iflib: Fix handling of mbuf cluster allocation failures..

Herald added a subscriber: imp. · View Herald TranscriptJul 6 2020, 2:52 PM

• hselasky mentioned this in D24236: Fix panic in iflib when out of mbuf clusters.Jul 9 2020, 10:12 AM

Revision Contents
Changeset List

Path

Size

sys/

net/

iflib.c

60 lines

Diff 73772

View Options

sys/net/iflib.c

Context not available.

	JAVA="%%LOCALBASE%%/bin/java"	JAVA="%%LOCALBASE%%/bin/java"
	JARPATH="%%JAVALIBDIR%%"	JARPATH="%%JAVALIBDIR%%"
	export JAVA_VERSION="%%JAVA_VERSION%%"
	glewisUnsubmitted Not Done Inline Actions Note that this may cause a subtle change in behaviour if either the ports tree is not present on a system or if javavmwrapper is otherwise instructed to use it's own internal logic (e.g. if JAVAVM_FALLBACK_ONLY is set). > env JAVAVM_FALLBACK_ONLY=yes JAVA_VERSION=1.6+ /usr/local/bin/java -version openjdk version "12.0.2" 2019-07-16 > env JAVAVM_FALLBACK_ONLY=yes /usr/local/bin/java -version openjdk version "13.0.1" 2019-10-15 This will depend on what versions the user has installed. This is actually more of a bug in javavmwrapper which I'll look into (it doesn't understand JDK 13 yet). It also doesn't have a default version (e.g. 8) and uses the "newest" one if no version is supplied. If a version is supplied then it uses the newest version that it understands. An alternative to all these script changes would be to have bsd.java.mk always add JAVA_VERSION to SUB_LIST, using the version it ends up deciding on if none is set. glewis: Note that this may cause a subtle change in behaviour if either the ports tree is not present…
	jkimAuthorUnsubmitted Not Done Inline Actions AFAIK, the inconsistency with JAVAVM_FALLBACK_ONLY is intentional. Actually, the manual page says: However, this option, when used with scripts installed by a port, may result in Java VM selection inconsistent with that intended by the script author. jkim: AFAIK, the inconsistency with JAVAVM_FALLBACK_ONLY is intentional. Actually, the manual page…
	glewisUnsubmitted Not Done Inline Actions As the author of javavmwrapper, it is certainly the case that when using JAVAVM_FALLBACK_ONLY that you may get different behaviour from what you do when the ports tree is present. What is a bug is that the presence of "JAVA_VERSION=1.6+" in the two command lines above shouldn't make a difference. Whether it is present or not, the fallback logic should choose the same version. That is what I was pointing out. So by removing that line from the invocation, this change may affect what version is chosen in that situation, even though it shouldn't. The likelihood of this impacting people is quite low though, so I'm not overly concerned. I also have a fix I can commit after this change (I don't want to commit while this is in flight since it will conflict with changes here). glewis: As the author of javavmwrapper, it is certainly the case that when using JAVAVM_FALLBACK_ONLY…
	jkimAuthorUnsubmitted Not Done Inline Actions Understood. Yes, it looks like there's a bug in javavmwrapper. It should not show any difference with or without the ports tree as the PR167799 said. jkim: Understood. Yes, it looks like there's a bug in javavmwrapper. It should not show any…

	exec "${JAVA}" -jar "${JARPATH}/%%JARNAME%%" "$@"	exec "${JAVA}" -jar "${JARPATH}/%%JARNAME%%" "$@"
Context not available.

iflib: Fix handling of mbuf cluster allocation failures.ClosedPublicActions