xfs: commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" change causes hang

IMA takes the i_rwsem (fomerly i_mutex) before reading the file to synchronize calculating the file hash and validating the file's hash/signature stored as security.ima xattr

Quoted 3 lines · Mimi Zohar

IMA takes the i_rwsem (fomerly i_mutex) before reading the file to synchronize calculating the file hash and validating the file's hash/signature stored as security.ima xattr

Well, it shouldn't do that. In the I/O path i_rwsem is up to the fs to use. Various other file systems also take it internally for reads, although mostly only for direct I/O.

So the answer here is that ima needs to stop playing with i_rwsem.

But not normally for a normal file read.

Unless something has changed recently, to synchronize reading files to calculate the file hash and writing xattrs it has to take the i_rwsem prior to reading the file.

Quoted 1 line · Mimi Zohar

But not normally for a normal file read.

Depends on the file system. In addition to XFS at least the NFS also uses i_rwsem by default. Also all file systems supporting a DAX I/O path.

Quoted 3 lines

Unless something has changed recently, to synchronize reading files to calculate the file hash and writing xattrs it has to take the i_rwsem prior to reading the file.

No, you must simply not do this at all. If you take a lock that belongs to the fs and is not your own over ->read_iter you're toast as you've seen.

Quoted 3 lines

Depends on the file system. In addition to XFS at least the NFS also uses i_rwsem by default. Also all file systems supporting a DAX I/O path.

We're only interested in the integrity of the local file system.

IMA needs a mechanism for quickly reading a file to calculate the file hash and validate (or set) the file signature/hash stored as an xattr, prior to any other process getting access to the file.

Quoted 5 lines · Mimi Zohar

Quoted 3 lines

Depends on the file system. In addition to XFS at least the NFS also uses i_rwsem by default. Also all file systems supporting a DAX I/O path.

We're only interested in the integrity of the local file system.

And I'm only interested in supporting sane use cases that don't include IMA.

But even if you only care about local file systems IMA is simply fundamentally broken. Read/Write zerialization in Linux has always been in the fs (or generic library called by it) and not by the caller.

Quoted 3 lines

IMA needs a mechanism for quickly reading a file to calculate the file hash and validate (or set) the file signature/hash stored as an xattr, prior to any other process getting access to the file.

And that's simply not doable in Linux from a layer outside the file system.
And really questionable even from inside the fs. We should simply drop
IMA from the tree in the current state because it's obviously broken.

If you can convince fs maintainers to do the right calls from the file systems itself for given file systems (and only support IMA for those) we can add it back just for those.

We definitely need VFS help in resolving this regression. As a last resort, we could always update the default IMA builtin policy to exclude XFS.

Quoted 3 lines · Mimi Zohar

IMA takes the i_rwsem (fomerly i_mutex) before reading the file to synchronize calculating the file hash and validating the file's hash/signature stored as security.ima xattr

Well, it shouldn't do that. In the I/O path i_rwsem is up to the fs to use. Various other file systems also take it internally for reads, although mostly only for direct I/O.

So the answer here is that ima needs to stop playing with i_rwsem.

Hey, that's not really true: the inode lock (i_rwsem) is used in all sorts of generic places, including generic_file_write_iter(). That's, I think, why ima is using it to try to prevent writes while it measures the file.

Quoted 2 lines

So the answer here is that ima needs to stop playing with i_rwsem.

Isn't there a happy medium? most sensible filesystems will allow shared reading (unless they want to tank performance) so we can rely on the fact that even if a fs does use i_rwsem internally on the read path, it will have to be shared.

So simply replacing the inode_lock() in ima
with inode_lock_shared() should do what ima wants and not interact badly even if the underlying FS uses i_rwsem. If there's ever a FS that takes it exclusively in the read path, ima can simply blacklist it.

Quoted 4 lines · James Bottomley

Hey, that's not really true: the inode lock (i_rwsem) is used in all sorts of generic places, including generic_file_write_iter().
That's, I think, why ima is using it to try to prevent writes
while it measures the file.

But all these are _below_ file_operations. The only place where take them in the VFS is for namespace locking, e.g. before calling into inode_operations (to generalize a little).

Quoted 7 lines

Quoted 2 lines

So the answer here is that ima needs to stop playing with i_rwsem.

Isn't there a happy medium? most sensible filesystems will allow shared reading (unless they want to tank performance) so we can rely on the fact that even if a fs does use i_rwsem internally on the read path, it will have to be shared.

At least for direct I/O that doesn't always have to be true.

Quoted 6 lines

So simply replacing the inode_lock() in ima
with inode_lock_shared() should do what ima wants and not interact badly even if the underlying FS uses i_rwsem. If there's ever a FS that takes it exclusively in the read path, ima can simply blacklist
it.

IFF we actually allow recursive readers for rw_semaphores this would work around the issue (but I'm not sure about that fact, at least in the past we didn't). It won't fix IMA for all the file systems use other synchronization for reads, e.g. the cluster locks in ocfs2 or gfs2. It won't fix NFS which will exhibit exacly the same issue as Mimi reported.

Last but not least it won't solve the problem that IMA has never been designed and does neither document the requires it has from a file system, nor is there any systematic testing for it. It will keep on breaking because it has all kinds of weird implicit assumptions never written down or verified, and the test coverage for it is basically non-existing.

I'm unsure about the DIO case, so lets try defining the semantics and see if they're implementable for DIO, otherwise simply exclude it.

OK, so how about we define it. I think we need two vfs calls:

inode_block_local_writes(inode)
inode_unblock_local_writes(inode)

With semantics that between these two, all write attempts to the file backed by the inode on this system block but reads of the underlying file are allowed (I added local so we don't have to implement for remote filesystems).

No. We need an ->ima_measure file_operation, guts of process_measurement turned into a library function that the FS can call after taking fs-specific locks. And maybe also a small wrapper around it that takes ilock and can be used directly for file systems not needing special locking.

Quoted 4 lines · Christoph Hellwig

No. We need an ->ima_measure file_operation, guts of process_measurement turned into a library function that the FS can call after taking fs-specific locks. And maybe also a small wrapper around it that takes ilock and can be used directly for file systems not needing special locking.

Incidentally, it had been literally years since the problems with their pathname handling had been brought up and we *still* have got no answer.

In the current tree, ima_d_path() is quite capable of returning path->dentry->d_name.name. Which gets used by subsequent code, even though there is no warranty whatsoever that it won't be pointing to freed memory by the time the caller of ima_d_path() gets it.

Quoted 2 lines · James Bottomley

I'm unsure about the DIO case, so lets try defining the semantics and see if they're implementable for DIO, otherwise simply exclude it.

Let's start with the semantics. First we need to write down what IMA requires from the FS, and have an interface how the FS can declare that it supports these features. As far as I can tell there are not proper feature checks anywhere right now. Once we have done that we can move forward from there.

As you seem to be interested in IMA how about you spearhead documenting the requirements and adding xfstests support?

Quoted 4 lines

OK, so how about we define it. I think we need two vfs calls:

inode_block_local_writes(inode)
inode_unblock_local_writes(inode)

No. We need an ->ima_measure file_operation, guts of process_measurement turned into a library function that the FS can call after taking fs-specific locks. And maybe also a small wrapper around it that takes ilock and can be used directly for file systems not needing special locking.

Quoted 4 lines

With semantics that between these two, all write attempts to the file backed by the inode on this system block but reads of the underlying file are allowed (I added local so we don't have to implement for remote filesystems).

How do you define local? Are GFS2 and OCFS2 local? Is XFS with outstanding pNFS layout local? Is NFS with the block or SCSI layout local because it operates on a block device?

The only sane way is to make INA opt-in with a check list of features that need to be supported, and declared to be supported by the fs, similar to how we handle NFS exporting.

Quoted 3 lines · James Bottomley

I'm unsure about the DIO case, so lets try defining the semantics and see if they're implementable for DIO, otherwise simply exclude it.

Let's start with the semantics. First we need to write down what IMA requires from the FS, and have an interface how the FS can declare that it supports these features. As far as I can tell there are not proper feature checks anywhere right now. Once we have done that we can move forward from there.

As you seem to be interested in IMA how about you spearhead documenting the requirements and adding xfstests support?

Quoted 5 lines

OK, so how about we define it. I think we need two vfs calls:

inode_block_local_writes(inode)
inode_unblock_local_writes(inode)

No. We need an ->ima_measure file_operation, guts of process_measurement turned into a library function that the FS can call after taking fs-specific locks. And maybe also a small wrapper around it that takes ilock and can be used directly for file systems not needing special locking.

Quoted 5 lines

With semantics that between these two, all write attempts to the file backed by the inode on this system block but reads of the underlying file are allowed (I added local so we don't have to implement for remote filesystems).

How do you define local? Are GFS2 and OCFS2 local? Is XFS with outstanding pNFS layout local? Is NFS with the block or SCSI layout local because it operates on a block device?

The only sane way is to make INA opt-in with a check list of features that need to be supported, and declared to be supported by the fs, similar to how we handle NFS exporting.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines · James Bottomley

I'm unsure about the DIO case, so lets try defining the semantics and see if they're implementable for DIO, otherwise simply exclude it.

Quoted 9 lines

Let's start with the semantics. First we need to write down what IMA requires from the FS, and have an interface how the FS can declare that it supports these features. As far as I can tell there are not proper feature checks anywhere right now. Once we have done that we can move forward from there.

As you seem to be interested in IMA how about you spearhead documenting the requirements and adding xfstests support?

Another datapoint here:

While doing the i_version rework patches, I noticed that IMA depends heavily on the filesystem correctly implementing the i_version counter, but that's only reliable for filesystems that set the MS_I_VERSION flag.

I see nowhere that IMA actually checks that that flag is set, so you can conceivably turn it on on filesystems that don't implement it correctly (or just have it turned off like ext4 defaults to) and never notice that your monitored file has changed.

Documenting the VFS and fs driver requirements for IMA would be a good way to start fixing some of these problems.

Quoted 28 lines

Quoted 5 lines

OK, so how about we define it. I think we need two vfs calls:

inode_block_local_writes(inode)
inode_unblock_local_writes(inode)

No. We need an ->ima_measure file_operation, guts of process_measurement turned into a library function that the FS can call after taking fs-specific locks. And maybe also a small wrapper around it that takes ilock and can be used directly for file systems not needing special locking.

Quoted 5 lines

With semantics that between these two, all write attempts to the file backed by the inode on this system block but reads of the underlying file are allowed (I added local so we don't have to implement for remote filesystems).

How do you define local? Are GFS2 and OCFS2 local? Is XFS with outstanding pNFS layout local? Is NFS with the block or SCSI layout local because it operates on a block device?

The only sane way is to make INA opt-in with a check list of features that need to be supported, and declared to be supported by the fs, similar to how we handle NFS exporting.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 25 lines · Jeff Layton

Quoted 7 lines · Christoph Hellwig

Quoted 4 lines · James Bottomley

I'm unsure about the DIO case, so lets try defining the semantics and see if they're implementable for DIO, otherwise simply exclude it.

Quoted 9 lines

Let's start with the semantics. First we need to write down what IMA requires from the FS, and have an interface how the FS can declare that it supports these features. As far as I can tell there are not proper feature checks anywhere right now. Once we have done that we can move forward from there.

As you seem to be interested in IMA how about you spearhead documenting the requirements and adding xfstests support?

Another datapoint here:

While doing the i_version rework patches, I noticed that IMA depends heavily on the filesystem correctly implementing the i_version counter, but that's only reliable for filesystems that set the MS_I_VERSION flag.

Quoted 4 lines

I see nowhere that IMA actually checks that that flag is set, so you can conceivably turn it on on filesystems that don't implement it correctly (or just have it turned off like ext4 defaults to) and never notice that your monitored file has changed.

Yes, the filesystem does need to be mounted with i_version for iMA to detect file changes.

Quoted 3 lines

Documenting the VFS and fs driver requirements for IMA would be a good way to start fixing some of these problems.

Agreed.

Mimi

Quoted 33 lines

Quoted 32 lines

Quoted 8 lines

OK, so how about we define it. I think we need two vfs calls:

inode_block_local_writes(inode)
inode_unblock_local_writes(inode)

No. We need an ->ima_measure file_operation, guts of process_measurement turned into a library function that the FS can call after taking fs-specific locks. And maybe also a small wrapper around it that takes ilock and can be used directly for file systems not needing special locking.

Quoted 6 lines

With semantics that between these two, all write attempts to the file backed by the inode on this system block but reads of the underlying file are allowed (I added local so we don't have to implement for remote filesystems).

How do you define local? Are GFS2 and OCFS2 local? Is XFS with outstanding pNFS layout local? Is NFS with the block or SCSI layout local because it operates on a block device?

The only sane way is to make INA opt-in with a check list of features that need to be supported, and declared to be supported by the fs, similar to how we handle NFS exporting.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Christoph Hellwig

Quoted 4 lines · James Bottomley

Hey, that's not really true: the inode lock (i_rwsem) is used in all sorts of generic places, including generic_file_write_iter().
That's, I think, why ima is using it to try to prevent writes
while it measures the file.

But all these are _below_ file_operations. The only place where take them in the VFS is for namespace locking, e.g. before calling into inode_operations (to generalize a little).

Definitely agree we need an abstraction with defined semantics.

Quoted 9 lines

Quoted 7 lines

Quoted 2 lines

So the answer here is that ima needs to stop playing with i_rwsem.

Isn't there a happy medium? most sensible filesystems will allow shared reading (unless they want to tank performance) so we can rely on the fact that even if a fs does use i_rwsem internally on the read path, it will have to be shared.

At least for direct I/O that doesn't always have to be true.

I'm unsure about the DIO case, so lets try defining the semantics and see if they're implementable for DIO, otherwise simply exclude it.

Quoted 14 lines

Quoted 6 lines

So simply replacing the inode_lock() in ima
with inode_lock_shared() should do what ima wants and not interact badly even if the underlying FS uses i_rwsem. If there's ever a FS that takes it exclusively in the read path, ima can simply blacklist
it.

IFF we actually allow recursive readers for rw_semaphores this would work around the issue (but I'm not sure about that fact, at least in the past we didn't). It won't fix IMA for all the file systems use other synchronization for reads, e.g. the cluster locks in ocfs2 or gfs2. It won't fix NFS which will exhibit exacly the same issue as Mimi reported.

Quoted 6 lines

Last but not least it won't solve the problem that IMA has never been designed and does neither document the requires it has from a file system, nor is there any systematic testing for it. It will keep on breaking because it has all kinds of weird implicit assumptions never written down or verified, and the test coverage for it is basically non-existing.

OK, so how about we define it. I think we need two vfs calls:

inode_block_local_writes(inode)
inode_unblock_local_writes(inode)

With semantics that between these two, all write attempts to the file backed by the inode on this system block but reads of the underlying file are allowed (I added local so we don't have to implement for remote filesystems). inode_block_local_writes() will block until all local writes to the file have finished, so you're guaranteed the file only allows reads when it succeeds.

As for implementation in the vfs, I suspect an outstanding write count in the inode might be the better way?