IMA: kernel reading files opened with O_DIRECT

DISCUSSION Dmitry Kasatkin 6 participants 10 messages Jul 2, 2014 – Jul 16, 2014

Subsystems (2)

Memory management VFS

lore

Share & tools

lore

Dmitry Kasatkin

author

2014-07-02 09:40 UTC #0 lore raw

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Jeff Moyer

2014-07-02 15:55 UTC #1 lore raw

Hi, Dmitry,

Quoted 3 lines · Dmitry Kasatkin

Hi,
We are looking for advice on reading files opened for direct_io.

[snip]

Quoted 1 line

2. Temporarily clear O_DIRECT in file->f_flags.

[snip]

Quoted 1 line

3. Open another instance of the file with 'dentry_open'

[snip]

Quoted 1 line

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.

Quoted 2 lines

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

Dmitry Kasatkin

2014-07-02 18:12 UTC #2 lore raw

Hi Jeff,

Thanks for reply.

Quoted 20 lines · Jeff Moyer

Hi, Dmitry,
Quoted 3 lines · Dmitry Kasatkin
Hi,
We are looking for advice on reading files opened for direct_io.
[snip]
Quoted 1 line
2. Temporarily clear O_DIRECT in file->f_flags.
[snip]
Quoted 1 line
3. Open another instance of the file with 'dentry_open'
[snip]
Quoted 1 line
Is temporarily clearing O_DIRECT flag really unacceptable or not?

Quoted 8 lines

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!

Quoted 8 lines

Quoted 2 lines
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

Signature · 8 lines

Thanks,
Dmitry

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Hellwig

2014-07-02 18:40 UTC #3 lore raw

In reply to Jeff Moyer

Quoted 1 line · Jeff Moyer

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.

Jeff Moyer

2014-07-02 18:45 UTC #4 lore raw

Quoted 5 lines · Christoph Hellwig

Quoted 1 line · Jeff Moyer
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

Dmitry Kasatkin

2014-07-02 19:07 UTC #5 lore raw

Quoted 9 lines · Jeff Moyer

Quoted 5 lines · Christoph Hellwig
Quoted 1 line · Jeff Moyer
It's acceptable.
It's not because it will then also affect other reads going on at the same time.

Quoted 6 lines

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.

Signature · 2 lines

Thanks,
Dmitry

Pavel Machek

2014-07-11 20:10 UTC #6 lore raw

In reply to Christoph Hellwig

Quoted 9 lines · Christoph Hellwig

Quoted 1 line · Jeff Moyer
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

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Dmitry Kasatkin

2014-07-11 22:22 UTC #7 lore raw

Quoted 7 lines · Pavel Machek

Quoted 6 lines · Christoph Hellwig
Quoted 1 line · Jeff Moyer
It's acceptable.
It's not because it will then also affect other reads going on at the same time.

Quoted 6 lines

Quoted 3 lines
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.

Quoted 3 lines

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

Quoted 3 lines

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Pavel Machek

2014-07-15 13:03 UTC #8 lore raw

Quoted 12 lines · Dmitry Kasatkin

Quoted 11 lines · Pavel Machek
Quoted 9 lines · Christoph Hellwig
Quoted 1 line · Jeff Moyer
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.

Quoted 5 lines

Quoted 2 lines
...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

Signature · 8 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mimi Zohar

2014-07-16 12:48 UTC #9 lore raw

Quoted 4 lines · Pavel Machek

Quoted 3 lines · Dmitry Kasatkin
Quoted 2 lines · Pavel Machek
Quoted 1 line · Christoph Hellwig
On Wed, Jul 02, 2014 at 11:55:41AM -0400, Jeff Moyer wrote:

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
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.

Quoted 9 lines

Quoted 5 lines
Quoted 2 lines
...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