From: Dave Chinner <david@fromorbit.com> To: Eric Biggers <ebiggers@kernel.org> Cc: Satya Tangirala <satyat@google.com>, linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org Subject: Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto Date: Fri, 24 Jul 2020 11:39:10 +1000 [thread overview] Message-ID: <20200724013910.GH2005@dread.disaster.area> (raw) In-Reply-To: <20200723230345.GB870@sol.localdomain> On Thu, Jul 23, 2020 at 04:03:45PM -0700, Eric Biggers wrote: > Hi Dave, > > On Fri, Jul 24, 2020 at 08:07:52AM +1000, Dave Chinner wrote: > > > > > @@ -183,11 +184,16 @@ static void > > > > > iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos, > > > > > unsigned len) > > > > > { > > > > > + struct inode *inode = file_inode(dio->iocb->ki_filp); > > > > > struct page *page = ZERO_PAGE(0); > > > > > int flags = REQ_SYNC | REQ_IDLE; > > > > > struct bio *bio; > > > > > > > > > > bio = bio_alloc(GFP_KERNEL, 1); > > > > > + > > > > > + /* encrypted direct I/O is guaranteed to be fs-block aligned */ > > > > > + WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode)); > > > > > > > > Which means you are now placing a new constraint on this code in > > > > that we cannot ever, in future, zero entire blocks here. > > > > > > > > This code can issue arbitrary sized zeroing bios - multiple entire fs blocks > > > > blocks if necessary - so I think constraining it to only support > > > > partial block zeroing by adding a warning like this is no correct. > > > > > > In v3 and earlier this instead had the code to set an encryption context: > > > > > > fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits, > > > GFP_KERNEL); > > > > > > Would you prefer that, even though the call to fscrypt_set_bio_crypt_ctx() would > > > > Actually, I have no idea what that function does. It's not in a > > 5.8-rc6 kernel, and it's not in this patchset.... > > The cover letter mentions that this patchset is based on fscrypt/master. Which the reader is left to work out where it exists. If you are going to say "based on", then a pointer to the actual tree like this: > That is, "master" of https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git is somewhat helpful. > fscrypt_set_bio_crypt_ctx() was introduced by > "fscrypt: add inline encryption support" on that branch. Way too much static inline function abstraction. fscrypt_inode_uses_inline_crypto() ends up being: if (IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode) && inode->i_crypt_info->ci_inlinecrypt) I note there are no checks for inode->i_crypt_info being non-null, and I note that S_ENCRYPTED is set on the inode when the on-disk encrypted flag is encountered, not when inode->i_crypt_info is set. Further, I note that the local variable for ci is fetched before fscrypt_inode_uses_inline_crypto() is run, so leaving a landmine for people who try to access ci before checking if inline crypto is enabled. Or, indeed, if S_ENCRYPTED is set and the crypt_info has not been set up, fscrypt_set_bio_crypt_ctx() will oops in the DIO path. > > > always be a no-op currently (since for now, iomap_dio_zero() will never be > > > called with an encrypted file) and thus wouldn't be properly tested? > > > > Same can be said for this WARN_ON_ONCE() code :) > > > > But, in the interests of not leaving landmines, if a fscrypt context > > is needed to be attached to the bio for data IO in direct IO, it > > should be attached to all bios that are allocated in the dio path > > rather than leave a landmine for people in future to trip over. > > My concern is that if we were to pass the wrong 'lblk' to > fscrypt_set_bio_crypt_ctx(), we wouldn't catch it because it's not tested. > Passing the wrong 'lblk' would cause the data to be encrypted/decrypted > incorrectly. When you implement sub-block DIO alignment for fscrypt enabled filesystems, fsx will tell you pretty quickly if you screwed up.... > Note that currently, I don't think iomap_dio_bio_actor() would handle an > encrypted file with blocksize > PAGE_SIZE correctly, as the I/O could be split > in the middle of a filesystem block (even after the filesystem ensures that > direct I/O on encrypted files is fully filesystem-block-aligned, which we do --- > see the rest of this patchset), which isn't allowed on encrypted files. That can already happen unless you've specifically restricted DIO alignments in the filesystem code. i.e. Direct IO already supports sub-block ranges and alignment, and we can already do user DIO on sub-block, sector aligned ranges just fine. And the filesystem can already split the iomap on sub-block alignments and ranges if it needs to because the iomap uses byte range addressing, not sector or block based addressing. So either you already have a situation where the 2^32 offset can land *inside* a filesystem block, or the offset is guaranteed to be filesystem block aligned and so you'll never get this "break an IO on sub-block alignment" problem regardless of the filesystem block size... Either way, it's not an iomap problem - it's a filesystem mapping problem... Cheers, Dave. -- Dave Chinner david@fromorbit.com
WARNING: multiple messages have this Message-ID (diff)
From: Dave Chinner <david@fromorbit.com> To: Eric Biggers <ebiggers@kernel.org> Cc: Satya Tangirala <satyat@google.com>, linux-f2fs-devel@lists.sourceforge.net, linux-xfs@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org Subject: Re: [f2fs-dev] [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto Date: Fri, 24 Jul 2020 11:39:10 +1000 [thread overview] Message-ID: <20200724013910.GH2005@dread.disaster.area> (raw) In-Reply-To: <20200723230345.GB870@sol.localdomain> On Thu, Jul 23, 2020 at 04:03:45PM -0700, Eric Biggers wrote: > Hi Dave, > > On Fri, Jul 24, 2020 at 08:07:52AM +1000, Dave Chinner wrote: > > > > > @@ -183,11 +184,16 @@ static void > > > > > iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos, > > > > > unsigned len) > > > > > { > > > > > + struct inode *inode = file_inode(dio->iocb->ki_filp); > > > > > struct page *page = ZERO_PAGE(0); > > > > > int flags = REQ_SYNC | REQ_IDLE; > > > > > struct bio *bio; > > > > > > > > > > bio = bio_alloc(GFP_KERNEL, 1); > > > > > + > > > > > + /* encrypted direct I/O is guaranteed to be fs-block aligned */ > > > > > + WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode)); > > > > > > > > Which means you are now placing a new constraint on this code in > > > > that we cannot ever, in future, zero entire blocks here. > > > > > > > > This code can issue arbitrary sized zeroing bios - multiple entire fs blocks > > > > blocks if necessary - so I think constraining it to only support > > > > partial block zeroing by adding a warning like this is no correct. > > > > > > In v3 and earlier this instead had the code to set an encryption context: > > > > > > fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits, > > > GFP_KERNEL); > > > > > > Would you prefer that, even though the call to fscrypt_set_bio_crypt_ctx() would > > > > Actually, I have no idea what that function does. It's not in a > > 5.8-rc6 kernel, and it's not in this patchset.... > > The cover letter mentions that this patchset is based on fscrypt/master. Which the reader is left to work out where it exists. If you are going to say "based on", then a pointer to the actual tree like this: > That is, "master" of https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git is somewhat helpful. > fscrypt_set_bio_crypt_ctx() was introduced by > "fscrypt: add inline encryption support" on that branch. Way too much static inline function abstraction. fscrypt_inode_uses_inline_crypto() ends up being: if (IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode) && inode->i_crypt_info->ci_inlinecrypt) I note there are no checks for inode->i_crypt_info being non-null, and I note that S_ENCRYPTED is set on the inode when the on-disk encrypted flag is encountered, not when inode->i_crypt_info is set. Further, I note that the local variable for ci is fetched before fscrypt_inode_uses_inline_crypto() is run, so leaving a landmine for people who try to access ci before checking if inline crypto is enabled. Or, indeed, if S_ENCRYPTED is set and the crypt_info has not been set up, fscrypt_set_bio_crypt_ctx() will oops in the DIO path. > > > always be a no-op currently (since for now, iomap_dio_zero() will never be > > > called with an encrypted file) and thus wouldn't be properly tested? > > > > Same can be said for this WARN_ON_ONCE() code :) > > > > But, in the interests of not leaving landmines, if a fscrypt context > > is needed to be attached to the bio for data IO in direct IO, it > > should be attached to all bios that are allocated in the dio path > > rather than leave a landmine for people in future to trip over. > > My concern is that if we were to pass the wrong 'lblk' to > fscrypt_set_bio_crypt_ctx(), we wouldn't catch it because it's not tested. > Passing the wrong 'lblk' would cause the data to be encrypted/decrypted > incorrectly. When you implement sub-block DIO alignment for fscrypt enabled filesystems, fsx will tell you pretty quickly if you screwed up.... > Note that currently, I don't think iomap_dio_bio_actor() would handle an > encrypted file with blocksize > PAGE_SIZE correctly, as the I/O could be split > in the middle of a filesystem block (even after the filesystem ensures that > direct I/O on encrypted files is fully filesystem-block-aligned, which we do --- > see the rest of this patchset), which isn't allowed on encrypted files. That can already happen unless you've specifically restricted DIO alignments in the filesystem code. i.e. Direct IO already supports sub-block ranges and alignment, and we can already do user DIO on sub-block, sector aligned ranges just fine. And the filesystem can already split the iomap on sub-block alignments and ranges if it needs to because the iomap uses byte range addressing, not sector or block based addressing. So either you already have a situation where the 2^32 offset can land *inside* a filesystem block, or the offset is guaranteed to be filesystem block aligned and so you'll never get this "break an IO on sub-block alignment" problem regardless of the filesystem block size... Either way, it's not an iomap problem - it's a filesystem mapping problem... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2020-07-24 1:39 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-20 23:37 [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-20 23:37 ` [PATCH v4 1/7] fscrypt: Add functions for direct I/O support Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-22 17:04 ` Jaegeuk Kim 2020-07-22 17:04 ` [f2fs-dev] " Jaegeuk Kim 2020-07-20 23:37 ` [PATCH v4 2/7] direct-io: add support for fscrypt using blk-crypto Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-22 17:05 ` Jaegeuk Kim 2020-07-22 17:05 ` [f2fs-dev] " Jaegeuk Kim 2020-07-20 23:37 ` [PATCH v4 3/7] iomap: support direct I/O with " Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-22 17:06 ` Jaegeuk Kim 2020-07-22 17:06 ` [f2fs-dev] " Jaegeuk Kim 2020-07-22 21:16 ` Dave Chinner 2020-07-22 21:16 ` [f2fs-dev] " Dave Chinner 2020-07-22 22:34 ` Eric Biggers 2020-07-22 22:34 ` [f2fs-dev] " Eric Biggers 2020-07-22 22:44 ` Matthew Wilcox 2020-07-22 22:44 ` [f2fs-dev] " Matthew Wilcox 2020-07-22 23:12 ` Eric Biggers 2020-07-22 23:12 ` [f2fs-dev] " Eric Biggers 2020-07-22 23:26 ` Eric Biggers 2020-07-22 23:26 ` [f2fs-dev] " Eric Biggers 2020-07-22 23:32 ` Darrick J. Wong 2020-07-22 23:32 ` [f2fs-dev] " Darrick J. Wong 2020-07-22 23:43 ` Eric Biggers 2020-07-22 23:43 ` [f2fs-dev] " Eric Biggers 2020-07-23 22:07 ` Dave Chinner 2020-07-23 22:07 ` [f2fs-dev] " Dave Chinner 2020-07-23 23:03 ` Eric Biggers 2020-07-23 23:03 ` [f2fs-dev] " Eric Biggers 2020-07-24 1:39 ` Dave Chinner [this message] 2020-07-24 1:39 ` Dave Chinner 2020-07-24 3:46 ` Eric Biggers 2020-07-24 3:46 ` [f2fs-dev] " Eric Biggers 2020-07-24 5:31 ` Dave Chinner 2020-07-24 5:31 ` [f2fs-dev] " Dave Chinner 2020-07-24 17:41 ` Eric Biggers 2020-07-24 17:41 ` [f2fs-dev] " Eric Biggers 2020-07-25 23:47 ` Dave Chinner 2020-07-25 23:47 ` [f2fs-dev] " Dave Chinner 2020-07-25 23:59 ` Dave Chinner 2020-07-25 23:59 ` [f2fs-dev] " Dave Chinner 2020-07-26 2:42 ` Eric Biggers 2020-07-26 2:42 ` [f2fs-dev] " Eric Biggers 2020-07-27 17:16 ` Eric Biggers 2020-07-27 17:16 ` [f2fs-dev] " Eric Biggers 2020-07-20 23:37 ` [PATCH v4 4/7] ext4: " Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-22 17:07 ` Jaegeuk Kim 2020-07-22 17:07 ` [f2fs-dev] " Jaegeuk Kim 2020-07-20 23:37 ` [PATCH v4 5/7] f2fs: " Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-21 20:11 ` Jaegeuk Kim 2020-07-21 20:11 ` [f2fs-dev] " Jaegeuk Kim 2020-07-20 23:37 ` [PATCH v4 6/7] fscrypt: document inline encryption support Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-22 17:01 ` Jaegeuk Kim 2020-07-22 17:01 ` [f2fs-dev] " Jaegeuk Kim 2020-07-20 23:37 ` [PATCH v4 7/7] fscrypt: update documentation for direct I/O support Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-21 0:47 ` Eric Biggers 2020-07-21 0:47 ` [f2fs-dev] " Eric Biggers 2020-07-22 16:57 ` Jaegeuk Kim 2020-07-22 16:57 ` [f2fs-dev] " Jaegeuk Kim 2020-07-21 0:56 ` [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Eric Biggers 2020-07-21 0:56 ` [f2fs-dev] " Eric Biggers
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200724013910.GH2005@dread.disaster.area \ --to=david@fromorbit.com \ --cc=ebiggers@kernel.org \ --cc=linux-ext4@vger.kernel.org \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-fscrypt@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-xfs@vger.kernel.org \ --cc=satyat@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.