svn commit: r334104 - in head/sys: netinet sys

[Mateusz Guzik]

On Thu, May 24, 2018 at 2:40 AM, Jonathan T. Looney <jtl at freebsd.org> wrote:

> On Wed, May 23, 2018 at 7:13 PM, Matthew Macy <mmacy at freebsd.org> wrote:
> >
> > On Wed, May 23, 2018 at 11:52 AM, John Baldwin <jhb at freebsd.org> wrote:
> > > On Wednesday, May 23, 2018 05:00:05 PM Matt Macy wrote:
> > >> Author: mmacy
> > >> Date: Wed May 23 17:00:05 2018
> > >> New Revision: 334104
> > >> URL: https://svnweb.freebsd.org/changeset/base/334104
> > >>
> > >> Log:
> > >>   epoch: allow for conditionally asserting that the epoch context
> fields
> > >>   are unused by zeroing on INVARIANTS builds
> > >
> > > Is M_ZERO really so bad that you need to make it conditional?
> >
> > In this case not at all. It's only exercised by sysctl handlers. I'm
> > mostly responding to an inquiry by jtl. However, gratuitous M_ZERO
> > usage does have a cumulative adverse performance impact.
>
> I appreciate you making this change. And, I do think it is worth avoiding
> M_ZERO where it is unnecessary, for the reason you state.
>
>
I agree that M_ZERO for no reason should be avoided, especially so with the
current implementation of said zeroing (mandatory call to bzero, which is
not fast
either).


>
> > > I would probably have preferred something like 'M_ZERO_INVARIANTS'
> > > instead perhaps (or M_ZERO_EPOCH) that only controls M_ZERO and is
> > > still or'd with M_WAITOK or M_NOWAIT.
> >
> > Yes. I like that better too. Thanks.
>
> Yes, that does seem better.
>
>
I fundamentally disagree with this part.

If a known value of a given field is needed for assertion purposes, you
can add (possibly conditional) code setting this specific value. It
probably should not be zero if it can be helped.

Conditional zeroing of the *whole* struct depending on invariants will
*hide* uninitialized memory read bugs - production kernel will have
whatever it happens to find, while *debug* kernel will guarantee to
have all the values zeroed. In fact the flag actively combats redzoning.
if the resulting allocation is zeroed, poisoning is actively neutered.
But only if debug is enabled.

That said, I find the change harmful.

#define epoch_debug_init or similar can be used here instead.

-- 
Mateusz Guzik <mjguzik gmail.com>