On Fri, 21 Oct 2011 01:46:29 +0200, Andreas Gruenbacher <agruen@xxxxxxxxxx> wrote: > On Thu, 2011-10-20 at 06:25 -0400, J. Bruce Fields wrote: > > On Thu, Oct 20, 2011 at 05:19:46AM -0400, Christoph Hellwig wrote: > > > On Thu, Oct 20, 2011 at 05:14:34AM -0400, J. Bruce Fields wrote: > > > > > > Does it really make sense to use a string here just to pick between the > > > > > > three choices OWNER@, GROUP@, and EVERYONE@? Why not just another small > > > > > > integer? Is the goal to expand this somehow eventually? > > > > > > > > > > > > > I guess Andreas wanted the disk layout to be able to store user@domain > > > > > format if needed. > > Yep. On the other hand, none of the code won't actually allow to use > user@domain identifiers, it won't help with other identifier types like > Windows SIDs, and it doesn't make the code any prettier, so this should > probably go away. > > > > > Is that likely? For that to be useful, tasks would need to be able to > > > > run as user@domain strings. And we'd probably want owners and groups to > > > > also be user@domain strings. > > I really don't see this happen anytime soon, and likely not at all. > > > > > The container people seem to eventually want to add some kind of > > > > namespace identifier everywhere: > > > > > > > > http://marc.info/?l=linux-kernel&m=131836778427871&w=2 > > > > > > > > in which case I guess we'd likely end up with (uid, user namespace id) > > > > instead of user@domain? > > The filesystem still wouldn't have namespace ids for the owner and > owning group, which is a much bigger issue. I think we're safe not to > worry about namespace ids at this point; they also might never happen. > > > > Storing strings is an extremly stupid idea. The only thing that would > > > make sense would be storing a windows-style 128-bit GUID. > > > > > > > So if we want to do this without strings: > > > > > > > +struct richace_xattr { > > > > > + __le16 e_type; > > > > > + __le16 e_flags; > > > > > + __le32 e_mask; > > > > > + __le32 e_id; > > > > > + char e_who[0]; > > > > We could drop that last field and use some predefined values for e_id to > > represent owner/group/everyone in the e_type == ACE4_SPECIAL_WHO case. > > That makes sense to me. > > There seems to be a WELL_KNOWN_SID_TYPE enumeration which maps those > kinds of special identifiers to small integers in Windows; maybe it > makes sense to use the same numbers for OWNER@, GROUP@, and EVERYONE@. > > > Then I'm not sure how you'd extend it if you later decided to add > > Windows GUID's or whatever. > > > > But maybe it's not realistic to expect to be able to do that without a > > new interface and on-disk format: how could old software be expected to > > deal with acls that didn't use uid's? > > The acl itself has a version field, so new formats could be introduced > in the future with a new version. How about the below change. This will require richacl tools change also. I made the e_flags 32 bit to make sure we don't take the space needed NFSv4 ACL related flags. diff --git a/fs/richacl_base.c b/fs/richacl_base.c index 9a57039..9179fcd 100644 --- a/fs/richacl_base.c +++ b/fs/richacl_base.c @@ -20,19 +20,6 @@ MODULE_LICENSE("GPL"); -/* - * Special e_who identifiers: ACEs which have ACE4_SPECIAL_WHO set in - * ace->e_flags use these constants in ace->u.e_who. - * - * For efficiency, we compare pointers instead of comparing strings. - */ -const char richace_owner_who[] = "OWNER@"; -EXPORT_SYMBOL_GPL(richace_owner_who); -const char richace_group_who[] = "GROUP@"; -EXPORT_SYMBOL_GPL(richace_group_who); -const char richace_everyone_who[] = "EVERYONE@"; -EXPORT_SYMBOL_GPL(richace_everyone_who); - /** * richacl_alloc - allocate a richacl * @count: number of entries @@ -191,41 +178,14 @@ EXPORT_SYMBOL_GPL(richacl_want_to_mask); int richace_is_same_identifier(const struct richace *a, const struct richace *b) { -#define WHO_FLAGS (ACE4_SPECIAL_WHO | ACE4_IDENTIFIER_GROUP) +#define WHO_FLAGS (ACE4_SPECIAL_WHO | ACE4_UNIXID_WHO | ACE4_IDENTIFIER_GROUP) if ((a->e_flags & WHO_FLAGS) != (b->e_flags & WHO_FLAGS)) return 0; - if (a->e_flags & ACE4_SPECIAL_WHO) - return a->u.e_who == b->u.e_who; - else - return a->u.e_id == b->u.e_id; + return a->e_id == b->e_id; #undef WHO_FLAGS } /** - * richacl_set_who - set a special who value - * @ace: acl entry - * @who: who value to use - */ -int -richace_set_who(struct richace *ace, const char *who) -{ - if (!strcmp(who, richace_owner_who)) - who = richace_owner_who; - else if (!strcmp(who, richace_group_who)) - who = richace_group_who; - else if (!strcmp(who, richace_everyone_who)) - who = richace_everyone_who; - else - return -EINVAL; - - ace->u.e_who = who; - ace->e_flags |= ACE4_SPECIAL_WHO; - ace->e_flags &= ~ACE4_IDENTIFIER_GROUP; - return 0; -} -EXPORT_SYMBOL_GPL(richace_set_who); - -/** * richacl_allowed_to_who - mask flags allowed to a specific who value * * Computes the mask values allowed to a specific who value, taking @@ -446,10 +406,10 @@ richacl_permission(struct inode *inode, const struct richacl *acl, continue; } else if (richace_is_unix_id(ace)) { if (ace->e_flags & ACE4_IDENTIFIER_GROUP) { - if (!in_group_p(ace->u.e_id)) + if (!in_group_p(ace->e_id)) continue; } else { - if (current_fsuid() != ace->u.e_id) + if (current_fsuid() != ace->e_id) continue; } } else diff --git a/fs/richacl_xattr.c b/fs/richacl_xattr.c index 02a7986..3f1f557 100644 --- a/fs/richacl_xattr.c +++ b/fs/richacl_xattr.c @@ -58,19 +58,14 @@ richacl_from_xattr(const void *value, size_t size) goto fail_einval; richacl_for_each_entry(ace, acl) { - const char *who = (void *)(xattr_ace + 1), *end; - ssize_t used = (void *)who - value; - if (used > size) - goto fail_einval; - end = memchr(who, 0, size - used); - if (!end) + if (((void *)xattr_ace + sizeof(*xattr_ace)) > value + size) goto fail_einval; - ace->e_type = le16_to_cpu(xattr_ace->e_type); - ace->e_flags = le16_to_cpu(xattr_ace->e_flags); - ace->e_mask = le32_to_cpu(xattr_ace->e_mask); - ace->u.e_id = le32_to_cpu(xattr_ace->e_id); + ace->e_type = le16_to_cpu(xattr_ace->e_type); + ace->e_flags = le32_to_cpu(xattr_ace->e_flags); + ace->e_mask = le32_to_cpu(xattr_ace->e_mask); + ace->e_id = le32_to_cpu(xattr_ace->e_id); if (ace->e_flags & ~ACE4_VALID_FLAGS) goto fail_einval; @@ -78,13 +73,7 @@ richacl_from_xattr(const void *value, size_t size) (ace->e_mask & ~ACE4_VALID_MASK)) goto fail_einval; - if (who == end) { - if (ace->u.e_id == -1) - goto fail_einval; /* uid/gid needed */ - } else if (richace_set_who(ace, who)) - goto fail_einval; - - xattr_ace = (void *)who + ALIGN(end - who + 1, 4); + xattr_ace = xattr_ace + 1; } return acl; @@ -102,13 +91,8 @@ size_t richacl_xattr_size(const struct richacl *acl) { size_t size = sizeof(struct richacl_xattr); - const struct richace *ace; - richacl_for_each_entry(ace, acl) { - size += sizeof(struct richace_xattr) + - (richace_is_unix_id(ace) ? 4 : - ALIGN(strlen(ace->u.e_who) + 1, 4)); - } + size += sizeof(struct richace_xattr) * acl->a_count; return size; } EXPORT_SYMBOL_GPL(richacl_xattr_size); @@ -136,21 +120,11 @@ richacl_to_xattr(const struct richacl *acl, void *buffer) xattr_ace = (void *)(xattr_acl + 1); richacl_for_each_entry(ace, acl) { xattr_ace->e_type = cpu_to_le16(ace->e_type); - xattr_ace->e_flags = cpu_to_le16(ace->e_flags & + xattr_ace->e_flags = cpu_to_le32(ace->e_flags & ACE4_VALID_FLAGS); xattr_ace->e_mask = cpu_to_le32(ace->e_mask); - if (richace_is_unix_id(ace)) { - xattr_ace->e_id = cpu_to_le32(ace->u.e_id); - memset(xattr_ace->e_who, 0, 4); - xattr_ace = (void *)xattr_ace->e_who + 4; - } else { - int sz = ALIGN(strlen(ace->u.e_who) + 1, 4); - - xattr_ace->e_id = cpu_to_le32(-1); - memset(xattr_ace->e_who + sz - 4, 0, 4); - strcpy(xattr_ace->e_who, ace->u.e_who); - xattr_ace = (void *)xattr_ace->e_who + sz; - } + xattr_ace->e_id = cpu_to_le32(ace->e_id); + xattr_ace = xattr_ace + 1; } } EXPORT_SYMBOL_GPL(richacl_to_xattr); diff --git a/include/linux/richacl.h b/include/linux/richacl.h index 4af6d22..e4c5156 100644 --- a/include/linux/richacl.h +++ b/include/linux/richacl.h @@ -17,14 +17,15 @@ #define __RICHACL_H #include <linux/slab.h> +#define ACE_OWNER_ID 130 +#define ACE_GROUP_ID 131 +#define ACE_EVERYONE_ID 110 + struct richace { unsigned short e_type; - unsigned short e_flags; + unsigned int e_flags; unsigned int e_mask; - union { - unsigned int e_id; - const char *e_who; - } u; + unsigned int e_id; }; struct richacl { @@ -74,8 +75,10 @@ struct richacl { /*#define ACE4_FAILED_ACCESS_ACE_FLAG 0x0020*/ #define ACE4_IDENTIFIER_GROUP 0x0040 #define ACE4_INHERITED_ACE 0x0080 -/* in-memory representation only */ -#define ACE4_SPECIAL_WHO 0x4000 +/* richacl specific flag values */ +#define ACE4_SPECIAL_WHO 0x80000000 +#define ACE4_UNIXID_WHO 0x40000000 + #define ACE4_VALID_FLAGS ( \ ACE4_FILE_INHERIT_ACE | \ @@ -83,7 +86,9 @@ struct richacl { ACE4_NO_PROPAGATE_INHERIT_ACE | \ ACE4_INHERIT_ONLY_ACE | \ ACE4_IDENTIFIER_GROUP | \ - ACE4_INHERITED_ACE) + ACE4_INHERITED_ACE | \ + ACE4_SPECIAL_WHO | \ + ACE4_UNIXID_WHO) /* e_mask bitflags */ #define ACE4_READ_DATA 0x00000001 @@ -254,14 +259,6 @@ richacl_is_protected(const struct richacl *acl) return acl->a_flags & ACL4_PROTECTED; } -/* - * Special e_who identifiers: we use these pointer values in comparisons - * instead of doing a strcmp. - */ -extern const char richace_owner_who[]; -extern const char richace_group_who[]; -extern const char richace_everyone_who[]; - /** * richace_is_owner - check if @ace is an OWNER@ entry */ @@ -269,7 +266,7 @@ static inline int richace_is_owner(const struct richace *ace) { return (ace->e_flags & ACE4_SPECIAL_WHO) && - ace->u.e_who == richace_owner_who; + ace->e_id == ACE_OWNER_ID; } /** @@ -279,7 +276,7 @@ static inline int richace_is_group(const struct richace *ace) { return (ace->e_flags & ACE4_SPECIAL_WHO) && - ace->u.e_who == richace_group_who; + ace->e_id == ACE_GROUP_ID; } /** @@ -289,7 +286,7 @@ static inline int richace_is_everyone(const struct richace *ace) { return (ace->e_flags & ACE4_SPECIAL_WHO) && - ace->u.e_who == richace_everyone_who; + ace->e_id == ACE_EVERYONE_ID; } /** @@ -298,7 +295,7 @@ richace_is_everyone(const struct richace *ace) static inline int richace_is_unix_id(const struct richace *ace) { - return !(ace->e_flags & ACE4_SPECIAL_WHO); + return (ace->e_flags & ACE4_UNIXID_WHO); } /** @@ -357,7 +354,7 @@ richace_is_deny(const struct richace *ace) extern struct richacl *richacl_alloc(int); extern int richace_is_same_identifier(const struct richace *, const struct richace *); -extern int richace_set_who(struct richace *, const char *); +extern int richace_set_who(struct richace *, const u8*, u_int32_t); extern int richacl_masks_to_mode(const struct richacl *); extern unsigned int richacl_mode_to_mask(mode_t); extern unsigned int richacl_want_to_mask(unsigned int); diff --git a/include/linux/richacl_xattr.h b/include/linux/richacl_xattr.h index f79ec12..19cb61e 100644 --- a/include/linux/richacl_xattr.h +++ b/include/linux/richacl_xattr.h @@ -22,10 +22,9 @@ struct richace_xattr { __le16 e_type; - __le16 e_flags; + __le32 e_flags; __le32 e_mask; __le32 e_id; - char e_who[0]; }; struct richacl_xattr { -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
