linux-kernel - Re: [PATCH] btrfs: zero out delayed node upon allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <50EAEB85-05BF-4E5E-B074-BF5E522236DF@suse.com>
Date:	Sun, 25 Oct 2015 13:33:45 -0400
From:	Jeff Mahoney <jeffm@...e.com>
To:	Alexandru Moise <00moses.alexander00@...il.com>
Cc:	clm@...com, jbacik@...com, dsterba@...e.com,
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: zero out delayed node upon allocation

On Oct 25, 2015, at 3:50 PM, Alexandru Moise <00moses.alexander00@...il.com> wrote:

>>> This allows us to trim out half of btrfs_init_delayed_node() which
>>> is now reduntant.
>> 
>> It's redundant if kmem_cache_zalloc is used, but you haven't
>> documented that doing so is now required.  For all of these changes
>> you've posted, if they're to be accepted, I'd really prefer to set up
>> the slab with a constructor instead.  Then we don't need to worry
>> about such guarantees.  The object returned via kmem_cache_alloc will
>> always be properly initialized.
> 
> Well I wouldn't say it's *required* just makes this particular piece
> of code neater, since we memset-zero the node's inode_item _anyways_.

But the rest of the delayed node still needs to be initialized. That's happening implicitly with your patch via zalloc but if anyone ever adds a new allocation from that cache, they'll need to know that zalloc is required for proper initializatiom. Documenting that is one approach. Constructors are another.

> I like the constructor idea though, do you suggest I should invest in
> that idea?

Well, see below.

>> 
>> This makes assumptions about atomic_t and what atomic_set does that
>> aren't guaranteed to be true.  When accessors/mutators are part of the
>> API they should be used.
>> 
>> - -Jeff
> 
> You're right, taking out that atomic_set was really stupid. I'll
> resent the patch with a proper explanation in the commit message and
> put the atomic_set back.
> 
> Unless you feel that the change is rather pointless, I'll gladly back
> off :-).

I don't want to dissuade new contributors but there's plenty of work to be done before we start worrying about cleaning initializers. These aren't hot paths (but if they were, the implicit memset of the whole object might have negative effects.) The immediate impact of cleanup patches like these is code churn so that developers with outstanding patch sets need to rebase on top of new context. It's a normal part of the development cycle but it's more work for not a lot of benefit.

So, if your goal is to contribute to btrfs, I'd suggest digging into the code. Try out changes. Break it and figure out how you broke it. It's like learning how to navigate in a new city: by getting lost you learn more. :)

-Jeff--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/