samba - Feb 2010 - [PATCH 03/23] vfs: rich ACL in-memory representation and manipulation

On Sun, 31 Jan 2010 23:28:52 -0800, Brad Boyer <flar at allandria.com>
wrote:
> 
> I have one suggestion about this part of the code.
> 
> On Mon, Feb 01, 2010 at 11:04:45AM +0530, Aneesh Kumar K.V wrote:
> 
> <snip>
> 
> > +/*
> > + * ACL entries that have ACE4_SPECIAL_WHO set in ace->e_flags use
the
> > + * pointer values of these constants in ace->u.e_who to avoid
massive
> > + * amounts of string comparisons.
> > + */
> > +
> > +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);
> 
> <snip>
> 
> > +/*
> > + * richace_is_same_who  -  do both acl entries refer to the same
identifier?
> > + */
> > +int
> > +richace_is_same_who(const struct richace *a, const struct richace *b)
> > +{
> > +#define WHO_FLAGS (ACE4_SPECIAL_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;
> > +#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
> > + * EVERYONE@ entries into account.
> > + */
> > +static unsigned int
> > +richacl_allowed_to_who(struct richacl *acl, struct richace *who)
> > +{
> > +	struct richace *ace;
> > +	unsigned int allowed = 0;
> > +
> > +	richacl_for_each_entry_reverse(ace, acl) {
> > +		if (richace_is_inherit_only(ace))
> > +			continue;
> > +		if (richace_is_same_who(ace, who) ||
> > +		    richace_is_everyone(ace)) {
> > +			if (richace_is_allow(ace))
> > +				allowed |= ace->e_mask;
> > +			else if (richace_is_deny(ace))
> > +				allowed &= ~ace->e_mask;
> > +		}
> > +	}
> > +	return allowed;
> > +}
> 
> <snip>
> 
> > +struct richace {
> > +	unsigned short	e_type;
> > +	unsigned short	e_flags;
> > +	unsigned int	e_mask;
> > +	union {
> > +		unsigned int	e_id;
> > +		const char	*e_who;
> > +	} u;
> > +};
> 
> <snip>
> 
> > +/* Special e_who identifiers: we use these pointer values in
comparisons
> > +   instead of strcmp for efficiency. */
> > +
> > +extern const char richace_owner_who[];
> > +extern const char richace_group_who[];
> > +extern const char richace_everyone_who[];
> > +
> > +static inline int
> > +richace_is_owner(const struct richace *ace)
> > +{
> > +	return (ace->e_flags & ACE4_SPECIAL_WHO) &&
> > +	       ace->u.e_who == richace_owner_who;
> > +}
> > +
> > +static inline int
> > +richace_is_group(const struct richace *ace)
> > +{
> > +	return (ace->e_flags & ACE4_SPECIAL_WHO) &&
> > +	       ace->u.e_who == richace_group_who;
> > +}
> > +
> > +static inline int
> > +richace_is_everyone(const struct richace *ace)
> > +{
> > +	return (ace->e_flags & ACE4_SPECIAL_WHO) &&
> > +	       ace->u.e_who == richace_everyone_who;
> > +}
> > +
> > +static inline int
> > +richace_is_unix_id(const struct richace *ace)
> > +{
> > +	return !(ace->e_flags & ACE4_SPECIAL_WHO);
> > +}
> 
> Wouldn't it make more sense to just store some small numeric value
> in ace->u.e_who rather than a pointer? It seems to me that the
> savings of storing and comparing an integer type would more than
> make up for the slight overhead of needing to lookup a pointer
> in the places where the code must handle an external representation.
> I know there is really only a difference on 64-bit systems, but my
> impression has been that most people are going that way. In particular,
> the struct richace would go to 12 bytes with 4-byte alignment from
> 16 bytes with 8-byte alignment on an architecture with 8-byte pointers.
> Plus doing an integer instead of a pointer should eliminate the need
> to export the constant pointer values. Even in 32-bit, there are a
> few odd architectures (like m68k) that have separate address and
> data registers, so using an integer may have some benefits there
> due to the fact that the instruction set treats them differently.
> 
> I know you mentioned that you didn't originally write this code, but
> it seems a logical change to me.
I guess id mapping needs more work in the patch. I would really like
to hear from both NFS and Samba people in how they would like the
id details to be stored. If we can't map an incoming user at domain
request on nfs, I guess we definitely don't want to store the acl with
'nobody' id

-aneesh

On Mon, Feb 01, 2010 at 11:32:59PM +0530, Aneesh Kumar K. V
wrote:
> I guess id mapping needs more work in the patch. I would really like
> to hear from both NFS and Samba people in how they would like the
> id details to be stored. If we can't map an incoming user at domain
> request on nfs, I guess we definitely don't want to store the acl with
> 'nobody' id
I don't see the point in allowing the acl's to refer to arbitrary
user at domain strings unless we're also going to allow those strings as
file owners, allow processes to run *as* one of those strings, etc.

If we're really going to try to teach the core kernel to handle foreign
NFS or Samba identities, that's a separate project.

As long as the kernel's working with ordinary uid's and gid's, the
acl's
should do the same, and NFS and Samba can take care of the conversion as
needed.

So I agree that we should be able to use a more compact representation
here.

--b.