Hi,
We are looking for advice on reading files opened for direct_io.
IMA subsystem (security/integrity/ima) reads file content to kernel
buffer with kernel_read() like function to calculate a file hash.
It does not open another instance of 'struct file' but uses one
allocated via 'open' system call.
It works well when file was opened without O_DIRECT flag. In such case
file content is read to the page cache and VFS simply copying data to
the buffer. When buffer is a kernel buffer, it is successfully manged like:
old_fs = get_fs();
set_fs(get_ds());
result = vfs_read(file, (void __user *)addr, count, &pos);
set_fs(old_fs);
In the case when file was opened with O_DIRECT flag, filesystem code
copying data directly to user-space memory and set_fs(get_ds()) trick
does not work anymore.
To overcome this problem we thought about following possible solutions.
1. Allocate user-space memory with vm_mmap().
It works when when file is opened. current->mm is there...
But IMA has certain case to re-read the file at file close.
If file was not explicitly closed with 'close', it will be closed at
process termination somewhere in do_exit().
But at the time file is closing from there, 'current->mm' has already
gone and vm_mmap() fails.
2. Temporarily clear O_DIRECT in file->f_flags.
This is how it looked like in the proposed patch some time ago...
https://lkml.org/lkml/2013/2/20/601
In such approach, by clearing a flag, VFS would be able to write data to
kernel buffer.
But in such case we force to populate page cache which was not an
intention of the process using O_DIRECT.
There reason for the patch was actually a deadlock, because IMA takes
i_mutex and VFS direct_io code also took it.
Al Viro rejected the patch as from his opinion we contaminated locking
rules.
Now we will introduce internal locking and deadlock would not happen
anyway and temporarily clearing O_DIRECT is not to workaround locking
but to be able to read to kernel buffer.
3. Open another instance of the file with 'dentry_open'
Such case would be also similar to temporarily clearing as we would
populate the page cache...
Yeah. O_DIRECT use is certainly rare.. 'man 2 open' has interesting
statement from Linus... :)
"The thing that has always disturbed me about O_DIRECT is that the whole
interface is just stupid, and was probably designed by a deranged monkey
on some serious mind-controlling substances."?Linus
But anyway, there was few complains about deadlock with IMA+O_DIRECT.
Currently we made a patch that when IMA is enabled it does not
measure/appraise files opened with O_DIRECT. Instead it can be
configured to block and log or allow and log O_DIRECT access...
But we would want to cover O_DIRECT case completely and be able to read
file..
Greg KH advised me to write to linux-mm and Andrew as well and ask about
advices on possibility to handle O_DIRECT files.
Is temporarily clearing O_DIRECT flag really unacceptable or not?
Or may be there is a way to allocate "special" user-space like buffer
for kernel and use it with O_DIRECT?
Thanks,
Dmitry
Hi, Dmitry,
Dmitry Kasatkin <[email protected]> writes:
> Hi,
>
> We are looking for advice on reading files opened for direct_io.
[snip]
> 2. Temporarily clear O_DIRECT in file->f_flags.
[snip]
> 3. Open another instance of the file with 'dentry_open'
[snip]
> Is temporarily clearing O_DIRECT flag really unacceptable or not?
It's acceptable. However, what you're proposing to do is read the
entire file into the page cache to calculate your checksum. Then, when
the application goes to read the file using O_DIRECT, it will ignore the
cached copy and re-read the portions of the file it wants from disk. So
yes, you can do that, but it's not going to be fast. If you want to
avoid polluting the cache, you can call invalidate_inode_pages2_range
after you're done calculating your checksum.
> Or may be there is a way to allocate "special" user-space like buffer
> for kernel and use it with O_DIRECT?
In-kernel O_DIRECT support has been proposed in the past, but there is
no solution for that yet.
Cheers,
Jeff
Hi Jeff,
Thanks for reply.
On 2 July 2014 18:55, Jeff Moyer <[email protected]> wrote:
> Hi, Dmitry,
>
> Dmitry Kasatkin <[email protected]> writes:
>
>> Hi,
>>
>> We are looking for advice on reading files opened for direct_io.
>
> [snip]
>
>> 2. Temporarily clear O_DIRECT in file->f_flags.
>
> [snip]
>
>> 3. Open another instance of the file with 'dentry_open'
>
> [snip]
>
>> Is temporarily clearing O_DIRECT flag really unacceptable or not?
>
> It's acceptable. However, what you're proposing to do is read the
> entire file into the page cache to calculate your checksum. Then, when
> the application goes to read the file using O_DIRECT, it will ignore the
> cached copy and re-read the portions of the file it wants from disk. So
> yes, you can do that, but it's not going to be fast. If you want to
> avoid polluting the cache, you can call invalidate_inode_pages2_range
> after you're done calculating your checksum.
>
Ok. If I understand correctly, after reading chunck/range like
kernel_read(offset, len),
just always drop loaded pages like
invalidate_inode_pages2_range(inode->i_mapping, offset, offset + len);
I see that generic_file_direct_write() calls this function too, before
doing direct IO and after...
Thanks!
>> Or may be there is a way to allocate "special" user-space like buffer
>> for kernel and use it with O_DIRECT?
>
> In-kernel O_DIRECT support has been proposed in the past, but there is
> no solution for that yet.
>
> Cheers,
> Jeff
--
Thanks,
Dmitry
On Wed, Jul 02, 2014 at 11:55:41AM -0400, Jeff Moyer wrote:
> It's acceptable.
It's not because it will then also affect other reads going on at the
same time.
The whole concept of ima is just broken, and if you want to do these
sort of verification they need to happen inside the filesystem and not
above it.
We really should never have merged ima, and I think we should leave
these sorts of issue that have been there since day one unfixed and
deprecate it instead of adding workaround on top of workaround.
Christoph Hellwig <[email protected]> writes:
> On Wed, Jul 02, 2014 at 11:55:41AM -0400, Jeff Moyer wrote:
>> It's acceptable.
>
> It's not because it will then also affect other reads going on at the
> same time.
OK, that part I was fuzzy on. I wasn't sure if they were preventing
other reads/writes to the same file somehow. I should have mentioned
that.
Cheers,
Jeff
On 2 July 2014 21:45, Jeff Moyer <[email protected]> wrote:
> Christoph Hellwig <[email protected]> writes:
>
>> On Wed, Jul 02, 2014 at 11:55:41AM -0400, Jeff Moyer wrote:
>>> It's acceptable.
>>
>> It's not because it will then also affect other reads going on at the
>> same time.
>
> OK, that part I was fuzzy on. I wasn't sure if they were preventing
> other reads/writes to the same file somehow. I should have mentioned
> that.
>
> Cheers,
> Jeff
What Christoph says is not very correct.
At open there cannot be any reads going on at the same time. IMA
reading is guarded by mutex. Following opens do not perform any IMA
readings and do not do what he says...
If file was modified with direct-io, VFS code itself always invalidate
pages before and after any write. It is basically what Christoph says.
But that is not IMA problem but direct-io itself. As it is stupid
interface. I would be more looking to kind of fadvise interface to
control amount of page caching...
So I think what Jeff suggest suites well to IMA.
--
Thanks,
Dmitry
On Wed 2014-07-02 11:40:50, Christoph Hellwig wrote:
> On Wed, Jul 02, 2014 at 11:55:41AM -0400, Jeff Moyer wrote:
> > It's acceptable.
>
> It's not because it will then also affect other reads going on at the
> same time.
>
> The whole concept of ima is just broken, and if you want to do these
> sort of verification they need to happen inside the filesystem and not
> above it.
...and doing it at filesystem layer would also permit verification of
per-block (64KB? 1MB?) hashes. Reading entire iso image when I run
"file foo.iso" is anti-social..
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 11 July 2014 23:10, Pavel Machek <[email protected]> wrote:
> On Wed 2014-07-02 11:40:50, Christoph Hellwig wrote:
>> On Wed, Jul 02, 2014 at 11:55:41AM -0400, Jeff Moyer wrote:
>> > It's acceptable.
>>
>> It's not because it will then also affect other reads going on at the
>> same time.
>>
>> The whole concept of ima is just broken, and if you want to do these
>> sort of verification they need to happen inside the filesystem and not
>> above it.
>
> ...and doing it at filesystem layer would also permit verification of
> per-block (64KB? 1MB?) hashes.
Please design one single and the best universal filesystem which does it.
> Reading entire iso image when I run
> "file foo.iso" is anti-social..
> Pavel
Please make the policy which does not make anti-social.
It is all about use-case.
- Dmitry
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat 2014-07-12 01:22:04, Dmitry Kasatkin wrote:
> On 11 July 2014 23:10, Pavel Machek <[email protected]> wrote:
> > On Wed 2014-07-02 11:40:50, Christoph Hellwig wrote:
> >> On Wed, Jul 02, 2014 at 11:55:41AM -0400, Jeff Moyer wrote:
> >> > It's acceptable.
> >>
> >> It's not because it will then also affect other reads going on at the
> >> same time.
> >>
> >> The whole concept of ima is just broken, and if you want to do these
> >> sort of verification they need to happen inside the filesystem and not
> >> above it.
> >
> > ...and doing it at filesystem layer would also permit verification of
> > per-block (64KB? 1MB?) hashes.
>
> Please design one single and the best universal filesystem which
> does it.
Given the overhead whole-file hashing has, you don't need single best
operating system. All you need it either ext4 or btrfs.. depending on
when you want it in production.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, 2014-07-15 at 15:03 +0200, Pavel Machek wrote:
> On Sat 2014-07-12 01:22:04, Dmitry Kasatkin wrote:
> > On 11 July 2014 23:10, Pavel Machek <[email protected]> wrote:
> > > On Wed 2014-07-02 11:40:50, Christoph Hellwig wrote:
> > >> On Wed, Jul 02, 2014 at 11:55:41AM -0400, Jeff Moyer wrote:
> > >> > It's acceptable.
> > >>
> > >> It's not because it will then also affect other reads going on at the
> > >> same time.
> > >>
> > >> The whole concept of ima is just broken, and if you want to do these
> > >> sort of verification they need to happen inside the filesystem and not
> > >> above it.
Agreed, maintaining the file's integrity hash should be done at the
filesystem layer. IMA would then be relegated to using the integrity
information to maintain the measurement list and enforce local file
integrity.
> > > ...and doing it at filesystem layer would also permit verification of
> > > per-block (64KB? 1MB?) hashes.
> >
> > Please design one single and the best universal filesystem which
> > does it.
>
> Given the overhead whole-file hashing has, you don't need single best
> operating system. All you need it either ext4 or btrfs.. depending on
> when you want it in production.
Mike Halcrow will be leading a discussion on EXT4 Encryption at LSS
http://kernsec.org/wiki/index.php/Linux_Security_Summit_2014/Abstracts/Halcrow.
One of the discussion topics will be the storage of file metadata
integrity. (Lukas Czerner's work.) Hope you'll be able to attend.
Mimi