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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
| sys/vm/uma_core.c | ||
|---|---|---|
| 1949 | joined lines? | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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)
Comment Actions
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.
Comment Actions
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. | |
Comment Actions
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. | |
Comment Actions
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(?). | |
Comment Actions
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..
Comment Actions
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. :)