linux-ext4 - Re: [f2fs-dev] [PATCH v6 1/7] fscrypt: Add functions for direct I/O support

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200726024920.GB14321@sol.localdomain>
Date:   Sat, 25 Jul 2020 19:49:20 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Dave Chinner <david@...morbit.com>
Cc:     Satya Tangirala <satyat@...gle.com>,
        linux-f2fs-devel@...ts.sourceforge.net, linux-xfs@...r.kernel.org,
        linux-fscrypt@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-ext4@...r.kernel.org
Subject: Re: [f2fs-dev] [PATCH v6 1/7] fscrypt: Add functions for direct I/O
 support

On Sat, Jul 25, 2020 at 10:14:41AM +1000, Dave Chinner wrote:
> > +bool fscrypt_dio_supported(struct kiocb *iocb, struct iov_iter *iter)
> > +{
> > +	const struct inode *inode = file_inode(iocb->ki_filp);
> > +	const unsigned int blocksize = i_blocksize(inode);
> > +
> > +	/* If the file is unencrypted, no veto from us. */
> > +	if (!fscrypt_needs_contents_encryption(inode))
> > +		return true;
> > +
> > +	/* We only support direct I/O with inline crypto, not fs-layer crypto */
> > +	if (!fscrypt_inode_uses_inline_crypto(inode))
> > +		return false;
> > +
> > +	/*
> > +	 * Since the granularity of encryption is filesystem blocks, the I/O
> > +	 * must be block aligned -- not just disk sector aligned.
> > +	 */
> > +	if (!IS_ALIGNED(iocb->ki_pos | iov_iter_alignment(iter), blocksize))
> > +		return false;
> 
> Doesn't this force user buffers to be filesystem block size aligned,
> instead of 512 byte aligned as is typical for direct IO?
> 
> That's going to cause applications that work fine on normal
> filesystems becaues the memalign() buffers to 512 bytes or logical
> block device sector sizes (as per the open(2) man page) to fail on
> encrypted volumes, and it's not going to be obvious to users as to
> why this happens.

The status quo is that direct I/O on encrypted files falls back to buffered I/O.

So this patch is strictly an improvement; it's making direct I/O work in a case
where previously it didn't work.

> 
> XFS has XFS_IOC_DIOINFO to expose exactly this information to
> userspace on a per-file basis. Other filesystem and VFS developers
> have said for the past 15 years "we don't need no stinking DIOINFO".
> The same people shot down adding optional IO alignment
> constraint fields to statx() a few years ago, too.
> 
> Yet here were are again, with alignment of DIO buffers being an
> issue that userspace needs to know about....
> 

A DIOINFO ioctl sounds like a good idea to me, although I'm not familiar with
previous discussions about it.

Note that there are lots of other cases where ext4 and f2fs fall back to
buffered I/O; see ext4_dio_supported() and f2fs_force_buffered_io().  So this
isn't a new problem.

- Eric