Page MenuHomeFreeBSD

iflib: Fix handling of mbuf cluster allocation failures.
Needs ReviewPublic

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

Details

Reviewers
shurd
hselasky
gallatin
Group Reviewers
iflib
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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 32090
Build 29609: arc lint + arc unit

Event Timeline

markj created this revision.Sat, Jun 27, 3:36 PM
markj requested review of this revision.Sat, Jun 27, 3:36 PM
markj edited the test plan for this revision. (Show Details)Sat, Jun 27, 3:45 PM
markj edited the test plan for this revision. (Show Details)
markj updated this revision to Diff 73817.Sun, Jun 28, 7:27 PM

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.

markj edited the summary of this revision. (Show Details)Sun, Jun 28, 7:28 PM
markj edited the test plan for this revision. (Show Details)Tue, Jun 30, 3:15 PM
gallatin accepted this revision.Tue, Jun 30, 5:01 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
1986–1987

while you're adding __predict_false(), this seems like a good place for one..

This revision is now accepted and ready to land.Tue, Jun 30, 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

markj added a subscriber: pho.Wed, Jul 1, 2: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

@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.

markj updated this revision to Diff 73964.Wed, Jul 1, 2:02 PM

Sprinkle more __predict_false().

This revision now requires review to proceed.Wed, Jul 1, 2:02 PM
markj marked an inline comment as done.Wed, Jul 1, 2:02 PM
emaste added a subscriber: emaste.Thu, Jul 2, 7:45 PM