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: <20150411133839.GH6540@thunk.org>
Date:	Sat, 11 Apr 2015 09:38:39 -0400
From:	Theodore Ts'o <tytso@....edu>
To:	Andreas Dilger <adilger@...ger.ca>
Cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>,
	jaegeuk@...nel.org, mhalcrow@...gle.com,
	Ildar Muslukhov <ildarm@...gle.com>
Subject: Re: [PATCH 13/22] ext4 crypto: implement the ext4 decryption read
 path

On Wed, Apr 08, 2015 at 12:51:04PM -0600, Andreas Dilger wrote:
> > @@ -200,9 +201,15 @@ static const struct vm_operations_struct ext4_file_vm_ops = {
> > 
> > static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
> > {
> > +	int res = 0;
> 
> This function is using "res", the one below "ret".  It would be nice to
> have some consistency.  "rc" is probably the most common and would be
> my preference for new/modified code.

Good point; I've restructured this code to make it simpler (and to
avoid conflicting with the DAX patches).  So it now looks like this:

	if (ext4_encrypted_inode(inode)) {
		int err = ext4_generate_encryption_key(inode);
		if (err)
			return 0;
	}

(I'm not a fan of rc, myself, but this makes the declaration and use
of the function much closer together than what we had before, which
makes it much easier to understand.  I'm guessing Microsoft had a
coding policy which stated that all functions have to only have one
return function at the end of the function, since I've noticed that
both Ildar and Michael seem to code that way, even when it requires
using goto statements and/or making the code less clear.)

> > +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> > +		if (S_ISREG(inode->i_mode) &&
> > +		    ext4_encrypted_inode(inode)) {
> > +			/* We expect the key to be set. */
> > +			BUG_ON(!ext4_has_encryption_key(inode));
> > +			BUG_ON(blocksize != PAGE_CACHE_SIZE);
> > +			WARN_ON_ONCE(ext4_decrypt_one(inode, page));
> > +		}
> > +#endif
> 
> This could avoid the #ifdef since ext4_encrypted_inode() is declared (0)
> in the !CONFIG_EXT4_FS_ENCRYPTION case.  The compiler will optimize out
> the resulting unreachable code in that case and make this code easier
> to read.

Good point, done.

> If the (bio->bi_private != NULL) check was moved to a helper function:
> 
> static inline bool ext4_bio_encrypted(struct bio *bio)
> {
> #ifdef CONFIG_EXT4_FS_ENCRYPTION
> 	return unlikely(bio->bi_private != NULL);
> #else
> 	return false;
> #endif
> }
> 
> Then the inline #ifdefs could be removed here, since the resulting

Sure, that makes the code a bit more readable, thanks.

> > +		if (err)
> > +			ext4_release_crypto_ctx(ctx);
> > +		else {
> 
> The if/else should have matching {} blocks.

Meh, I don't really think it's that important, but sure.

> > @@ -94,9 +137,15 @@ int ext4_mpage_readpages(struct address_space *mapping,
> > 	unsigned page_block;
> > 	struct block_device *bdev = inode->i_sb->s_bdev;
> > 	int length;
> > +	int do_decryption = 0;
> 
> Can be bool instead of int.

Actually, since we only use do_decryption in one place now, I'll just remove this and this:

> > +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> > +	if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode))
> > +		do_decryption = 1;
> > +#endif
> #ifdef can be removed.

And move the condition to the one place where we test for do_decryption.

Thanks,

					- Ted

    
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ