Re: [PATCH v10 24/46] xfs: Add richacl support

From: Austin S Hemmelgarn
Date: Tue Oct 13 2015 - 15:22:35 EST

On 2015-10-12 01:57, Andreas Gruenbacher wrote:
On Mon, Oct 12, 2015 at 6:05 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
On Mon, Oct 12, 2015 at 03:51:15AM +0200, Andreas GrÃnbacher wrote:
2015-10-12 2:10 GMT+02:00 Dave Chinner <david@xxxxxxxxxxxxx>:
On Mon, Oct 12, 2015 at 12:58:35AM +0200, Andreas Gruenbacher wrote:
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 9590a06..8c6da45 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -461,10 +461,18 @@ xfs_sb_has_ro_compat_feature(
#define XFS_SB_FEAT_INCOMPAT_FTYPE (1 << 0) /* filetype in dirent */
#define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */
#define XFS_SB_FEAT_INCOMPAT_META_UUID (1 << 2) /* metadata UUID */
+
+#ifdef CONFIG_XFS_RICHACL
+#define XFS_SB_FEAT_INCOMPAT_RICHACL (1 << 3) /* richacls */
+#else
+#define XFS_SB_FEAT_INCOMPAT_RICHACL 0
+#endif
+

Regardless of whether we build in richacl support, this is wrong.
Feature bits are on-disk format definitions, so it will always have
this value whether or not the kernel supports the feature.

This is so that xfs_mount_validate_sb() will complain when mounting a
richacl filesystem on a kernel which doesn't have richacl suport
compiled in. The same effect can be had with extra code there of
course.

If the kernel doesn't know about a feature, then it will report
"unknown feature". However, as of this patch set, the kernel will
know about the richacl feature, and so the correct error message
is to say "Rich ACLs not supported by this kernel".

Also, from a disk format perspective, this is a this is a read-only
compat feature, as kernels that don't have richacl support are still
able to mount and walk the filesystem without errors occurring. It's
only when allowing modifications are made that problems will arise,
so why did you make it an incompat feature?

As a read-only compat flag, kernels that cannot enforce richacls would
still be able to mount richacl file systems read-only. That's a
problem when acls are used to forbid read/execute access.
It's also an irrelevant problem, anyone with a minimal knowledge of the filesystem's on-disk layout can unset the feature bit by hand and force it to be mounted anyway, thus bypassing the ACL's (this is the case for any filesystem, not just XFS). If someone has access to the hardware, they have access to the data stored on it, period, irrespective of what the data says about how it should be accessed.

The 3 most common cases for trying to mount a filesystem with this on a kernel that doesn't support it are:
a. Someone is trying to recover data on their own system using something like SystemRescueCD.
b. Someone is trying to recover data from a non-functional system that they own or have been authorized to access for this purpose by connecting the disk to another system.
c. Someone is trying to bisect a kernel bug or track down what config option is causing them issues.
All three of these cases _need_ to keep working properly without needing to manually twiddle with compat bits, otherwise it _will_ cause a lot of people to advocate not using richacls.

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature