linux-ext4 - Re: [PATCH 03/11] ext4: Convert DAX reads to iomap infrastructure

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]

Date:   Fri, 11 Nov 2016 11:17:51 +0100
From:   Jan Kara <jack@...e.cz>
To:     Ross Zwisler <ross.zwisler@...ux.intel.com>
Cc:     Jan Kara <jack@...e.cz>, Ted Tso <tytso@....edu>,
        linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH 03/11] ext4: Convert DAX reads to iomap infrastructure

On Thu 10-11-16 14:54:31, Ross Zwisler wrote:
> On Tue, Nov 08, 2016 at 12:08:09PM +0100, Jan Kara wrote:
> > Implement basic iomap_begin function that handles reading and use it for
> > DAX reads.
> > 
> > Signed-off-by: Jan Kara <jack@...e.cz>
> > ---
> >  fs/ext4/ext4.h  |  2 ++
> >  fs/ext4/file.c  | 38 +++++++++++++++++++++++++++++++++++++-
> >  fs/ext4/inode.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 93 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > index 282a51b07c57..098b39910001 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -3271,6 +3271,8 @@ static inline bool ext4_aligned_io(struct inode *inode, loff_t off, loff_t len)
> >  	return IS_ALIGNED(off, blksize) && IS_ALIGNED(len, blksize);
> >  }
> >  
> > +extern struct iomap_ops ext4_iomap_ops;
> > +
> >  #endif	/* __KERNEL__ */
> >  
> >  #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
> > diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> > index 9facb4dc5c70..1f25c644cb12 100644
> > --- a/fs/ext4/file.c
> > +++ b/fs/ext4/file.c
> > @@ -31,6 +31,42 @@
> >  #include "xattr.h"
> >  #include "acl.h"
> >  
> > +#ifdef CONFIG_FS_DAX
> > +static ssize_t ext4_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > +{
> > +	struct inode *inode = file_inode(iocb->ki_filp);
> > +	ssize_t ret;
> > +
> > +	inode_lock_shared(inode);
> > +	/*
> > +	 * Recheck under inode lock - at this point we are sure it cannot
> > +	 * change anymore
> > +	 */
> > +	if (!IS_DAX(inode)) {
> > +		inode_unlock_shared(inode);
> > +		/* Fallback to buffered IO in case we cannot support DAX */
> > +		return generic_file_read_iter(iocb, to);
> 
> Is this not also racy, since we've just dropped the inode lock?  What's to
> prevent this sequence?
> 
> Thread 0				Thread 1
> --------				--------
> ext4_file_read_iter()
>   IS_DAX() returns true
>   					changes S_DAX to false
>   ext4_dax_read_iter()
>     inode_lock_shared()
>     IS_DAX() returns false
>     inode_unlock_shared()
>   					changes S_DAX to true
>     generic_file_read_iter() on a DAX inode
> 
> 
> Or are we okay in this scenario?

Yup, I'm aware of this. The real problem is that there's no way to
serialize with buffered reads for ext4 (they take only page locks) so
currently you can have buffered reads in flight when inode gets switched to
DAX mode. I agree there is a potential for breakage and it needs to be
resolved eventually but the problem is not new and these patches don't make
it really any worse so I just somewhat fixed it up by patch 2/11 and left
full solution to a separate patch set.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR
--
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