linux-kernel - Re: [PATCH] fscrypt: Factor out bio specific functions

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: <20161216221428.GC14264@gmail.com>
Date:   Fri, 16 Dec 2016 14:14:28 -0800
From:   Eric Biggers <ebiggers3@...il.com>
To:     Richard Weinberger <richard@....at>
Cc:     David Gstir <david@...ma-star.at>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-kernel@...r.kernel.org, jaegeuk@...nel.org, tytso@....edu,
        hch@...radead.org, arnd@...db.de, dedekind1@...il.com,
        linux-mtd@...ts.infradead.org, adrian.hunter@...el.com,
        linux-ext4@...r.kernel.org, ebiggers@...gle.com,
        rdunlap@...radead.org
Subject: Re: [PATCH] fscrypt: Factor out bio specific functions

On Fri, Dec 16, 2016 at 09:48:19PM +0100, Richard Weinberger wrote:
> On 16.12.2016 16:37, David Gstir wrote:
> >> @@ -349,33 +347,10 @@ int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
> >> 		err = do_page_crypto(inode, FS_ENCRYPT, lblk,
> >> 					ZERO_PAGE(0), ciphertext_page,
> >> 					PAGE_SIZE, 0, GFP_NOFS);
> >> +		err = fscrypt_bio_submit_page(inode, pblk, ciphertext_page);
> > 
> > Any specific reason why you didn't just move the whole fscrypt_zeroout_range() to bio.c?
> 
> The function depends other internal functions of crypto.c which I didn't want to
> export.
> At the end of the day it's a matter of taste. I found it less ugly to keep
> fscrypt_zeroout_range() in crypto.c than exposing internal stuff.
> 

Hmm, it still seems weird to define fscrypt_zeroout_range() when it can't
actually be used.  It looks like the problem is specifically the use of
alloc_bounce_page() and do_page_crypto().  Would it be that bad to make those
available in fscrypt_internal.h (not exported to filesystems)?

Also, it seems the actual problem is that fscrypt_zeroout_range() tries to be
clever and reuse one bounce page over and over.  But this seems very inefficient
because it has to wait for each block to be synchronously written out before
moving on to the next.  I'm thinking it really should be updated to work more
like the normal write path, and then it could use fscrypt_encrypt_page()...

Eric