diff mbox

[v11,21/48] ext4: Add richacl feature flag

Message ID 1445008706-15115-22-git-send-email-agruenba@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andreas Gruenbacher Oct. 16, 2015, 3:17 p.m. UTC
From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>

This feature flag selects richacl instead of posix acl support on the
file system. In addition, the "acl" mount option is needed for enabling
either of the two kinds of acls.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/ext4/ext4.h  |  6 ++++--
 fs/ext4/super.c | 42 +++++++++++++++++++++++++++++++++---------
 2 files changed, 37 insertions(+), 11 deletions(-)

Comments

Austin S. Hemmelgarn Oct. 16, 2015, 5:31 p.m. UTC | #1
On 2015-10-16 11:17, Andreas Gruenbacher wrote:
> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>
> This feature flag selects richacl instead of posix acl support on the
> file system. In addition, the "acl" mount option is needed for enabling
> either of the two kinds of acls.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>   fs/ext4/ext4.h  |  6 ++++--
>   fs/ext4/super.c | 42 +++++++++++++++++++++++++++++++++---------
>   2 files changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index fd1f28b..b97a3b1 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -991,7 +991,7 @@ struct ext4_inode_info {
>   #define EXT4_MOUNT_UPDATE_JOURNAL	0x01000	/* Update the journal format */
>   #define EXT4_MOUNT_NO_UID32		0x02000  /* Disable 32-bit UIDs */
>   #define EXT4_MOUNT_XATTR_USER		0x04000	/* Extended user attributes */
> -#define EXT4_MOUNT_POSIX_ACL		0x08000	/* POSIX Access Control Lists */
> +#define EXT4_MOUNT_ACL			0x08000	/* Access Control Lists */
>   #define EXT4_MOUNT_NO_AUTO_DA_ALLOC	0x10000	/* No auto delalloc mapping */
>   #define EXT4_MOUNT_BARRIER		0x20000 /* Use block barriers */
>   #define EXT4_MOUNT_QUOTA		0x80000 /* Some quota option set */
> @@ -1582,6 +1582,7 @@ static inline int ext4_encrypted_inode(struct inode *inode)
>   #define EXT4_FEATURE_INCOMPAT_LARGEDIR		0x4000 /* >2GB or 3-lvl htree */
>   #define EXT4_FEATURE_INCOMPAT_INLINE_DATA	0x8000 /* data in inode */
>   #define EXT4_FEATURE_INCOMPAT_ENCRYPT		0x10000
> +#define EXT4_FEATURE_INCOMPAT_RICHACL		0x20000
I would like to re-iterate, on both XFS and ext4, I _really_ think this 
should be a ro_compat flag, and not an incompat one.  If a person has 
the ability to mount the FS (even if it's a read-only mount), then they 
by definition have read access to the file or partition that the 
filesystem is contained in, which means that any ACL's stored on the 
filesystem are functionally irrelevant, and making this an incompat 
feature will just complicate things further for people who have a 
legitimate need to recover data.
Andreas Gruenbacher Oct. 16, 2015, 5:41 p.m. UTC | #2
On Fri, Oct 16, 2015 at 7:31 PM, Austin S Hemmelgarn
<ahferroin7@gmail.com> wrote:
> I would like to re-iterate, on both XFS and ext4, I _really_ think this
> should be a ro_compat flag, and not an incompat one.  If a person has the
> ability to mount the FS (even if it's a read-only mount), then they by
> definition have read access to the file or partition that the filesystem is
> contained in, which means that any ACL's stored on the filesystem are
> functionally irrelevant,

It is unfortunately not safe to make such a file system accessible to
other users, so the feature is not strictly read-only compatible.

Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Austin S. Hemmelgarn Oct. 16, 2015, 6:27 p.m. UTC | #3
On 2015-10-16 13:41, Andreas Gruenbacher wrote:
> On Fri, Oct 16, 2015 at 7:31 PM, Austin S Hemmelgarn
> <ahferroin7@gmail.com> wrote:
>> I would like to re-iterate, on both XFS and ext4, I _really_ think this
>> should be a ro_compat flag, and not an incompat one.  If a person has the
>> ability to mount the FS (even if it's a read-only mount), then they by
>> definition have read access to the file or partition that the filesystem is
>> contained in, which means that any ACL's stored on the filesystem are
>> functionally irrelevant,
>
> It is unfortunately not safe to make such a file system accessible to
> other users, so the feature is not strictly read-only compatible.
If it's not safe WRT data integrity, then the design needs to be 
reworked, as that directly implies that isn't safe for even every day 
usage on a writable filesystem.

If it's not safe WRT the ACL's being honored, then that really isn't 
something we should be worrying about.  POSIX ACL's have this issue, as 
does mounting a filesystem on any system with a different 
/etc/{passwd,shadow,group,gshadow} than the one that wrote the 
permissions to the FS in the first place, and as such this is the type 
of thing any sensible system administrator will already expect to be 
dangerous, which means in turn that they will only do it if there is no 
other choice.

Trying to rely on making this an incompat feature to 'enforce' the ACL's 
is inherently flawed for two very specific reasons:
1. If the person theoretically trying to attack the system has write 
access to the disk, they can flip the feature bit and get access anyway 
(seriously, this takes maybe ten minutes of looking at the source code, 
some simple math and a hex editor).
2. If the disk is read-only (or even if it's writable), they can just 
forgo mounting the filesystem entirely and use any of a number of 
existing tools to pull the data directly off of the disk.

As I said in a previous discussion about this, the three most likely 
reasons for someone mounting a filesystem with this feature on a kernel 
that doesn't support it are:
1. They've booted into a recovery environment (eg SystemRescueCD) to 
attempt to recover data from the system itself (this usage implies 
access to the hardware, and therefore the ACL's are inherently useless 
for protecting the data anyway).
2. They've pulled the disk and hooked it up to a different system to 
recover data from it (again, this implies access to the hardware, and 
ACL's are inherently useless for protecting from this).
3. They're trying to bisect a kernel bug introduced at around the same 
time that richacls went in (which means again they have hardware access 
and ACL's are useless).
All that making this an incompat feature will do is make things harder 
for these legitimate use cases, for any competent attacker it will only 
be a minor inconvenience.
Dave Chinner Oct. 17, 2015, 11:17 p.m. UTC | #4
On Fri, Oct 16, 2015 at 02:27:57PM -0400, Austin S Hemmelgarn wrote:
> On 2015-10-16 13:41, Andreas Gruenbacher wrote:
> >On Fri, Oct 16, 2015 at 7:31 PM, Austin S Hemmelgarn
> ><ahferroin7@gmail.com> wrote:
> >>I would like to re-iterate, on both XFS and ext4, I _really_ think this
> >>should be a ro_compat flag, and not an incompat one.  If a person has the
> >>ability to mount the FS (even if it's a read-only mount), then they by
> >>definition have read access to the file or partition that the filesystem is
> >>contained in, which means that any ACL's stored on the filesystem are
> >>functionally irrelevant,
> >
> >It is unfortunately not safe to make such a file system accessible to
> >other users, so the feature is not strictly read-only compatible.
> If it's not safe WRT data integrity, then the design needs to be
> reworked, as that directly implies that isn't safe for even every
> day usage on a writable filesystem.

This is exactly what we have *incompat feature flags for*: to
protect old code that doesn't know about potentially dangerous new
on-disk formats from trying to parse those formats and causing
unpredictable bad things from happening.

Austin, your arguments hold no weight because they are no different
to the considerations for any new on-disk feature: the user needs to
have both kernel and userspace support to recover filesystems that
go bad. If you are using a brand new fs/kernel feature, then it is
expected that you know that your DR processes take this into
account.

This is also why we XFS devs wait at least a year after new on-disk
features are merged into XFS before we consider turning them on by
default. i.e.  to give distros and recovery utilities time to pick
up kernels and userspace pacakges that support the new feature
before the average user will encounter it....

Cheers,

Dave.
Austin S. Hemmelgarn Oct. 19, 2015, 1:12 p.m. UTC | #5
On 2015-10-17 19:17, Dave Chinner wrote:
> On Fri, Oct 16, 2015 at 02:27:57PM -0400, Austin S Hemmelgarn wrote:
>> On 2015-10-16 13:41, Andreas Gruenbacher wrote:
>>> On Fri, Oct 16, 2015 at 7:31 PM, Austin S Hemmelgarn
>>> <ahferroin7@gmail.com> wrote:
>>>> I would like to re-iterate, on both XFS and ext4, I _really_ think this
>>>> should be a ro_compat flag, and not an incompat one.  If a person has the
>>>> ability to mount the FS (even if it's a read-only mount), then they by
>>>> definition have read access to the file or partition that the filesystem is
>>>> contained in, which means that any ACL's stored on the filesystem are
>>>> functionally irrelevant,
>>>
>>> It is unfortunately not safe to make such a file system accessible to
>>> other users, so the feature is not strictly read-only compatible.
>> If it's not safe WRT data integrity, then the design needs to be
>> reworked, as that directly implies that isn't safe for even every
>> day usage on a writable filesystem.
>
> This is exactly what we have *incompat feature flags for*: to
> protect old code that doesn't know about potentially dangerous new
> on-disk formats from trying to parse those formats and causing
> unpredictable bad things from happening.
However, unless things have changed (I haven't had time to re-read the 
patch-set yet), then the only change will be a new set of xattrs, and 
that type of change _shouldn't_ break existing code that doesn't know 
about them. Andreas really needs to explain _exactly_ why it isn't safe 
to mount this on a kernel that doesn't support it and let other users 
access it, and if the answer is 'because the ACL's won't be honored' 
then that really isn't acceptable reason IMHO, because (as I outlined in 
the previous e-mail) being able to mount the filesystem implies that 
they have at least read access to the underlying storage, which means 
that the ACL's in the filesystem are irrelevant as far as any competent 
individual who is actively trying to illegitimately access the data is 
concerned.
> Austin, your arguments hold no weight because they are no different
> to the considerations for any new on-disk feature: the user needs to
> have both kernel and userspace support to recover filesystems that
> go bad. If you are using a brand new fs/kernel feature, then it is
> expected that you know that your DR processes take this into
> account.
Except that the given argument from Andreas as to why it's an incompat 
feature does not clarify whether it's to prevent breaking the existing 
filesystem code (which I understand and agree is a proper usage for such 
a flag), or to try and provide some thin facade of security when there 
really is none (which is what the bit about 'and expose it to other 
users' really sounds like to me).  Yes the argument that I made which 
you have replied to was admittedly shortsighted and didn't need to be 
made to get the point that I was actually trying to make across, and I 
sincerely apologize for that.
Austin S. Hemmelgarn Oct. 19, 2015, 1:16 p.m. UTC | #6
On 2015-10-16 13:41, Andreas Gruenbacher wrote:
> On Fri, Oct 16, 2015 at 7:31 PM, Austin S Hemmelgarn
> <ahferroin7@gmail.com> wrote:
>> I would like to re-iterate, on both XFS and ext4, I _really_ think this
>> should be a ro_compat flag, and not an incompat one.  If a person has the
>> ability to mount the FS (even if it's a read-only mount), then they by
>> definition have read access to the file or partition that the filesystem is
>> contained in, which means that any ACL's stored on the filesystem are
>> functionally irrelevant,
>
> It is unfortunately not safe to make such a file system accessible to
> other users, so the feature is not strictly read-only compatible.
>
OK, seeing as I wasn't particularly clear as to why I object to this in 
my other e-mail, let's try this again.

Can you please explain exactly why it isn't safe to make such a 
filesystem accessible to other users?  Because that _really_ sounds to 
me like you are trying to rely on this being un-mountable on a kernel 
that doesn't support richacls to try and provide the illusion of better 
security.
Andreas Gruenbacher Oct. 19, 2015, 3:34 p.m. UTC | #7
On Mon, Oct 19, 2015 at 3:16 PM, Austin S Hemmelgarn
<ahferroin7@gmail.com> wrote:
> On 2015-10-16 13:41, Andreas Gruenbacher wrote:
>>
>> On Fri, Oct 16, 2015 at 7:31 PM, Austin S Hemmelgarn
>> <ahferroin7@gmail.com> wrote:
>>>
>>> I would like to re-iterate, on both XFS and ext4, I _really_ think this
>>> should be a ro_compat flag, and not an incompat one.  If a person has the
>>> ability to mount the FS (even if it's a read-only mount), then they by
>>> definition have read access to the file or partition that the filesystem
>>> is contained in, which means that any ACL's stored on the filesystem are
>>> functionally irrelevant,
>>
>> It is unfortunately not safe to make such a file system accessible to
>> other users, so the feature is not strictly read-only compatible.
>>
> OK, seeing as I wasn't particularly clear as to why I object to this in my
> other e-mail, let's try this again.
>
> Can you please explain exactly why it isn't safe to make such a filesystem
> accessible to other users?

See here: http://www.spinics.net/lists/linux-ext4/msg49541.html

Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Austin S. Hemmelgarn Oct. 19, 2015, 4:19 p.m. UTC | #8
On 2015-10-19 11:34, Andreas Gruenbacher wrote:
> On Mon, Oct 19, 2015 at 3:16 PM, Austin S Hemmelgarn
> <ahferroin7@gmail.com> wrote:
>> On 2015-10-16 13:41, Andreas Gruenbacher wrote:
>>>
>>> On Fri, Oct 16, 2015 at 7:31 PM, Austin S Hemmelgarn
>>> <ahferroin7@gmail.com> wrote:
>>>>
>>>> I would like to re-iterate, on both XFS and ext4, I _really_ think this
>>>> should be a ro_compat flag, and not an incompat one.  If a person has the
>>>> ability to mount the FS (even if it's a read-only mount), then they by
>>>> definition have read access to the file or partition that the filesystem
>>>> is contained in, which means that any ACL's stored on the filesystem are
>>>> functionally irrelevant,
>>>
>>> It is unfortunately not safe to make such a file system accessible to
>>> other users, so the feature is not strictly read-only compatible.
>>>
>> OK, seeing as I wasn't particularly clear as to why I object to this in my
>> other e-mail, let's try this again.
>>
>> Can you please explain exactly why it isn't safe to make such a filesystem
>> accessible to other users?
>
> See here: http://www.spinics.net/lists/linux-ext4/msg49541.html
OK, so to clarify, this isn't 'safe' because:
1. The richacls that exist on the filesystem won't be enforced.
2. Newly created files will have no ACL's set.

It is worth noting that these are also issues with any kind of access 
control mechanism.  Using your logic, all LSM's need to set separate 
incompat feature flags in filesystems they are being used on, as should 
POSIX ACLs, and for that matter so should Samba in many circumstances, 
and any NFS system not using idmapping or synchronized/centralized user 
databases.  Now, if the SELinux (or SMACK, or TOMOYO) people had taken 
this approach, then I might be inclined to not complain (at least not to 
you, I'd be complaining to them about this rather poor design choice), 
but that is not the case, because (I assume) they realized that all this 
provides is a false sense of security.

Issue 1, as I have said before, is functionally irrelevant for anyone 
who actually knows what they are doing; all you need for ext* is one of 
the myriad of programs for un-deleting files on such a filesystem (such 
as ext4magic or extundelete, and good luck convincing them to not allow 
being used when this flag is set), for BTRFS you just need the regular 
filesystem administration utilities ('btrfs restore' works wonders, and 
that one will _never_ honor any kind of permissions, because it's for 
disaster recovery), and while I don't know of any way to do this with 
XFS, that is only because I don't use XFS myself and have not had the 
need to provide tech support for anyone who does.  If somebody 
absolutely _needs_ a guarantee that the acls will be enforced, they need 
to be using whole disk encryption, not just acls, and even that can't 
provide such a guarantee.

As for issue 2, that can be solved by making it a read-only compatible 
flag, which is what I was suggesting be done in the first place.  The 
only situation I can think of that this would cause an issue for is if 
the filesystem was not cleanly unmounted, and the log-replay doesn't set 
the ACL's, but mounting an uncleanly unmounted filesystem that has 
richacls on a kernel without support should fall into one of the 
following 2 cases more than 99% of the time:
1. The system crashed hard, and the regular kernel is un-bootable for 
some reason, in this case you're at the point of disaster recovery, 
should not be exposing _anything_ to a multi-user environment, and 
probably care a lot more about being able to get the system running 
again than about not accidentally creating a file with a missing ACL.
2. The filesystem was maliciously stolen in some way (either the 
hardware was acquired, or more likely, someone got an image of a still 
mounted filesystem), in which case all of my statements above regarding 
issue 1 apply.
Andreas Dilger Oct. 19, 2015, 4:39 p.m. UTC | #9
On Oct 19, 2015, at 10:19 AM, Austin S Hemmelgarn <ahferroin7@gmail.com> wrote:
> 
> On 2015-10-19 11:34, Andreas Gruenbacher wrote:
>> On Mon, Oct 19, 2015 at 3:16 PM, Austin S Hemmelgarn
>> <ahferroin7@gmail.com> wrote:
>>> On 2015-10-16 13:41, Andreas Gruenbacher wrote:
>>>> 
>>>> On Fri, Oct 16, 2015 at 7:31 PM, Austin S Hemmelgarn
>>>> <ahferroin7@gmail.com> wrote:
>>>>> 
>>>>> I would like to re-iterate, on both XFS and ext4, I _really_ think this
>>>>> should be a ro_compat flag, and not an incompat one.  If a person has the
>>>>> ability to mount the FS (even if it's a read-only mount), then they by
>>>>> definition have read access to the file or partition that the filesystem
>>>>> is contained in, which means that any ACL's stored on the filesystem are
>>>>> functionally irrelevant,
>>>> 
>>>> It is unfortunately not safe to make such a file system accessible to
>>>> other users, so the feature is not strictly read-only compatible.
>>>> 
>>> OK, seeing as I wasn't particularly clear as to why I object to this in my
>>> other e-mail, let's try this again.
>>> 
>>> Can you please explain exactly why it isn't safe to make such a filesystem
>>> accessible to other users?
>> 
>> See here: http://www.spinics.net/lists/linux-ext4/msg49541.html
> OK, so to clarify, this isn't 'safe' because:
> 1. The richacls that exist on the filesystem won't be enforced.
> 2. Newly created files will have no ACL's set.
> 
> It is worth noting that these are also issues with any kind of access control mechanism.  Using your logic, all LSM's need to set separate incompat feature flags in filesystems they are being used on, as should POSIX ACLs, and for that matter so should Samba in many circumstances, and any NFS system not using idmapping or synchronized/centralized user databases.  Now, if the SELinux (or SMACK, or TOMOYO) people had taken this approach, then I might be inclined to not complain (at least not to you, I'd be complaining to them about this rather poor design choice), but that is not the case, because (I assume) they realized that all this provides is a false sense of security.

I would tend to agree here.  Anyone who can mount the filesystem on a kernel
without RichACL support can do whatever they want, so at most having a
RO_COMPAT flag would serve as a reminder for accidental problems by a
sysadmin not in the know.  Using an INCOMPAT flag is just asking for major
headaches when someone needs to recover their filesystem on an old kernel
but doesn't provide any added safety.

Cheers, Andreas

> Issue 1, as I have said before, is functionally irrelevant for anyone who actually knows what they are doing; all you need for ext* is one of the myriad of programs for un-deleting files on such a filesystem (such as ext4magic or extundelete, and good luck convincing them to not allow being used when this flag is set), for BTRFS you just need the regular filesystem administration utilities ('btrfs restore' works wonders, and that one will _never_ honor any kind of permissions, because it's for disaster recovery), and while I don't know of any way to do this with XFS, that is only because I don't use XFS myself and have not had the need to provide tech support for anyone who does.  If somebody absolutely _needs_ a guarantee that the acls will be enforced, they need to be using whole disk encryption, not just acls, and even that can't provide such a guarantee.
> 
> As for issue 2, that can be solved by making it a read-only compatible flag, which is what I was suggesting be done in the first place.  The only situation I can think of that this would cause an issue for is if the filesystem was not cleanly unmounted, and the log-replay doesn't set the ACL's, but mounting an uncleanly unmounted filesystem that has richacls on a kernel without support should fall into one of the following 2 cases more than 99% of the time:
> 1. The system crashed hard, and the regular kernel is un-bootable for some reason, in this case you're at the point of disaster recovery, should not be exposing _anything_ to a multi-user environment, and probably care a lot more about being able to get the system running again than about not accidentally creating a file with a missing ACL.
> 2. The filesystem was maliciously stolen in some way (either the hardware was acquired, or more likely, someone got an image of a still mounted filesystem), in which case all of my statements above regarding issue 1 apply.
> 


Cheers, Andreas
Andreas Gruenbacher Oct. 19, 2015, 5:33 p.m. UTC | #10
On Mon, Oct 19, 2015 at 6:19 PM, Austin S Hemmelgarn
<ahferroin7@gmail.com> wrote:
> On 2015-10-19 11:34, Andreas Gruenbacher wrote:
>>
>> On Mon, Oct 19, 2015 at 3:16 PM, Austin S Hemmelgarn
>> <ahferroin7@gmail.com> wrote:
>>>
>>> On 2015-10-16 13:41, Andreas Gruenbacher wrote:
>>>>
>>>>
>>>> On Fri, Oct 16, 2015 at 7:31 PM, Austin S Hemmelgarn
>>>> <ahferroin7@gmail.com> wrote:
>>>>>
>>>>>
>>>>> I would like to re-iterate, on both XFS and ext4, I _really_ think this
>>>>> should be a ro_compat flag, and not an incompat one.  If a person has
>>>>> the
>>>>> ability to mount the FS (even if it's a read-only mount), then they by
>>>>> definition have read access to the file or partition that the
>>>>> filesystem
>>>>> is contained in, which means that any ACL's stored on the filesystem
>>>>> are
>>>>> functionally irrelevant,
>>>>
>>>>
>>>> It is unfortunately not safe to make such a file system accessible to
>>>> other users, so the feature is not strictly read-only compatible.
>>>>
>>> OK, seeing as I wasn't particularly clear as to why I object to this in
>>> my
>>> other e-mail, let's try this again.
>>>
>>> Can you please explain exactly why it isn't safe to make such a
>>> filesystem
>>> accessible to other users?
>>
>>
>> See here: http://www.spinics.net/lists/linux-ext4/msg49541.html
>
> OK, so to clarify, this isn't 'safe' because:
> 1. The richacls that exist on the filesystem won't be enforced.
> 2. Newly created files will have no ACL's set.
>
> It is worth noting that these are also issues with any kind of access
> control mechanism.  Using your logic, all LSM's need to set separate
> incompat feature flags in filesystems they are being used on, as should
> POSIX ACLs, and for that matter so should Samba in many circumstances, and
> any NFS system not using idmapping or synchronized/centralized user
> databases.  Now, if the SELinux (or SMACK, or TOMOYO) people had taken this
> approach, then I might be inclined to not complain (at least not to you, I'd
> be complaining to them about this rather poor design choice), but that is
> not the case, because (I assume) they realized that all this provides is a
> false sense of security.

LSMs reside above the filesystem level. Let's take SELinux as an
example. It has its own consistency check mechanism (relabeling). Fsck
could check the syntax of SELinux labels, but it couldn't do anything
sensible about corrupted labels, and syntactically correct labels also
don't mean much. A relabeling run to verify or restory the appropriate
policy would still be necessary to verify that labels are semantically
correct, and for that, the filesystem needs to be mounted in the right
place in the filesystem hierarchy.

TOMOYO and AppArmor are not based on inode labels at all.

LSMs usually also just provide an extra layer of security; when turned
off, the basic security mechanisms still in effect will make sure that
the system works just like before. (There are configurations like MLS
where that is not the case, but those are uncommon.)

ACLs are quite different from that. They can be checked statically by
fsck. They are a basic security concept, and when turned off, there is
no guarantee that the system will still be safe.

> Issue 1, as I have said before, is functionally irrelevant for anyone who
> actually knows what they are doing; all you need for ext* is one of the
> myriad of programs for un-deleting files on such a filesystem (such as
> ext4magic or extundelete, and good luck convincing them to not allow being
> used when this flag is set), for BTRFS you just need the regular filesystem
> administration utilities ('btrfs restore' works wonders, and that one will
> _never_ honor any kind of permissions, because it's for disaster recovery),
> and while I don't know of any way to do this with XFS, that is only because
> I don't use XFS myself and have not had the need to provide tech support for
> anyone who does.  If somebody absolutely _needs_ a guarantee that the acls
> will be enforced, they need to be using whole disk encryption, not just
> acls, and even that can't provide such a guarantee.
>
> As for issue 2, that can be solved by making it a read-only compatible flag,
> which is what I was suggesting be done in the first place.  The only
> situation I can think of that this would cause an issue for is if the
> filesystem was not cleanly unmounted, and the log-replay doesn't set the
> ACL's, but mounting an uncleanly unmounted filesystem that has richacls on a
> kernel without support should fall into one of the following 2 cases more
> than 99% of the time:
> 1. The system crashed hard, and the regular kernel is un-bootable for some
> reason, in this case you're at the point of disaster recovery, should not be
> exposing _anything_ to a multi-user environment, and probably care a lot
> more about being able to get the system running again than about not
> accidentally creating a file with a missing ACL.
> 2. The filesystem was maliciously stolen in some way (either the hardware
> was acquired, or more likely, someone got an image of a still mounted
> filesystem), in which case all of my statements above regarding issue 1
> apply.

Please spare me with all that nonsense. Compared to mount options,
filesystem feature flags in this case simplify things (you don't have
to specify whether a filesystem contains POSIX ACLs or richacls), and
they prevent administrator errors: when a filesystem mounts, it is
safe to use; when it doesn't, it is not. That's all there is to it.

Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Austin S. Hemmelgarn Oct. 19, 2015, 6:45 p.m. UTC | #11
On 2015-10-19 13:33, Andreas Gruenbacher wrote:
> On Mon, Oct 19, 2015 at 6:19 PM, Austin S Hemmelgarn
> <ahferroin7@gmail.com> wrote:
>> On 2015-10-19 11:34, Andreas Gruenbacher wrote:
>>> On Mon, Oct 19, 2015 at 3:16 PM, Austin S Hemmelgarn
>>> <ahferroin7@gmail.com> wrote:
>>>> On 2015-10-16 13:41, Andreas Gruenbacher wrote:
>>>>> On Fri, Oct 16, 2015 at 7:31 PM, Austin S Hemmelgarn
>>>>> <ahferroin7@gmail.com> wrote:
>>>>>> I would like to re-iterate, on both XFS and ext4, I _really_ think this
>>>>>> should be a ro_compat flag, and not an incompat one.  If a person has
>>>>>> the
>>>>>> ability to mount the FS (even if it's a read-only mount), then they by
>>>>>> definition have read access to the file or partition that the
>>>>>> filesystem
>>>>>> is contained in, which means that any ACL's stored on the filesystem
>>>>>> are
>>>>>> functionally irrelevant,
>>>>> It is unfortunately not safe to make such a file system accessible to
>>>>> other users, so the feature is not strictly read-only compatible.
>>>> OK, seeing as I wasn't particularly clear as to why I object to this in
>>>> my
>>>> other e-mail, let's try this again.
>>>> Can you please explain exactly why it isn't safe to make such a
>>>> filesystem
>>>> accessible to other users?
>>> See here: http://www.spinics.net/lists/linux-ext4/msg49541.html
>> OK, so to clarify, this isn't 'safe' because:
>> 1. The richacls that exist on the filesystem won't be enforced.
>> 2. Newly created files will have no ACL's set.
>>
>> It is worth noting that these are also issues with any kind of access
>> control mechanism.  Using your logic, all LSM's need to set separate
>> incompat feature flags in filesystems they are being used on, as should
>> POSIX ACLs, and for that matter so should Samba in many circumstances, and
>> any NFS system not using idmapping or synchronized/centralized user
>> databases.  Now, if the SELinux (or SMACK, or TOMOYO) people had taken this
>> approach, then I might be inclined to not complain (at least not to you, I'd
>> be complaining to them about this rather poor design choice), but that is
>> not the case, because (I assume) they realized that all this provides is a
>> false sense of security.
>
> LSMs reside above the filesystem level. Let's take SELinux as an
> example. It has its own consistency check mechanism (relabeling). Fsck
> could check the syntax of SELinux labels, but it couldn't do anything
> sensible about corrupted labels, and syntactically correct labels also
> don't mean much. A relabeling run to verify or restory the appropriate
> policy would still be necessary to verify that labels are semantically
> correct, and for that, the filesystem needs to be mounted in the right
> place in the filesystem hierarchy.
>
> TOMOYO and AppArmor are not based on inode labels at all.
Apologies for being unintentionally over-inclusive WRT LSM's, and also 
for forgetting that TOMOYO doesn't use inode labels.
> LSMs usually also just provide an extra layer of security; when turned
> off, the basic security mechanisms still in effect will make sure that
> the system works just like before. (There are configurations like MLS
> where that is not the case, but those are uncommon.)
Um, actually no, even without MLS (or MCS), there is no guarantee 
whatsoever that the system will work 'just' like before (I've actually 
seen systems break on any (or even in one case all) of the transitions 
between enforcing/permissive/off for SELinux, even assuming that 
relabeling is done properly).
> ACLs are quite different from that. They can be checked statically by
> fsck. They are a basic security concept, and when turned off, there is
> no guarantee that the system will still be safe.
LSM's hook into the VFS code right alongside the regular permissions 
checks. They are intended to supplement the regular UNIX DAC based 
permissions. Richacls (and POSIX ACLs) also hook into the regular 
permissions checks, and also are intended to supplement the regular UNIX 
DAC based permissions. Fsck only checks the syntax of regular UNIX DAC 
based permissions (it doesn't verify that the listed UID/GID are 
actually valid in userspace, nor does it check for semantically 
nonsensical permissions modes like 007 or 000), and it really can't 
properly check anything more than that on ACL's either.  Based on all of 
this, richacls and LSM's which mediate filesystem accesses are on 
exactly the same level with the exception that LSM's usually have way 
more functionality beyond just using ACL's to control file access, LSM's 
are usually MAC based while ACL's are DAC based, and the fact that ACL's 
are (usually) not dependent on where in the filesystem hierarchy they 
are found.

On top of that, there is no guarantee that a system will still be safe 
when you turn SELinux (or other LSM's) off either (in fact, for some 
configurations, it can be mathematically proven that the system will not 
be safe if you turn off SELinux).
>> Issue 1, as I have said before, is functionally irrelevant for anyone who
>> actually knows what they are doing; all you need for ext* is one of the
>> myriad of programs for un-deleting files on such a filesystem (such as
>> ext4magic or extundelete, and good luck convincing them to not allow being
>> used when this flag is set), for BTRFS you just need the regular filesystem
>> administration utilities ('btrfs restore' works wonders, and that one will
>> _never_ honor any kind of permissions, because it's for disaster recovery),
>> and while I don't know of any way to do this with XFS, that is only because
>> I don't use XFS myself and have not had the need to provide tech support for
>> anyone who does.  If somebody absolutely _needs_ a guarantee that the acls
>> will be enforced, they need to be using whole disk encryption, not just
>> acls, and even that can't provide such a guarantee.
>>
>> As for issue 2, that can be solved by making it a read-only compatible flag,
>> which is what I was suggesting be done in the first place.  The only
>> situation I can think of that this would cause an issue for is if the
>> filesystem was not cleanly unmounted, and the log-replay doesn't set the
>> ACL's, but mounting an uncleanly unmounted filesystem that has richacls on a
>> kernel without support should fall into one of the following 2 cases more
>> than 99% of the time:
>> 1. The system crashed hard, and the regular kernel is un-bootable for some
>> reason, in this case you're at the point of disaster recovery, should not be
>> exposing _anything_ to a multi-user environment, and probably care a lot
>> more about being able to get the system running again than about not
>> accidentally creating a file with a missing ACL.
>> 2. The filesystem was maliciously stolen in some way (either the hardware
>> was acquired, or more likely, someone got an image of a still mounted
>> filesystem), in which case all of my statements above regarding issue 1
>> apply.
> Please spare me with all that nonsense. Compared to mount options,
> filesystem feature flags in this case simplify things (you don't have
> to specify whether a filesystem contains POSIX ACLs or richacls), and
> they prevent administrator errors: when a filesystem mounts, it is
> safe to use; when it doesn't, it is not. That's all there is to it.
You're ignoring what I'm actually saying.  I've said absolutely nothing 
about needing to use mount options at all, and I'm not arguing against 
using filesystem feature flags, I'm arguing for using them sensibly in a 
way that does not present a false sense of security.

Making the comparability flag read-only will exactly solve the second 
issue that you outlined for more than 99% of all use cases (and the 
remaining cases are provably irrelevant), still indicate clearly to the 
FS code which type of ACL is in use on the FS, and remove the entirely 
false sense of security WRT the first issue you outlined.

Making it an incompatible flag will likely cause headaches for some 
legitimate users, and at most delay competent hackers by a few seconds 
to a few minutes, and script kiddies by a few hours, and is really no 
better than security by obscurity (and from a purely logistical 
standpoint, that's _all_ it is) in that it actively tries to hide the 
fact that someone having read access to the storage the filesystem is on 
can bypass the ACL's.

To reiterate, if someone can call mount() on a filesystem, and mount() 
does not return -EPERM, then even if mount() returns a different error, 
they still have the ability to completely bypass all permissions and 
ACL's in that filesystem, because they have the ability to read the 
entire filesystem directly.

The _only_ way to properly protect against people bypassing the ACL's is 
to use full disk encryption and lock down root access on the system, and 
even that can't completely prevent it from happening.
Andreas Gruenbacher Oct. 19, 2015, 8:20 p.m. UTC | #12
On Mon, Oct 19, 2015 at 8:45 PM, Austin S Hemmelgarn
<ahferroin7@gmail.com> wrote:
> On 2015-10-19 13:33, Andreas Gruenbacher wrote:
>> Please spare me with all that nonsense. Compared to mount options,
>> filesystem feature flags in this case simplify things (you don't have
>> to specify whether a filesystem contains POSIX ACLs or richacls), and
>> they prevent administrator errors: when a filesystem mounts, it is
>> safe to use; when it doesn't, it is not. That's all there is to it.
>
> You're ignoring what I'm actually saying. I've said absolutely nothing
> about needing to use mount options at all, and I'm not arguing against using
> filesystem feature flags, I'm arguing for using them sensibly in a way that
> does not present a false sense of security.

We could be on a multi-user system, and the user mounting the
filesystem may not be the only user on the system. When a filesystem
can be mounted read-only, it should be safe to use read-only. It is
not safe in general to use such a filesystem read-only, so an
incompatible feature flag which prevents such unsafe mounting is more
approporiate than a read-only incompatible feature flag.

Mounting a filesystem read-only doesn't mean that the filesystem is
being recovered, it is perfectly legal to mount a filesystem read-only
for other reasons. I don't want to give people using read-only
filesystems the false sense that everything is okay.

> Making it an incompatible flag will likely cause headaches for some
> legitimate users,

Indeed. It will also make it less likely for users to accidentally
shoot themselves in the foot. If someone knows better, they can clear
the feature flag.

When recovering a broken system that contains richacl filesystems, you
really want to have richacl support in the rescue system as well.
Otherwise, you won't be able to fsck those filesystems.

> and at most delay competent hackers by a few seconds to a
> few minutes, and script kiddies by a few hours, and is really no better than
> security by obscurity (and from a purely logistical standpoint, that's _all_
> it is) in that it actively tries to hide the fact that someone having read
> access to the storage the filesystem is on can bypass the ACL's.
>
> To reiterate, if someone can call mount() on a filesystem, and mount() does
> not return -EPERM, then even if mount() returns a different error, they
> still have the ability to completely bypass all permissions and ACL's in
> that filesystem, because they have the ability to read the entire filesystem
> directly.
>
> The _only_ way to properly protect against people bypassing the ACL's is to
> use full disk encryption and lock down root access on the system, and even
> that can't completely prevent it from happening.

That's all completely beside the point. I'm not talking about
preventing attacks at all, just basic administrative workflows.

Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Austin S. Hemmelgarn Oct. 20, 2015, 12:33 p.m. UTC | #13
On 2015-10-19 16:20, Andreas Gruenbacher wrote:
> On Mon, Oct 19, 2015 at 8:45 PM, Austin S Hemmelgarn
> <ahferroin7@gmail.com> wrote:
>> On 2015-10-19 13:33, Andreas Gruenbacher wrote:
>>> Please spare me with all that nonsense. Compared to mount options,
>>> filesystem feature flags in this case simplify things (you don't have
>>> to specify whether a filesystem contains POSIX ACLs or richacls), and
>>> they prevent administrator errors: when a filesystem mounts, it is
>>> safe to use; when it doesn't, it is not. That's all there is to it.
>>
>> You're ignoring what I'm actually saying. I've said absolutely nothing
>> about needing to use mount options at all, and I'm not arguing against using
>> filesystem feature flags, I'm arguing for using them sensibly in a way that
>> does not present a false sense of security.
>
> We could be on a multi-user system, and the user mounting the
> filesystem may not be the only user on the system. When a filesystem
> can be mounted read-only, it should be safe to use read-only. It is
> not safe in general to use such a filesystem read-only, so an
> incompatible feature flag which prevents such unsafe mounting is more
> approporiate than a read-only incompatible feature flag.
Except that mounting a filesystem that wasn't created on the system 
mounting it always has that exact same risk, because (AFAIK) all 
Linux/BSD/Other UNIX filesystems store GID's and UID's as numbers, not 
names.  Perfect example of this, take a minimal install of some Linux 
distribution, install a couple of packages that add new UID's, and mount 
the root filesystem on a completely different distribution (say mount a 
Debian root filesystem on a Gentoo box), any of the new UID's from the 
first system will almost certainly not map to the same user on the 
second system (in fact, you can also do this with two Gentoo systems 
where the packages were installed in different orders).  This is a 
really basic security risk that any seasoned sysadmin should know, and 
most new ones find out about rather quickly.

Also, based on established context of _every_ other feature with an 
incompat feature flag in both ext4 and XFS, 'safe' means that you can 
get the correct file contents off of the filesystem,and don't run the 
risk of crashing the system, not that you have no risk of compromising 
security.
> Mounting a filesystem read-only doesn't mean that the filesystem is
> being recovered, it is perfectly legal to mount a filesystem read-only
> for other reasons. I don't want to give people using read-only
> filesystems the false sense that everything is okay.
Yes, and this is why any sane filesystem will spit a warning out through 
dmesg when a mount is forced read-only because of incompatible features. 
  If someone is actively mounting such a filesystem read-only (that is, 
they've specified 'ro' in the mount options), then it's relatively safe 
for the kernel to assume they are doing so for some very specific reason 
and/or already know about the data safety implications.
>
>> Making it an incompatible flag will likely cause headaches for some
>> legitimate users,
>
> Indeed. It will also make it less likely for users to accidentally
> shoot themselves in the foot. If someone knows better, they can clear
> the feature flag.
Except there will be a lot of people who think they know better but 
really don't.  Such people will do this anyway, and by modifying the 
filesystem run a bigger risk of shooting themselves in the foot (because 
they'll almost certainly mount the filesystem read-write, and then end 
up turning on richacls again).  You're not removing the gun, you're just 
hiding a potentially bigger one somewhere else.

It's a separate issue entirely however whether or not you absolutely 
need to know that the richacls have the correct syntax in a recovery 
situation.  Fsck doesn't parse SELinux labels, (AFAIK) doesn't parse 
filecaps attributes (bad filecaps syntax will only make things have 
fewer privileges, but it will cause user visible brokenness), (again, 
AFAIK) doesn't validate POSIX ACL's, and absolutely doesn't check IMA or 
EVM hashes.  IMA and EVM hashes being wrong _will_ cause almost any 
system actually using them they way they are intended to fail to boot, 
and incorrect SELinux labeling will make many systems not boot 
correctly, and both situations are much worse for a significant majority 
of users than a (security leak due to a bad ACL.
>
> When recovering a broken system that contains richacl filesystems, you
> really want to have richacl support in the rescue system as well.
> Otherwise, you won't be able to fsck those filesystems.
While it's something that 'should' be the case, there are probably quite 
a few people who will not realize this until they're already in a 
situation that they need to recover data.  On top of that some people 
will likely assume that they just need richacl support in userspace for 
their recovery environment.
>
>> and at most delay competent hackers by a few seconds to a
>> few minutes, and script kiddies by a few hours, and is really no better than
>> security by obscurity (and from a purely logistical standpoint, that's _all_
>> it is) in that it actively tries to hide the fact that someone having read
>> access to the storage the filesystem is on can bypass the ACL's.
>>
>> To reiterate, if someone can call mount() on a filesystem, and mount() does
>> not return -EPERM, then even if mount() returns a different error, they
>> still have the ability to completely bypass all permissions and ACL's in
>> that filesystem, because they have the ability to read the entire filesystem
>> directly.
>>
>> The _only_ way to properly protect against people bypassing the ACL's is to
>> use full disk encryption and lock down root access on the system, and even
>> that can't completely prevent it from happening.
>
> That's all completely beside the point. I'm not talking about
> preventing attacks at all, just basic administrative workflows.
While that may be the case, there will be people who assume that because 
it's an incompat feature, their ACL's will _always_ be enforced.  Such 
people should admittedly not be allowed to run systems with any real 
world security requirements, but that mentality is something that still 
needs to be considered, and this is arguably making it easier for them 
to shoot themselves in the foot.
diff mbox

Patch

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index fd1f28b..b97a3b1 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -991,7 +991,7 @@  struct ext4_inode_info {
 #define EXT4_MOUNT_UPDATE_JOURNAL	0x01000	/* Update the journal format */
 #define EXT4_MOUNT_NO_UID32		0x02000  /* Disable 32-bit UIDs */
 #define EXT4_MOUNT_XATTR_USER		0x04000	/* Extended user attributes */
-#define EXT4_MOUNT_POSIX_ACL		0x08000	/* POSIX Access Control Lists */
+#define EXT4_MOUNT_ACL			0x08000	/* Access Control Lists */
 #define EXT4_MOUNT_NO_AUTO_DA_ALLOC	0x10000	/* No auto delalloc mapping */
 #define EXT4_MOUNT_BARRIER		0x20000 /* Use block barriers */
 #define EXT4_MOUNT_QUOTA		0x80000 /* Some quota option set */
@@ -1582,6 +1582,7 @@  static inline int ext4_encrypted_inode(struct inode *inode)
 #define EXT4_FEATURE_INCOMPAT_LARGEDIR		0x4000 /* >2GB or 3-lvl htree */
 #define EXT4_FEATURE_INCOMPAT_INLINE_DATA	0x8000 /* data in inode */
 #define EXT4_FEATURE_INCOMPAT_ENCRYPT		0x10000
+#define EXT4_FEATURE_INCOMPAT_RICHACL		0x20000
 
 #define EXT2_FEATURE_COMPAT_SUPP	EXT4_FEATURE_COMPAT_EXT_ATTR
 #define EXT2_FEATURE_INCOMPAT_SUPP	(EXT4_FEATURE_INCOMPAT_FILETYPE| \
@@ -1607,7 +1608,8 @@  static inline int ext4_encrypted_inode(struct inode *inode)
 					 EXT4_FEATURE_INCOMPAT_FLEX_BG| \
 					 EXT4_FEATURE_INCOMPAT_MMP | \
 					 EXT4_FEATURE_INCOMPAT_INLINE_DATA | \
-					 EXT4_FEATURE_INCOMPAT_ENCRYPT)
+					 EXT4_FEATURE_INCOMPAT_ENCRYPT | \
+					 EXT4_FEATURE_INCOMPAT_RICHACL)
 #define EXT4_FEATURE_RO_COMPAT_SUPP	(EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER| \
 					 EXT4_FEATURE_RO_COMPAT_LARGE_FILE| \
 					 EXT4_FEATURE_RO_COMPAT_GDT_CSUM| \
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index a63c7b0..84d1a5d 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1270,6 +1270,28 @@  static ext4_fsblk_t get_sb_block(void **data)
 	return sb_block;
 }
 
+static int enable_acl(struct super_block *sb)
+{
+	sb->s_flags &= ~(MS_POSIXACL | MS_RICHACL);
+	if (test_opt(sb, ACL)) {
+		if (EXT4_HAS_INCOMPAT_FEATURE(sb,
+					      EXT4_FEATURE_INCOMPAT_RICHACL)) {
+#ifdef CONFIG_EXT4_FS_RICHACL
+			sb->s_flags |= MS_RICHACL;
+#else
+			return -EOPNOTSUPP;
+#endif
+		} else {
+#ifdef CONFIG_EXT4_FS_POSIX_ACL
+			sb->s_flags |= MS_POSIXACL;
+#else
+			return -EOPNOTSUPP;
+#endif
+		}
+	}
+	return 0;
+}
+
 #define DEFAULT_JOURNAL_IOPRIO (IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 3))
 static char deprecated_msg[] = "Mount option \"%s\" will be removed by %s\n"
 	"Contact linux-ext4@vger.kernel.org if you think we should keep it.\n";
@@ -1416,9 +1438,9 @@  static const struct mount_opts {
 	 MOPT_NO_EXT2 | MOPT_DATAJ},
 	{Opt_user_xattr, EXT4_MOUNT_XATTR_USER, MOPT_SET},
 	{Opt_nouser_xattr, EXT4_MOUNT_XATTR_USER, MOPT_CLEAR},
-#ifdef CONFIG_EXT4_FS_POSIX_ACL
-	{Opt_acl, EXT4_MOUNT_POSIX_ACL, MOPT_SET},
-	{Opt_noacl, EXT4_MOUNT_POSIX_ACL, MOPT_CLEAR},
+#if defined(CONFIG_EXT4_FS_POSIX_ACL) || defined(CONFIG_EXT4_FS_RICHACL)
+	{Opt_acl, EXT4_MOUNT_ACL, MOPT_SET},
+	{Opt_noacl, EXT4_MOUNT_ACL, MOPT_CLEAR},
 #else
 	{Opt_acl, 0, MOPT_NOSUPPORT},
 	{Opt_noacl, 0, MOPT_NOSUPPORT},
@@ -3576,8 +3598,8 @@  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		set_opt(sb, NO_UID32);
 	/* xattr user namespace & acls are now defaulted on */
 	set_opt(sb, XATTR_USER);
-#ifdef CONFIG_EXT4_FS_POSIX_ACL
-	set_opt(sb, POSIX_ACL);
+#if defined(CONFIG_EXT4_FS_POSIX_ACL) || defined(CONFIG_EXT4_FS_RICHACL)
+	set_opt(sb, ACL);
 #endif
 	/* don't forget to enable journal_csum when metadata_csum is enabled. */
 	if (ext4_has_metadata_csum(sb))
@@ -3660,8 +3682,9 @@  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		sb->s_iflags |= SB_I_CGROUPWB;
 	}
 
-	sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
-		(test_opt(sb, POSIX_ACL) ? MS_POSIXACL : 0);
+	err = enable_acl(sb);
+	if (err)
+		goto failed_mount;
 
 	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
 	    (EXT4_HAS_COMPAT_FEATURE(sb, ~0U) ||
@@ -4981,8 +5004,9 @@  static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	if (sbi->s_mount_flags & EXT4_MF_FS_ABORTED)
 		ext4_abort(sb, "Abort forced by user");
 
-	sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
-		(test_opt(sb, POSIX_ACL) ? MS_POSIXACL : 0);
+	err = enable_acl(sb);
+	if (err)
+		goto restore_opts;
 
 	es = sbi->s_es;