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

From: Austin S Hemmelgarn <ahferroin7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Andreas Gruenbacher
	<agruenba-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
	Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
Cc: "Andreas Grünbacher"
	<andreas.gruenbacher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	"Alexander Viro"
	<viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>,
	"Theodore Ts'o" <tytso-3s7WtUTddSA@public.gmane.org>,
	"Andreas Dilger"
	<adilger.kernel-m1MBpc4rdrD3fQ9qLvQP4Q@public.gmane.org>,
	"J. Bruce Fields"
	<bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>,
	"Jeff Layton" <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>,
	"Trond Myklebust"
	<trond.myklebust-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>,
	"Anna Schumaker"
	<anna.schumaker-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>,
	linux-ext4 <linux-ext4-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	xfs-VZNHf3L845pBDgjK7y7TUQ@public.gmane.org,
	"Linux Kernel Mailing List"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"Linux FS-devel Mailing List"
	<linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"Linux NFS Mailing List"
	<linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	"Linux API Mailing List"
	<linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH v10 24/46] xfs: Add richacl support
Date: Tue, 13 Oct 2015 15:21:50 -0400	[thread overview]
Message-ID: <561D59CE.9050507@gmail.com> (raw)
In-Reply-To: <CAHc6FU55eOK4gWH1bhKvoujQ1zkT+we0xcfPUOeWrF_X0XHXZg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 3488 bytes --]

On 2015-10-12 01:57, Andreas Gruenbacher wrote:
> On Mon, Oct 12, 2015 at 6:05 AM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> 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-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>:
>>>> 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 #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3019 bytes --]