Page MenuHomeFreeBSD
Differential D16882

Handle case where mbuf == &txq when IFC_QFLUSH is set
ClosedPublic
Actions

Authored by shurd on Aug 24 2018, 3:38 PM.
Tags
None
Referenced Files
F105962852: D16882.diff
Mon, Dec 23, 4:21 AM
Unknown Object (File)
Fri, Dec 20, 10:58 AM
Unknown Object (File)
Fri, Dec 20, 10:32 AM
Unknown Object (File)
Fri, Dec 20, 10:29 AM
Unknown Object (File)
Wed, Dec 18, 11:19 AM
Unknown Object (File)
Nov 21 2024, 7:27 PM
Unknown Object (File)
Nov 21 2024, 7:27 PM
Unknown Object (File)
Nov 21 2024, 7:26 PM
View All 68 Files
Subscribers
ae
imp

Details

Reviewers
mmacy
scottl
gallatin
sbruno
Commits
rS338372: Fix potential data corruption in iflib
Summary

The ring can have a pointer to txq in it instead of an mbuf.
When this happens, we absolutely don't want to pass it to m_free().

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19242
Build 18855: arc lint + arc unit

Event Timeline

shurd created this revision.Aug 24 2018, 3:38 PM
Herald added subscribers: ae, imp. · View Herald TranscriptAug 24 2018, 3:38 PM
Harbormaster completed remote builds in B19137: Diff 47239.Aug 24 2018, 3:38 PM
shurd updated this revision to Diff 47392.Aug 28 2018, 5:21 PM

Fix test for txq, and cast to the appropriate type before comparison

Harbormaster completed remote builds in B19242: Diff 47392.Aug 28 2018, 5:21 PM
shurd updated this revision to Diff 47393.Aug 28 2018, 5:22 PM

mp_ring->items is void *, not struct mbuf *.

Harbormaster completed remote builds in B19243: Diff 47393.Aug 28 2018, 5:22 PM
gallatin added a comment.Aug 28 2018, 6:20 PM

What keeps the normal, non-IFC_QFLUSH from hitting this condition? Is the txq pointer included in r->size?

shurd added a comment.Aug 28 2018, 8:58 PM
In D16882#361076, @gallatin wrote:

What keeps the normal, non-IFC_QFLUSH from hitting this condition? Is the txq pointer included in r->size?

The check at line 3671 prevents it, and yes, the txq pointer is added to the ring just as though it's an mbuf, so anything looking at the ring contents needs to test against txq before assuming it's an mbuf, and it's included in r->size.

gallatin added a comment.Aug 28 2018, 10:57 PM

Sorry, having the ring pointer in the ring is just so odd.. Can you explain why it is there? Maybe it would be better to just not have the ring pointer in the ring and not have to have all these special cases.

I'm fighting an iflib bug myself, and every time I have to dig into the code, i'm irritated at what seems to be complexity driven by premature optimization...

shurd added a comment.Aug 28 2018, 11:45 PM
In D16882#361193, @gallatin wrote:

Sorry, having the ring pointer in the ring is just so odd.. Can you explain why it is there? Maybe it would be better to just not have the ring pointer in the ring and not have to have all these special cases.

I'm fighting an iflib bug myself, and every time I have to dig into the code, i'm irritated at what seems to be complexity driven by premature optimization...

As I recall, it's used to wake up the ring to send laggards. Basically, if you add items to the mp ring while there's an active consumer, those items won't be sent until the next enqueue(). Since the time to the next packet is unbounded, the txq pointer is passed to enqueue() so they get sent at that point. I suspect txq was used simply because it's a non-NULL pointer we have laying around that we know will never be a valid mbuf. @mmacy may be able to add more colour to that.

It's one of those things that is on the Big List of Things To Change.

mmacy added a comment.Aug 29 2018, 1:27 AM
In D16882#361200, @shurd wrote:
In D16882#361193, @gallatin wrote:

Sorry, having the ring pointer in the ring is just so odd.. Can you explain why it is there? Maybe it would be better to just not have the ring pointer in the ring and not have to have all these special cases.

I'm fighting an iflib bug myself, and every time I have to dig into the code, i'm irritated at what seems to be complexity driven by premature optimization...

As I recall, it's used to wake up the ring to send laggards. Basically, if you add items to the mp ring while there's an active consumer, those items won't be sent until the next enqueue(). Since the time to the next packet is unbounded, the txq pointer is passed to enqueue() so they get sent at that point. I suspect txq was used simply because it's a non-NULL pointer we have laying around that we know will never be a valid mbuf. @mmacy may be able to add more colour to that.

It's one of those things that is on the Big List of Things To Change.

It's a sentinel value for the reason described above. It could be any non-mbuf value - Van Jacobsen's birthday if you like.

gallatin accepted this revision.Aug 29 2018, 10:08 AM

For what it is, Van's birthday might be a better choice. Eg, some non-valid pointer that would cause free or mfree to panic rather than accept it and corrupt data.

This revision is now accepted and ready to land.Aug 29 2018, 10:08 AM
Closed by commit rS338372: Fix potential data corruption in iflib (authored by shurd). · Explain WhyAug 29 2018, 3:55 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
net/
3 lines

Diff 47392

View Options

sys/net/iflib.c

Loading...