Add extra INVARIANTS check for ctor/dtor

[jmg]

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.

[emaste]

sys/vm/uma_core.c

1949 ↗

(On Diff #5912)

joined lines?

[pfg]

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.

[gsuryacse7k_gmail.com]

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.

[pfg]

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)

[gsuryacse7k_gmail.com]

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.

[gsuryacse7k_gmail.com]

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

[op]

sys/vm/uma_dbg.c

78 ↗

(On Diff #5922)

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

[pfg]

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 ↗

(On Diff #5912)

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

[gsuryacse7k_gmail.com]

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.

[pfg]

sys/vm/uma_dbg.c

78 ↗

(On Diff #5923)

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(?).

[op]

Yes, will do in the next 8 hours.

[pfg]

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

[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 ↗

(On Diff #5923)

This block was committed as part of D2890, r284733.

[jmg]

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]

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. :)