Page MenuHomeFreeBSD

Add extra INVARIANTS check for ctor/dtor
ClosedPublic

Authored by jmg on Jun 3 2015, 3:26 PM.
Tags
None
Referenced Files
F81969270: D2725.diff
Sat, Dec 14, 4:32 AM
Unknown Object (File)
Sat, Nov 16, 5:58 AM
Unknown Object (File)
Fri, Nov 15, 5:41 PM
Unknown Object (File)
Fri, Nov 15, 12:02 PM
Unknown Object (File)
Fri, Nov 15, 3:02 AM
Unknown Object (File)
Fri, Nov 15, 2:42 AM
Unknown Object (File)
Thu, Nov 14, 11:29 AM
Unknown Object (File)
Nov 14 2024, 1:40 AM

Details

Summary

NETAPP has added a check for all private zones with trash_ctor/trash_dtor.
They pass the trash_ctor/trash_dtor to uma_zcreate(9) if it is called
with NULL for constructor/destructor.

This patch is contributed by Suresh Gumpula at Netapp.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

pfg retitled this revision from to Add extra INVARIANTS check for ctor/dtor.
pfg updated this object.
pfg edited the test plan for this revision. (Show Details)
emaste added inline comments.
sys/vm/uma_core.c
1949

joined lines?

Reduce a little the width of the comments. It all looks weird
in phabricator.

While here I should mention this is not tested at all, I am
just hoping to help getting this interesting patch reviewed.

The mtx init code is assuming the object being initialized with zeros. Please make this change too in v_addpollinfo() in
freebsd/sys/kern/vfs_subr.c. Otherwise it panics because of uma_junk(0xdeadc0de) in the newly created object out of this zone.

Please replace this line
From: vi = uma_zalloc(vnodepoll_zone, M_WAITOK);
To: vi = uma_zalloc(vnodepoll_zone, M_ZERO | M_WAITOK);

Just to mention. This patch is in our product for almost a month without any issue in our testing. And we found couple of issues.

And this mutex issue in v_addpollinfo() is uncovered recently when a tail -f command is run.

M_ZERO uma_zalloc() in v_addpollinfo(). (submitted by Suresh)

It wouldn't hurt to commit this before the extra invariant check, but
we should sort out all similar cases before committing the main check.
(I think there are people running current with invariants in
production - sigh)

We have not seen any side effect other than this mutex issue for a month+ invariant build testing. I am thinking its good to go now.

We uncovered/realized this bug and others only after we committed this patch. So I think its really a useful check to have in .
https://svnweb.freebsd.org/base?view=revision&revision=281599

sys/vm/uma_dbg.c
78

What do you think about making this panic(...) optionally, by adding a new sysctl or kernel config knob.

I trust Suresh has tested the change very well and the check has proven useful.
I would like some more feedback since I am not running this directly though.

I am adding the reviewers of D2079 since it was found using this patch.

sys/vm/uma_core.c
1949

It only looks weird on phabricator. I will change it a little to see if it improves, thanks.

mtrash_ctor() on standard malloc zones also panics if it detects use after free unconditionally. So may be its good to keep both in sync instead of a panic knob with sysctl/options.

sys/vm/uma_dbg.c
78

I think it may be excessive to hide knobs within knobs or to add sysctls that depend on invariants. A panic may be too much though(?).

Yes, will do in the next 8 hours.

pfg marked 3 inline comments as done.Jun 19 2015, 10:07 PM

Hmm .. no one has approved this change .... abandon?

jmg edited reviewers, added: pfg; removed: jmg.

I have committed part to properly break up the change...

I'm taking over as pfg says he's busy.

sys/kern/vfs_subr.c
3597

This block was committed as part of D2890, r284733.

remove the vfs change as it has already been committed... This patch will be
committed in 24 hours unless comments are added.. It's languished too long..

jmg marked an inline comment as done.Jun 23 2015, 6:51 PM

INVARIANTS already covers this. If you didn't want your machine to panic when it detects code failures, you shouldn't have compiled w/ INVARIANTS. :)

This revision was automatically updated to reflect the committed changes.