Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-22 Thread J. Bruce Fields
On Wed, Sep 23, 2015 at 03:39:44AM +0200, Andreas Gruenbacher wrote:
> Here are my improvements; hope that helps ...

Yes, looks good, thanks!--b.

> 
> Thanks,
> Andreas
> 
> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> index 9b76fc0..21af9a0 100644
> --- a/fs/richacl_compat.c
> +++ b/fs/richacl_compat.c
> @@ -351,26 +351,26 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
>   struct richace *ace;
>   unsigned int owner_allow, group_allow;
>  
> - /*
> -  * If the owner mask contains permissions which are not in the group
> -  * mask, the group mask contains permissions which are not in the other
> -  * mask, or the owner class contains permissions which are not in the
> -  * other mask, we may need to propagate permissions up from the
> -  * everyone@ allow ace.  The third condition is implied by the first
> -  * two.
> -  */
> - if (!((acl->a_owner_mask & ~acl->a_group_mask) ||
> -   (acl->a_group_mask & ~acl->a_other_mask)))
> - return 0;
>   if (!acl->a_count)
>   return 0;
>   ace = acl->a_entries + acl->a_count - 1;
>   if (richace_is_inherit_only(ace) || !richace_is_everyone(ace))
>   return 0;
>  
> + /*
> +  * Permissions the owner and group class are granted through the
> +  * trailing everyone@ allow ace.
> +  */
>   owner_allow = ace->e_mask & acl->a_owner_mask;
>   group_allow = ace->e_mask & acl->a_group_mask;
>  
> + /*
> +  * If the group or other masks hide permissions which the owner should
> +  * be allowed, we need to propagate those permissions up.  Otherwise,
> +  * those permissions may be lost when applying the other mask to the
> +  * trailing everyone@ allow ace, or when isolating the group class from
> +  * the other class through additional deny aces.
> +  */
>   if (owner_allow & ~(acl->a_group_mask & acl->a_other_mask)) {
>   /* Propagate everyone@ permissions through to owner@. */
>   who.e_id.special = RICHACE_OWNER_SPECIAL_ID;
> @@ -379,6 +379,11 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
>   acl = alloc->acl;
>   }
>  
> + /*
> +  * If the other mask hides permissions which the group class should be
> +  * allowed, we need to propagate those permissions up to the owning
> +  * group and to all other members in the group class.
> +  */
>   if (group_allow & ~acl->a_other_mask) {
>   int n;
>  
> @@ -399,16 +404,15 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
>   richace_is_owner(ace) ||
>   richace_is_group(ace))
>   continue;
> - if (richace_is_allow(ace) || richace_is_deny(ace)) {
> - /*
> -  * Any inserted entry will end up below the
> -  * current entry
> -  */
> - if (__richacl_propagate_everyone(alloc, ace,
> -  group_allow))
> - return -1;
> - acl = alloc->acl;
> - }
> +
> + /*
> +  * Any inserted entry will end up below the current
> +  * entry.
> +  */
> + if (__richacl_propagate_everyone(alloc, ace,
> +  group_allow))
> + return -1;
> + acl = alloc->acl;
>   }
>   }
>   return 0;
> -- 
> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-22 Thread Andreas Gruenbacher
Here are my improvements; hope that helps ...

Thanks,
Andreas

diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
index 9b76fc0..21af9a0 100644
--- a/fs/richacl_compat.c
+++ b/fs/richacl_compat.c
@@ -351,26 +351,26 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
struct richace *ace;
unsigned int owner_allow, group_allow;
 
-   /*
-* If the owner mask contains permissions which are not in the group
-* mask, the group mask contains permissions which are not in the other
-* mask, or the owner class contains permissions which are not in the
-* other mask, we may need to propagate permissions up from the
-* everyone@ allow ace.  The third condition is implied by the first
-* two.
-*/
-   if (!((acl->a_owner_mask & ~acl->a_group_mask) ||
- (acl->a_group_mask & ~acl->a_other_mask)))
-   return 0;
if (!acl->a_count)
return 0;
ace = acl->a_entries + acl->a_count - 1;
if (richace_is_inherit_only(ace) || !richace_is_everyone(ace))
return 0;
 
+   /*
+* Permissions the owner and group class are granted through the
+* trailing everyone@ allow ace.
+*/
owner_allow = ace->e_mask & acl->a_owner_mask;
group_allow = ace->e_mask & acl->a_group_mask;
 
+   /*
+* If the group or other masks hide permissions which the owner should
+* be allowed, we need to propagate those permissions up.  Otherwise,
+* those permissions may be lost when applying the other mask to the
+* trailing everyone@ allow ace, or when isolating the group class from
+* the other class through additional deny aces.
+*/
if (owner_allow & ~(acl->a_group_mask & acl->a_other_mask)) {
/* Propagate everyone@ permissions through to owner@. */
who.e_id.special = RICHACE_OWNER_SPECIAL_ID;
@@ -379,6 +379,11 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
acl = alloc->acl;
}
 
+   /*
+* If the other mask hides permissions which the group class should be
+* allowed, we need to propagate those permissions up to the owning
+* group and to all other members in the group class.
+*/
if (group_allow & ~acl->a_other_mask) {
int n;
 
@@ -399,16 +404,15 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
richace_is_owner(ace) ||
richace_is_group(ace))
continue;
-   if (richace_is_allow(ace) || richace_is_deny(ace)) {
-   /*
-* Any inserted entry will end up below the
-* current entry
-*/
-   if (__richacl_propagate_everyone(alloc, ace,
-group_allow))
-   return -1;
-   acl = alloc->acl;
-   }
+
+   /*
+* Any inserted entry will end up below the current
+* entry.
+*/
+   if (__richacl_propagate_everyone(alloc, ace,
+group_allow))
+   return -1;
+   acl = alloc->acl;
}
}
return 0;
-- 
2.4.3

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


Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-22 Thread Andreas Gruenbacher
2015-09-21 21:24 GMT+02:00 J. Bruce Fields :
> On Fri, Sep 18, 2015 at 05:56:11PM -0400, bfields wrote:
>> On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote:
>> > +   /*
>> > +* If the owner mask contains permissions which are not in the group
>> > +* mask, the group mask contains permissions which are not in the other
>> > +* mask, or the owner class contains permissions which are not in the
>>
>> s/owner class/owner mask?
>>
>> > +* other mask, we may need to propagate permissions up from the
>> > +* everyone@ allow ace.  The third condition is implied by the first
>> > +* two.
>> > +*/
>> > +   if (!((acl->a_owner_mask & ~acl->a_group_mask) ||
>> > + (acl->a_group_mask & ~acl->a_other_mask)))
>> > +   return 0;
>>
>> The code looks right, but I don't understand the preceding comment.
>>
>> For example,
>>
>>   owner mask: rw
>>   group mask:  wx
>>   other mask: rw
>>
>> satisfies the first two conditions, but not the third.
>>
>> Also, I don't understand why the first condition would imply that we
>> might need to propagate permissions.
>
> OK, maybe I get the part about the owner mask containing permissions
> not in the group mask: we'll need to insert a deny ace for the bits in
> the other mask but not in the group mask, and then we'll need an allow
> ace for the owner to get those bits back.  I think?

That is indeed the reason, and it also seems clear that this wasn't
documented well enough. Let me remove the offending comment and tiny
optimization, and add better comments instead.

>> > +   if (richace_is_allow(ace) || richace_is_deny(ace)) {
>
> The v4 spec allows aces other than allow and deny aces (audit and
> alarm), but I didn't think you were implementing those.

Right, I don't see that happening. I'll remove that as well.

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


Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-22 Thread Andreas Gruenbacher
Here are my improvements; hope that helps ...

Thanks,
Andreas

diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
index 9b76fc0..21af9a0 100644
--- a/fs/richacl_compat.c
+++ b/fs/richacl_compat.c
@@ -351,26 +351,26 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
struct richace *ace;
unsigned int owner_allow, group_allow;
 
-   /*
-* If the owner mask contains permissions which are not in the group
-* mask, the group mask contains permissions which are not in the other
-* mask, or the owner class contains permissions which are not in the
-* other mask, we may need to propagate permissions up from the
-* everyone@ allow ace.  The third condition is implied by the first
-* two.
-*/
-   if (!((acl->a_owner_mask & ~acl->a_group_mask) ||
- (acl->a_group_mask & ~acl->a_other_mask)))
-   return 0;
if (!acl->a_count)
return 0;
ace = acl->a_entries + acl->a_count - 1;
if (richace_is_inherit_only(ace) || !richace_is_everyone(ace))
return 0;
 
+   /*
+* Permissions the owner and group class are granted through the
+* trailing everyone@ allow ace.
+*/
owner_allow = ace->e_mask & acl->a_owner_mask;
group_allow = ace->e_mask & acl->a_group_mask;
 
+   /*
+* If the group or other masks hide permissions which the owner should
+* be allowed, we need to propagate those permissions up.  Otherwise,
+* those permissions may be lost when applying the other mask to the
+* trailing everyone@ allow ace, or when isolating the group class from
+* the other class through additional deny aces.
+*/
if (owner_allow & ~(acl->a_group_mask & acl->a_other_mask)) {
/* Propagate everyone@ permissions through to owner@. */
who.e_id.special = RICHACE_OWNER_SPECIAL_ID;
@@ -379,6 +379,11 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
acl = alloc->acl;
}
 
+   /*
+* If the other mask hides permissions which the group class should be
+* allowed, we need to propagate those permissions up to the owning
+* group and to all other members in the group class.
+*/
if (group_allow & ~acl->a_other_mask) {
int n;
 
@@ -399,16 +404,15 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
richace_is_owner(ace) ||
richace_is_group(ace))
continue;
-   if (richace_is_allow(ace) || richace_is_deny(ace)) {
-   /*
-* Any inserted entry will end up below the
-* current entry
-*/
-   if (__richacl_propagate_everyone(alloc, ace,
-group_allow))
-   return -1;
-   acl = alloc->acl;
-   }
+
+   /*
+* Any inserted entry will end up below the current
+* entry.
+*/
+   if (__richacl_propagate_everyone(alloc, ace,
+group_allow))
+   return -1;
+   acl = alloc->acl;
}
}
return 0;
-- 
2.4.3

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


Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-22 Thread Andreas Gruenbacher
2015-09-21 21:24 GMT+02:00 J. Bruce Fields :
> On Fri, Sep 18, 2015 at 05:56:11PM -0400, bfields wrote:
>> On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote:
>> > +   /*
>> > +* If the owner mask contains permissions which are not in the group
>> > +* mask, the group mask contains permissions which are not in the other
>> > +* mask, or the owner class contains permissions which are not in the
>>
>> s/owner class/owner mask?
>>
>> > +* other mask, we may need to propagate permissions up from the
>> > +* everyone@ allow ace.  The third condition is implied by the first
>> > +* two.
>> > +*/
>> > +   if (!((acl->a_owner_mask & ~acl->a_group_mask) ||
>> > + (acl->a_group_mask & ~acl->a_other_mask)))
>> > +   return 0;
>>
>> The code looks right, but I don't understand the preceding comment.
>>
>> For example,
>>
>>   owner mask: rw
>>   group mask:  wx
>>   other mask: rw
>>
>> satisfies the first two conditions, but not the third.
>>
>> Also, I don't understand why the first condition would imply that we
>> might need to propagate permissions.
>
> OK, maybe I get the part about the owner mask containing permissions
> not in the group mask: we'll need to insert a deny ace for the bits in
> the other mask but not in the group mask, and then we'll need an allow
> ace for the owner to get those bits back.  I think?

That is indeed the reason, and it also seems clear that this wasn't
documented well enough. Let me remove the offending comment and tiny
optimization, and add better comments instead.

>> > +   if (richace_is_allow(ace) || richace_is_deny(ace)) {
>
> The v4 spec allows aces other than allow and deny aces (audit and
> alarm), but I didn't think you were implementing those.

Right, I don't see that happening. I'll remove that as well.

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


Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-22 Thread J. Bruce Fields
On Wed, Sep 23, 2015 at 03:39:44AM +0200, Andreas Gruenbacher wrote:
> Here are my improvements; hope that helps ...

Yes, looks good, thanks!--b.

> 
> Thanks,
> Andreas
> 
> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> index 9b76fc0..21af9a0 100644
> --- a/fs/richacl_compat.c
> +++ b/fs/richacl_compat.c
> @@ -351,26 +351,26 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
>   struct richace *ace;
>   unsigned int owner_allow, group_allow;
>  
> - /*
> -  * If the owner mask contains permissions which are not in the group
> -  * mask, the group mask contains permissions which are not in the other
> -  * mask, or the owner class contains permissions which are not in the
> -  * other mask, we may need to propagate permissions up from the
> -  * everyone@ allow ace.  The third condition is implied by the first
> -  * two.
> -  */
> - if (!((acl->a_owner_mask & ~acl->a_group_mask) ||
> -   (acl->a_group_mask & ~acl->a_other_mask)))
> - return 0;
>   if (!acl->a_count)
>   return 0;
>   ace = acl->a_entries + acl->a_count - 1;
>   if (richace_is_inherit_only(ace) || !richace_is_everyone(ace))
>   return 0;
>  
> + /*
> +  * Permissions the owner and group class are granted through the
> +  * trailing everyone@ allow ace.
> +  */
>   owner_allow = ace->e_mask & acl->a_owner_mask;
>   group_allow = ace->e_mask & acl->a_group_mask;
>  
> + /*
> +  * If the group or other masks hide permissions which the owner should
> +  * be allowed, we need to propagate those permissions up.  Otherwise,
> +  * those permissions may be lost when applying the other mask to the
> +  * trailing everyone@ allow ace, or when isolating the group class from
> +  * the other class through additional deny aces.
> +  */
>   if (owner_allow & ~(acl->a_group_mask & acl->a_other_mask)) {
>   /* Propagate everyone@ permissions through to owner@. */
>   who.e_id.special = RICHACE_OWNER_SPECIAL_ID;
> @@ -379,6 +379,11 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
>   acl = alloc->acl;
>   }
>  
> + /*
> +  * If the other mask hides permissions which the group class should be
> +  * allowed, we need to propagate those permissions up to the owning
> +  * group and to all other members in the group class.
> +  */
>   if (group_allow & ~acl->a_other_mask) {
>   int n;
>  
> @@ -399,16 +404,15 @@ richacl_propagate_everyone(struct richacl_alloc *alloc)
>   richace_is_owner(ace) ||
>   richace_is_group(ace))
>   continue;
> - if (richace_is_allow(ace) || richace_is_deny(ace)) {
> - /*
> -  * Any inserted entry will end up below the
> -  * current entry
> -  */
> - if (__richacl_propagate_everyone(alloc, ace,
> -  group_allow))
> - return -1;
> - acl = alloc->acl;
> - }
> +
> + /*
> +  * Any inserted entry will end up below the current
> +  * entry.
> +  */
> + if (__richacl_propagate_everyone(alloc, ace,
> +  group_allow))
> + return -1;
> + acl = alloc->acl;
>   }
>   }
>   return 0;
> -- 
> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-21 Thread Andreas Gruenbacher
2015-09-18 23:36 GMT+02:00 J. Bruce Fields :
> On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote:
>> + if (!richace_is_owner(who) &&
>> + richace_is_everyone(ace) && richace_is_allow(ace) &&
>
> That richace_is_allow(ace) check is redundant at this point, isn't it?

Yes, I'll change that.

>> + !(allow & ~(ace->e_mask & acl->a_other_mask)))
>
> Uh, I wish C had a subset-of operator, that construct took me longer to
> work out than I should admit.
>
>> + allow = 0;
>> +
>> + if (allow) {
>> + if (allow_last)
>> + return richace_change_mask(alloc, _last,
>> +allow_last->e_mask | allow);
>> + else {
>> + struct richace who_copy;
>> +
>> + richace_copy(_copy, who);
>> + ace = acl->a_entries + acl->a_count - 1;
>
> Isn't ace already set to the last ace?

Yes indeed, that line can also go.

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


Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-21 Thread J. Bruce Fields
On Fri, Sep 18, 2015 at 05:56:11PM -0400, bfields wrote:
> On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote:
> > The trailing everyone@ allow ace can grant permissions to all file
> > classes including the owner and group class.  Before we can apply the
> > other mask to this entry to turn it into an "other class" entry, we need
> > to ensure that members of the owner or group class will not lose any
> > permissions from that ace.
> > 
> > Conceptually, we do this by inserting additional :::allow
> > entries before the trailing everyone@ allow ace with the same
> > permissions as the trailing everyone@ allow ace for owner@, group@, and
> > all explicitly mentioned users and groups.  (In practice, we will rarely
> > need to insert any additional aces in this step.)
> > 
> > Signed-off-by: Andreas Gruenbacher 
> > ---
> >  fs/richacl_compat.c | 195 
> > 
> >  1 file changed, 195 insertions(+)
> > 
> > diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> > index 4f0acf5..9b76fc0 100644
> > --- a/fs/richacl_compat.c
> > +++ b/fs/richacl_compat.c
> > @@ -218,3 +218,198 @@ richacl_move_everyone_aces_down(struct richacl_alloc 
> > *alloc)
> > }
> > return 0;
> >  }
> > +
> > +/**
> > + * __richacl_propagate_everyone  -  propagate everyone@ permissions up for 
> > @who
> > + * @alloc: acl and number of allocated entries
> > + * @who:   identifier to propagate permissions for
> > + * @allow: permissions to propagate up
> > + *
> > + * Propagate the permissions in @allow up from the end of the acl to the 
> > start
> > + * for the specified principal @who.
> > + *
> > + * The simplest possible approach to achieve this would be to insert a
> > + * ":::allow" ace before the final everyone@ allow ace.  Since 
> > this
> > + * would often result in aces which are not needed or which could be merged
> > + * with an existing ace, we make the following optimizations:
> > + *
> > + *   - We go through the acl and determine which permissions are already
> > + * allowed or denied to @who, and we remove those permissions from
> > + * @allow.
> > + *
> > + *   - If the acl contains an allow ace for @who and no aces after this 
> > entry
> > + * deny permissions in @allow, we add the permissions in @allow to this
> > + * ace.  (Propagating permissions across a deny ace which can match the
> > + * process can elevate permissions.)
> > + *
> > + * This transformation does not alter the permissions that the acl grants.
> > + */
> > +static int
> > +__richacl_propagate_everyone(struct richacl_alloc *alloc, struct richace 
> > *who,
> > +unsigned int allow)
> > +{
> > +   struct richace *allow_last = NULL, *ace;
> > +   struct richacl *acl = alloc->acl;
> > +
> > +   /*
> > +* Remove the permissions from allow that are already determined for
> > +* this who value, and figure out if there is an allow entry for
> > +* this who value that is "reachable" from the trailing everyone@
> > +* allow ace.
> > +*/
> > +   richacl_for_each_entry(ace, acl) {
> > +   if (richace_is_inherit_only(ace))
> > +   continue;
> > +   if (richace_is_allow(ace)) {
> > +   if (richace_is_same_identifier(ace, who)) {
> > +   allow &= ~ace->e_mask;
> > +   allow_last = ace;
> > +   }
> > +   } else if (richace_is_deny(ace)) {
> > +   if (richace_is_same_identifier(ace, who))
> > +   allow &= ~ace->e_mask;
> > +   else if (allow & ace->e_mask)
> > +   allow_last = NULL;
> > +   }
> > +   }
> > +   ace--;
> > +
> > +   /*
> > +* If for group class entries, all the remaining permissions will
> > +* remain granted by the trailing everyone@ ace, no additional entry is
> > +* needed.
> > +*/
> > +   if (!richace_is_owner(who) &&
> > +   richace_is_everyone(ace) && richace_is_allow(ace) &&
> > +   !(allow & ~(ace->e_mask & acl->a_other_mask)))
> > +   allow = 0;
> > +
> > +   if (allow) {
> > +   if (allow_last)
> > +   return richace_change_mask(alloc, _last,
> > +  allow_last->e_mask | allow);
> > +   else {
> > +   struct richace who_copy;
> > +
> > +   richace_copy(_copy, who);
> > +   ace = acl->a_entries + acl->a_count - 1;
> > +   if (richacl_insert_entry(alloc, ))
> > +   return -1;
> > +   richace_copy(ace, _copy);
> > +   ace->e_type = RICHACE_ACCESS_ALLOWED_ACE_TYPE;
> > +   ace->e_flags &= ~RICHACE_INHERITANCE_FLAGS;
> > +   ace->e_mask = allow;
> > +   }
> > +   }
> > +   return 0;
> > +}
> > +
> > +/**
> > + * richacl_propagate_everyone  -  

Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-21 Thread Andreas Gruenbacher
2015-09-18 23:36 GMT+02:00 J. Bruce Fields :
> On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote:
>> + if (!richace_is_owner(who) &&
>> + richace_is_everyone(ace) && richace_is_allow(ace) &&
>
> That richace_is_allow(ace) check is redundant at this point, isn't it?

Yes, I'll change that.

>> + !(allow & ~(ace->e_mask & acl->a_other_mask)))
>
> Uh, I wish C had a subset-of operator, that construct took me longer to
> work out than I should admit.
>
>> + allow = 0;
>> +
>> + if (allow) {
>> + if (allow_last)
>> + return richace_change_mask(alloc, _last,
>> +allow_last->e_mask | allow);
>> + else {
>> + struct richace who_copy;
>> +
>> + richace_copy(_copy, who);
>> + ace = acl->a_entries + acl->a_count - 1;
>
> Isn't ace already set to the last ace?

Yes indeed, that line can also go.

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


Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-21 Thread J. Bruce Fields
On Fri, Sep 18, 2015 at 05:56:11PM -0400, bfields wrote:
> On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote:
> > The trailing everyone@ allow ace can grant permissions to all file
> > classes including the owner and group class.  Before we can apply the
> > other mask to this entry to turn it into an "other class" entry, we need
> > to ensure that members of the owner or group class will not lose any
> > permissions from that ace.
> > 
> > Conceptually, we do this by inserting additional :::allow
> > entries before the trailing everyone@ allow ace with the same
> > permissions as the trailing everyone@ allow ace for owner@, group@, and
> > all explicitly mentioned users and groups.  (In practice, we will rarely
> > need to insert any additional aces in this step.)
> > 
> > Signed-off-by: Andreas Gruenbacher 
> > ---
> >  fs/richacl_compat.c | 195 
> > 
> >  1 file changed, 195 insertions(+)
> > 
> > diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> > index 4f0acf5..9b76fc0 100644
> > --- a/fs/richacl_compat.c
> > +++ b/fs/richacl_compat.c
> > @@ -218,3 +218,198 @@ richacl_move_everyone_aces_down(struct richacl_alloc 
> > *alloc)
> > }
> > return 0;
> >  }
> > +
> > +/**
> > + * __richacl_propagate_everyone  -  propagate everyone@ permissions up for 
> > @who
> > + * @alloc: acl and number of allocated entries
> > + * @who:   identifier to propagate permissions for
> > + * @allow: permissions to propagate up
> > + *
> > + * Propagate the permissions in @allow up from the end of the acl to the 
> > start
> > + * for the specified principal @who.
> > + *
> > + * The simplest possible approach to achieve this would be to insert a
> > + * ":::allow" ace before the final everyone@ allow ace.  Since 
> > this
> > + * would often result in aces which are not needed or which could be merged
> > + * with an existing ace, we make the following optimizations:
> > + *
> > + *   - We go through the acl and determine which permissions are already
> > + * allowed or denied to @who, and we remove those permissions from
> > + * @allow.
> > + *
> > + *   - If the acl contains an allow ace for @who and no aces after this 
> > entry
> > + * deny permissions in @allow, we add the permissions in @allow to this
> > + * ace.  (Propagating permissions across a deny ace which can match the
> > + * process can elevate permissions.)
> > + *
> > + * This transformation does not alter the permissions that the acl grants.
> > + */
> > +static int
> > +__richacl_propagate_everyone(struct richacl_alloc *alloc, struct richace 
> > *who,
> > +unsigned int allow)
> > +{
> > +   struct richace *allow_last = NULL, *ace;
> > +   struct richacl *acl = alloc->acl;
> > +
> > +   /*
> > +* Remove the permissions from allow that are already determined for
> > +* this who value, and figure out if there is an allow entry for
> > +* this who value that is "reachable" from the trailing everyone@
> > +* allow ace.
> > +*/
> > +   richacl_for_each_entry(ace, acl) {
> > +   if (richace_is_inherit_only(ace))
> > +   continue;
> > +   if (richace_is_allow(ace)) {
> > +   if (richace_is_same_identifier(ace, who)) {
> > +   allow &= ~ace->e_mask;
> > +   allow_last = ace;
> > +   }
> > +   } else if (richace_is_deny(ace)) {
> > +   if (richace_is_same_identifier(ace, who))
> > +   allow &= ~ace->e_mask;
> > +   else if (allow & ace->e_mask)
> > +   allow_last = NULL;
> > +   }
> > +   }
> > +   ace--;
> > +
> > +   /*
> > +* If for group class entries, all the remaining permissions will
> > +* remain granted by the trailing everyone@ ace, no additional entry is
> > +* needed.
> > +*/
> > +   if (!richace_is_owner(who) &&
> > +   richace_is_everyone(ace) && richace_is_allow(ace) &&
> > +   !(allow & ~(ace->e_mask & acl->a_other_mask)))
> > +   allow = 0;
> > +
> > +   if (allow) {
> > +   if (allow_last)
> > +   return richace_change_mask(alloc, _last,
> > +  allow_last->e_mask | allow);
> > +   else {
> > +   struct richace who_copy;
> > +
> > +   richace_copy(_copy, who);
> > +   ace = acl->a_entries + acl->a_count - 1;
> > +   if (richacl_insert_entry(alloc, ))
> > +   return -1;
> > +   richace_copy(ace, _copy);
> > +   ace->e_type = RICHACE_ACCESS_ALLOWED_ACE_TYPE;
> > +   ace->e_flags &= ~RICHACE_INHERITANCE_FLAGS;
> > +   ace->e_mask = allow;
> > +   }
> > +   }
> > +   return 0;
> > +}
> > +
> > +/**
> > + * 

Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-18 Thread J. Bruce Fields
On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote:
> The trailing everyone@ allow ace can grant permissions to all file
> classes including the owner and group class.  Before we can apply the
> other mask to this entry to turn it into an "other class" entry, we need
> to ensure that members of the owner or group class will not lose any
> permissions from that ace.
> 
> Conceptually, we do this by inserting additional :::allow
> entries before the trailing everyone@ allow ace with the same
> permissions as the trailing everyone@ allow ace for owner@, group@, and
> all explicitly mentioned users and groups.  (In practice, we will rarely
> need to insert any additional aces in this step.)
> 
> Signed-off-by: Andreas Gruenbacher 
> ---
>  fs/richacl_compat.c | 195 
> 
>  1 file changed, 195 insertions(+)
> 
> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> index 4f0acf5..9b76fc0 100644
> --- a/fs/richacl_compat.c
> +++ b/fs/richacl_compat.c
> @@ -218,3 +218,198 @@ richacl_move_everyone_aces_down(struct richacl_alloc 
> *alloc)
>   }
>   return 0;
>  }
> +
> +/**
> + * __richacl_propagate_everyone  -  propagate everyone@ permissions up for 
> @who
> + * @alloc:   acl and number of allocated entries
> + * @who: identifier to propagate permissions for
> + * @allow:   permissions to propagate up
> + *
> + * Propagate the permissions in @allow up from the end of the acl to the 
> start
> + * for the specified principal @who.
> + *
> + * The simplest possible approach to achieve this would be to insert a
> + * ":::allow" ace before the final everyone@ allow ace.  Since 
> this
> + * would often result in aces which are not needed or which could be merged
> + * with an existing ace, we make the following optimizations:
> + *
> + *   - We go through the acl and determine which permissions are already
> + * allowed or denied to @who, and we remove those permissions from
> + * @allow.
> + *
> + *   - If the acl contains an allow ace for @who and no aces after this entry
> + * deny permissions in @allow, we add the permissions in @allow to this
> + * ace.  (Propagating permissions across a deny ace which can match the
> + * process can elevate permissions.)
> + *
> + * This transformation does not alter the permissions that the acl grants.
> + */
> +static int
> +__richacl_propagate_everyone(struct richacl_alloc *alloc, struct richace 
> *who,
> +  unsigned int allow)
> +{
> + struct richace *allow_last = NULL, *ace;
> + struct richacl *acl = alloc->acl;
> +
> + /*
> +  * Remove the permissions from allow that are already determined for
> +  * this who value, and figure out if there is an allow entry for
> +  * this who value that is "reachable" from the trailing everyone@
> +  * allow ace.
> +  */
> + richacl_for_each_entry(ace, acl) {
> + if (richace_is_inherit_only(ace))
> + continue;
> + if (richace_is_allow(ace)) {
> + if (richace_is_same_identifier(ace, who)) {
> + allow &= ~ace->e_mask;
> + allow_last = ace;
> + }
> + } else if (richace_is_deny(ace)) {
> + if (richace_is_same_identifier(ace, who))
> + allow &= ~ace->e_mask;
> + else if (allow & ace->e_mask)
> + allow_last = NULL;
> + }
> + }
> + ace--;
> +
> + /*
> +  * If for group class entries, all the remaining permissions will
> +  * remain granted by the trailing everyone@ ace, no additional entry is
> +  * needed.
> +  */
> + if (!richace_is_owner(who) &&
> + richace_is_everyone(ace) && richace_is_allow(ace) &&
> + !(allow & ~(ace->e_mask & acl->a_other_mask)))
> + allow = 0;
> +
> + if (allow) {
> + if (allow_last)
> + return richace_change_mask(alloc, _last,
> +allow_last->e_mask | allow);
> + else {
> + struct richace who_copy;
> +
> + richace_copy(_copy, who);
> + ace = acl->a_entries + acl->a_count - 1;
> + if (richacl_insert_entry(alloc, ))
> + return -1;
> + richace_copy(ace, _copy);
> + ace->e_type = RICHACE_ACCESS_ALLOWED_ACE_TYPE;
> + ace->e_flags &= ~RICHACE_INHERITANCE_FLAGS;
> + ace->e_mask = allow;
> + }
> + }
> + return 0;
> +}
> +
> +/**
> + * richacl_propagate_everyone  -  propagate everyone@ permissions up the acl
> + * @alloc:   acl and number of allocated entries
> + *
> + * Make sure that group@ and all other users and groups mentioned in the acl
> + * 

Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-18 Thread J. Bruce Fields
On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote:
> The trailing everyone@ allow ace can grant permissions to all file
> classes including the owner and group class.  Before we can apply the
> other mask to this entry to turn it into an "other class" entry, we need
> to ensure that members of the owner or group class will not lose any
> permissions from that ace.
> 
> Conceptually, we do this by inserting additional :::allow
> entries before the trailing everyone@ allow ace with the same
> permissions as the trailing everyone@ allow ace for owner@, group@, and
> all explicitly mentioned users and groups.  (In practice, we will rarely
> need to insert any additional aces in this step.)
> 
> Signed-off-by: Andreas Gruenbacher 
> ---
>  fs/richacl_compat.c | 195 
> 
>  1 file changed, 195 insertions(+)
> 
> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> index 4f0acf5..9b76fc0 100644
> --- a/fs/richacl_compat.c
> +++ b/fs/richacl_compat.c
> @@ -218,3 +218,198 @@ richacl_move_everyone_aces_down(struct richacl_alloc 
> *alloc)
>   }
>   return 0;
>  }
> +
> +/**
> + * __richacl_propagate_everyone  -  propagate everyone@ permissions up for 
> @who
> + * @alloc:   acl and number of allocated entries
> + * @who: identifier to propagate permissions for
> + * @allow:   permissions to propagate up
> + *
> + * Propagate the permissions in @allow up from the end of the acl to the 
> start
> + * for the specified principal @who.
> + *
> + * The simplest possible approach to achieve this would be to insert a
> + * ":::allow" ace before the final everyone@ allow ace.  Since 
> this
> + * would often result in aces which are not needed or which could be merged
> + * with an existing ace, we make the following optimizations:
> + *
> + *   - We go through the acl and determine which permissions are already
> + * allowed or denied to @who, and we remove those permissions from
> + * @allow.
> + *
> + *   - If the acl contains an allow ace for @who and no aces after this entry
> + * deny permissions in @allow, we add the permissions in @allow to this
> + * ace.  (Propagating permissions across a deny ace which can match the
> + * process can elevate permissions.)
> + *
> + * This transformation does not alter the permissions that the acl grants.
> + */
> +static int
> +__richacl_propagate_everyone(struct richacl_alloc *alloc, struct richace 
> *who,
> +  unsigned int allow)
> +{
> + struct richace *allow_last = NULL, *ace;
> + struct richacl *acl = alloc->acl;
> +
> + /*
> +  * Remove the permissions from allow that are already determined for
> +  * this who value, and figure out if there is an allow entry for
> +  * this who value that is "reachable" from the trailing everyone@
> +  * allow ace.
> +  */
> + richacl_for_each_entry(ace, acl) {
> + if (richace_is_inherit_only(ace))
> + continue;
> + if (richace_is_allow(ace)) {
> + if (richace_is_same_identifier(ace, who)) {
> + allow &= ~ace->e_mask;
> + allow_last = ace;
> + }
> + } else if (richace_is_deny(ace)) {
> + if (richace_is_same_identifier(ace, who))
> + allow &= ~ace->e_mask;
> + else if (allow & ace->e_mask)
> + allow_last = NULL;
> + }
> + }
> + ace--;
> +
> + /*
> +  * If for group class entries, all the remaining permissions will
> +  * remain granted by the trailing everyone@ ace, no additional entry is
> +  * needed.
> +  */
> + if (!richace_is_owner(who) &&
> + richace_is_everyone(ace) && richace_is_allow(ace) &&

That richace_is_allow(ace) check is redundant at this point, isn't it?

> + !(allow & ~(ace->e_mask & acl->a_other_mask)))

Uh, I wish C had a subset-of operator, that construct took me longer to
work out than I should admit.

> + allow = 0;
> +
> + if (allow) {
> + if (allow_last)
> + return richace_change_mask(alloc, _last,
> +allow_last->e_mask | allow);
> + else {
> + struct richace who_copy;
> +
> + richace_copy(_copy, who);
> + ace = acl->a_entries + acl->a_count - 1;

Isn't ace already set to the last ace?

--b.

> + if (richacl_insert_entry(alloc, ))
> + return -1;
> + richace_copy(ace, _copy);
> + ace->e_type = RICHACE_ACCESS_ALLOWED_ACE_TYPE;
> + ace->e_flags &= ~RICHACE_INHERITANCE_FLAGS;
> + ace->e_mask = allow;
> + }
> + }
> + return 0;
> +}
> +
> +/**
> + 

Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-18 Thread J. Bruce Fields
On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote:
> The trailing everyone@ allow ace can grant permissions to all file
> classes including the owner and group class.  Before we can apply the
> other mask to this entry to turn it into an "other class" entry, we need
> to ensure that members of the owner or group class will not lose any
> permissions from that ace.
> 
> Conceptually, we do this by inserting additional :::allow
> entries before the trailing everyone@ allow ace with the same
> permissions as the trailing everyone@ allow ace for owner@, group@, and
> all explicitly mentioned users and groups.  (In practice, we will rarely
> need to insert any additional aces in this step.)
> 
> Signed-off-by: Andreas Gruenbacher 
> ---
>  fs/richacl_compat.c | 195 
> 
>  1 file changed, 195 insertions(+)
> 
> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> index 4f0acf5..9b76fc0 100644
> --- a/fs/richacl_compat.c
> +++ b/fs/richacl_compat.c
> @@ -218,3 +218,198 @@ richacl_move_everyone_aces_down(struct richacl_alloc 
> *alloc)
>   }
>   return 0;
>  }
> +
> +/**
> + * __richacl_propagate_everyone  -  propagate everyone@ permissions up for 
> @who
> + * @alloc:   acl and number of allocated entries
> + * @who: identifier to propagate permissions for
> + * @allow:   permissions to propagate up
> + *
> + * Propagate the permissions in @allow up from the end of the acl to the 
> start
> + * for the specified principal @who.
> + *
> + * The simplest possible approach to achieve this would be to insert a
> + * ":::allow" ace before the final everyone@ allow ace.  Since 
> this
> + * would often result in aces which are not needed or which could be merged
> + * with an existing ace, we make the following optimizations:
> + *
> + *   - We go through the acl and determine which permissions are already
> + * allowed or denied to @who, and we remove those permissions from
> + * @allow.
> + *
> + *   - If the acl contains an allow ace for @who and no aces after this entry
> + * deny permissions in @allow, we add the permissions in @allow to this
> + * ace.  (Propagating permissions across a deny ace which can match the
> + * process can elevate permissions.)
> + *
> + * This transformation does not alter the permissions that the acl grants.
> + */
> +static int
> +__richacl_propagate_everyone(struct richacl_alloc *alloc, struct richace 
> *who,
> +  unsigned int allow)
> +{
> + struct richace *allow_last = NULL, *ace;
> + struct richacl *acl = alloc->acl;
> +
> + /*
> +  * Remove the permissions from allow that are already determined for
> +  * this who value, and figure out if there is an allow entry for
> +  * this who value that is "reachable" from the trailing everyone@
> +  * allow ace.
> +  */
> + richacl_for_each_entry(ace, acl) {
> + if (richace_is_inherit_only(ace))
> + continue;
> + if (richace_is_allow(ace)) {
> + if (richace_is_same_identifier(ace, who)) {
> + allow &= ~ace->e_mask;
> + allow_last = ace;
> + }
> + } else if (richace_is_deny(ace)) {
> + if (richace_is_same_identifier(ace, who))
> + allow &= ~ace->e_mask;
> + else if (allow & ace->e_mask)
> + allow_last = NULL;
> + }
> + }
> + ace--;
> +
> + /*
> +  * If for group class entries, all the remaining permissions will
> +  * remain granted by the trailing everyone@ ace, no additional entry is
> +  * needed.
> +  */
> + if (!richace_is_owner(who) &&
> + richace_is_everyone(ace) && richace_is_allow(ace) &&

That richace_is_allow(ace) check is redundant at this point, isn't it?

> + !(allow & ~(ace->e_mask & acl->a_other_mask)))

Uh, I wish C had a subset-of operator, that construct took me longer to
work out than I should admit.

> + allow = 0;
> +
> + if (allow) {
> + if (allow_last)
> + return richace_change_mask(alloc, _last,
> +allow_last->e_mask | allow);
> + else {
> + struct richace who_copy;
> +
> + richace_copy(_copy, who);
> + ace = acl->a_entries + acl->a_count - 1;

Isn't ace already set to the last ace?

--b.

> + if (richacl_insert_entry(alloc, ))
> + return -1;
> + richace_copy(ace, _copy);
> + ace->e_type = RICHACE_ACCESS_ALLOWED_ACE_TYPE;
> + ace->e_flags &= ~RICHACE_INHERITANCE_FLAGS;
> + ace->e_mask = allow;
> + }
> + }
> + return 0;

Re: [RFC v7 22/41] richacl: Propagate everyone@ permissions to other aces

2015-09-18 Thread J. Bruce Fields
On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote:
> The trailing everyone@ allow ace can grant permissions to all file
> classes including the owner and group class.  Before we can apply the
> other mask to this entry to turn it into an "other class" entry, we need
> to ensure that members of the owner or group class will not lose any
> permissions from that ace.
> 
> Conceptually, we do this by inserting additional :::allow
> entries before the trailing everyone@ allow ace with the same
> permissions as the trailing everyone@ allow ace for owner@, group@, and
> all explicitly mentioned users and groups.  (In practice, we will rarely
> need to insert any additional aces in this step.)
> 
> Signed-off-by: Andreas Gruenbacher 
> ---
>  fs/richacl_compat.c | 195 
> 
>  1 file changed, 195 insertions(+)
> 
> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> index 4f0acf5..9b76fc0 100644
> --- a/fs/richacl_compat.c
> +++ b/fs/richacl_compat.c
> @@ -218,3 +218,198 @@ richacl_move_everyone_aces_down(struct richacl_alloc 
> *alloc)
>   }
>   return 0;
>  }
> +
> +/**
> + * __richacl_propagate_everyone  -  propagate everyone@ permissions up for 
> @who
> + * @alloc:   acl and number of allocated entries
> + * @who: identifier to propagate permissions for
> + * @allow:   permissions to propagate up
> + *
> + * Propagate the permissions in @allow up from the end of the acl to the 
> start
> + * for the specified principal @who.
> + *
> + * The simplest possible approach to achieve this would be to insert a
> + * ":::allow" ace before the final everyone@ allow ace.  Since 
> this
> + * would often result in aces which are not needed or which could be merged
> + * with an existing ace, we make the following optimizations:
> + *
> + *   - We go through the acl and determine which permissions are already
> + * allowed or denied to @who, and we remove those permissions from
> + * @allow.
> + *
> + *   - If the acl contains an allow ace for @who and no aces after this entry
> + * deny permissions in @allow, we add the permissions in @allow to this
> + * ace.  (Propagating permissions across a deny ace which can match the
> + * process can elevate permissions.)
> + *
> + * This transformation does not alter the permissions that the acl grants.
> + */
> +static int
> +__richacl_propagate_everyone(struct richacl_alloc *alloc, struct richace 
> *who,
> +  unsigned int allow)
> +{
> + struct richace *allow_last = NULL, *ace;
> + struct richacl *acl = alloc->acl;
> +
> + /*
> +  * Remove the permissions from allow that are already determined for
> +  * this who value, and figure out if there is an allow entry for
> +  * this who value that is "reachable" from the trailing everyone@
> +  * allow ace.
> +  */
> + richacl_for_each_entry(ace, acl) {
> + if (richace_is_inherit_only(ace))
> + continue;
> + if (richace_is_allow(ace)) {
> + if (richace_is_same_identifier(ace, who)) {
> + allow &= ~ace->e_mask;
> + allow_last = ace;
> + }
> + } else if (richace_is_deny(ace)) {
> + if (richace_is_same_identifier(ace, who))
> + allow &= ~ace->e_mask;
> + else if (allow & ace->e_mask)
> + allow_last = NULL;
> + }
> + }
> + ace--;
> +
> + /*
> +  * If for group class entries, all the remaining permissions will
> +  * remain granted by the trailing everyone@ ace, no additional entry is
> +  * needed.
> +  */
> + if (!richace_is_owner(who) &&
> + richace_is_everyone(ace) && richace_is_allow(ace) &&
> + !(allow & ~(ace->e_mask & acl->a_other_mask)))
> + allow = 0;
> +
> + if (allow) {
> + if (allow_last)
> + return richace_change_mask(alloc, _last,
> +allow_last->e_mask | allow);
> + else {
> + struct richace who_copy;
> +
> + richace_copy(_copy, who);
> + ace = acl->a_entries + acl->a_count - 1;
> + if (richacl_insert_entry(alloc, ))
> + return -1;
> + richace_copy(ace, _copy);
> + ace->e_type = RICHACE_ACCESS_ALLOWED_ACE_TYPE;
> + ace->e_flags &= ~RICHACE_INHERITANCE_FLAGS;
> + ace->e_mask = allow;
> + }
> + }
> + return 0;
> +}
> +
> +/**
> + * richacl_propagate_everyone  -  propagate everyone@ permissions up the acl
> + * @alloc:   acl and number of allocated entries
> + *
> + * Make sure that group@ and all other users and groups mentioned