From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:54279 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752448AbdHDVH3 (ORCPT ); Fri, 4 Aug 2017 17:07:29 -0400 Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v74L3nve125603 for ; Fri, 4 Aug 2017 17:07:29 -0400 Received: from e23smtp03.au.ibm.com (e23smtp03.au.ibm.com [202.81.31.145]) by mx0a-001b2d01.pphosted.com with ESMTP id 2c4v6dc6rs-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 04 Aug 2017 17:07:28 -0400 Received: from localhost by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sat, 5 Aug 2017 07:07:26 +1000 Subject: Re: [PATCH v4 2/5] ima: use fs method to read integrity data [updated] From: Mimi Zohar To: Jan Kara Cc: Christoph Hellwig , Al Viro , James Morris , linux-fsdevel@vger.kernel.org, linux-ima-devel@lists.sourceforge.net, linux-security-module@vger.kernel.org, Matthew Garrett , Jan Kara , "Theodore Ts'o" , Andreas Dilger , Jaegeuk Kim , Chao Yu , Steven Whitehouse , Bob Peterson , David Woodhouse , Dave Kleikamp , Ryusuke Konishi , Mark Fasheh , Joel Becker , Richard Weinberger , "Darrick J. Wong" , Hugh Dickins , Chris Mason Date: Fri, 04 Aug 2017 17:07:11 -0400 In-Reply-To: <20170803105631.GB2579@quack2.suse.cz> References: <1501075375-29469-1-git-send-email-zohar@linux.vnet.ibm.com> <1501075375-29469-3-git-send-email-zohar@linux.vnet.ibm.com> <1501619070.3204.4.camel@linux.vnet.ibm.com> <20170802080108.GB2732@quack2.suse.cz> <1501693912.20268.29.camel@linux.vnet.ibm.com> <20170803105631.GB2579@quack2.suse.cz> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Message-Id: <1501880831.27872.153.camel@linux.vnet.ibm.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, 2017-08-03 at 12:56 +0200, Jan Kara wrote: > On Wed 02-08-17 13:11:52, Mimi Zohar wrote: > > On Wed, 2017-08-02 at 10:01 +0200, Jan Kara wrote: > > > On Tue 01-08-17 16:24:30, Mimi Zohar wrote: > > > > From: Christoph Hellwig > > > > > > > > Add a new ->integrity_read file operation to read data for integrity > > > > hash collection. This is defined to be equivalent to ->read_iter, > > > > except that it will be called with the i_rwsem held exclusively. > > > > > > > > Signed-off-by: Christoph Hellwig > > > > Cc: Matthew Garrett > > > > Cc: Jan Kara > > > > Cc: "Theodore Ts'o" > > > > Cc: Andreas Dilger > > > > Cc: Jaegeuk Kim > > > > Cc: Chao Yu > > > > Cc: Steven Whitehouse > > > > Cc: Bob Peterson > > > > Cc: David Woodhouse > > > > Cc: Dave Kleikamp > > > > Cc: Ryusuke Konishi > > > > Cc: Mark Fasheh > > > > Cc: Joel Becker > > > > Cc: Richard Weinberger > > > > Cc: "Darrick J. Wong" > > > > Cc: Hugh Dickins > > > > Cc: Chris Mason > > > > Signed-off-by: Mimi Zohar > > > > > > ... > > > > > > > +static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb, > > > > + struct iov_iter *to) > > > > +{ > > > > + struct inode *inode = file_inode(iocb->ki_filp); > > > > + int o_direct = iocb->ki_flags & IOCB_DIRECT; > > > > + > > > > + lockdep_assert_held(&inode->i_rwsem); > > > > + if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) > > > > + return -EIO; > > > > + > > > > + if (!iov_iter_count(to)) > > > > + return 0; /* skip atime */ > > > > + > > > > +#ifdef CONFIG_FS_DAX > > > > + if (IS_DAX(inode)) > > > > + return dax_iomap_rw(iocb, to, &ext4_iomap_ops); > > > > +#endif > > > > + if (o_direct) > > > > + return -EINVAL; > > > > + return generic_file_read_iter(iocb, to); > > > > +} > > > > > > I have noticed this o_direct check - why is it only in ext4 and shouldn't > > > rather higher layers make sure IOCB_DIRECT iocbs cannot reach > > > .integrity_read() methods? > > > > This failure happens when opening a file with O_DIRECT on a block > > device that does not support dax (eg. loop). xfs makes it to here too, > > but the call to generic_file_read_iter() fails properly with -EINVAL.  > > (Only tested on those filesystems included that support dax (eg. ext2, > > ext4, and xfs).) > > Well, yes, ext2 and ext4 will silently fall back to buffered read (as do > pretty much all filesystems except for XFS). However I fail to see why IMA > should care (which is probably due to my lack of knowledge about IMA). > Is O_DIRECT somehow excepted from IMA? But then why it is not for DAX? Thank you for the explanation!  (I was confused about the relationship between O_DIRECT and DAX.)  You're correct.  IMA does not support O_DIRECT in the buffered case for two reasons, locking and updating the file hash, which are described in commit f9b2a735bddd "ima: audit log files opened with O_DIRECT flag".  After reverting this commit, the O_DIRECT check is needed before calling generic_file_read_iter().  Most likely the same would need to be done for other filesystems that support O_DIRECT.  Probably a generic_integrity_file_read_iter() should be defined.   For DAX, unlike do_blockdev_direct_IO() which takes the lock, reading the file with O_DIRECT is fine, as dax_iomap_rw() only checks that the lock has been taken.  Assuming the file system is mounted with i_version, the file hash is updated properly. Mimi