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

[Andreas Gruenbacher]

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/Kconfig b/fs/xfs/Kconfig
>> >> index 5d47b4d..05dd312 100644
>> >> --- a/fs/xfs/Kconfig
>> >> +++ b/fs/xfs/Kconfig
>> >> @@ -52,6 +52,17 @@ config XFS_POSIX_ACL
>> >>
>> >>         If you don't know what Access Control Lists are, say N.
>> >>
>> >> +config XFS_RICHACL
>> >> +        bool "XFS Rich Access Control Lists (EXPERIMENTAL)"
>> >> +        depends on XFS_FS
>> >> +        select FS_RICHACL
>> >> +        help
>> >> +          Richacls are an implementation of NFSv4 ACLs, extended by file masks
>> >> +          to cleanly integrate into the POSIX file permission model.  To learn
>> >> +       more about them, see http://www.bestbits.at/richacl/.
>> >> +
>> >> +          If you don't know what Richacls are, say N.
>> >> +
>> >
>> > So now we have multiple compile time ACL configs that we have to
>> > test. Personally, I see no reason for making either posix or rich
>> > acls compile time dependent. How about we just change CONFIG_FS_XFS
>> > to have "select FS_POSIX_ACL" and "select FS_RICHACL"
>> > unconditionally, and then we can get rid of all the conditional
>> > code?
>>
>> I'm sure neither kind of ACLs makes sense on many tiny systems, it
>> should be possible to disable them. XFS may not make sense on those
>> kinds of systems either, that is not my call to make though.
>
> Well, the smallest systems that use XFS are typically 1-2 disk NAS
> boxes, and I can see them wanting richacls. As it is, the xfs kernel
> module is almost 1MB of object code, so it you have a small embedded
> box you don't want XFS. Almost all distros turn on ACL support I'm
> not sure that it makes sense to have these config options in XFS
> anymore....

You have me convinced about removing CONFIG_XFS_RICHACL.

>> >> 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.

>> >> +int
>> >> +xfs_set_richacl(struct inode *inode, struct richacl *acl)
>> >> +{
>> >> +     struct xfs_inode *ip = XFS_I(inode);
>> >> +     umode_t mode = inode->i_mode;
>> >> +     int error;
>> >> +
>> >> +     if (acl) {
>> >> +             /* Don't allow acls with unmapped identifiers. */
>> >> +             if (richacl_has_unmapped_identifiers(acl))
>> >> +                     return -EINVAL;
>> >> +
>> >> +             if (richacl_equiv_mode(acl, &mode) == 0) {
>> >> +                     xfs_set_mode(inode, mode);
>> >> +                     acl = NULL;
>> >> +             }
>> >
>> > Comment needed here - why does this now seem to accidentally fall
>> > through to removing the ACL?
>>
>> Setting an ACL that is equivalent to a file mode is the same as
>> removing any existing ACLs and doing a chmod to that equivalent file
>> mode. It's the sams as with POSIX ACLs, and the code is the same as
>> well. I'll add a comment though.
>>
>> > Also, why is this in the XFS specific
>> > code when it's generic richacl functionality that is being checked?
>>
>> This turns into two non-atomic operations if we move it into generic code.
>
> I don't follow you - this isn't an atomic operation to begin with...

Right, I shouldn't have used the term atomic. It's one inode operation
under i_mutex, and the filesystem can pack it into one transaction.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/