Re: [PATCH V3 02/10] capabilities: intuitive names for cap gain status
On Thu, Aug 24, 2017 at 3:06 PM, Kees Cook <keescook(a)chromium.org> wrote:
On Thu, Aug 24, 2017 at 9:37 AM, Serge E. Hallyn
<serge(a)hallyn.com> wrote:
> Quoting Richard Guy Briggs (rgb(a)redhat.com):
>> On 2017-08-24 11:03, Serge E. Hallyn wrote:
>> > Quoting Richard Guy Briggs (rgb(a)redhat.com):
>> > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the
>> > > negation of is_subset() easier to read and analyse.
>> > >
>> > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
>> > > ---
>> > > security/commoncap.c | 16 ++++++++++------
>> > > 1 files changed, 10 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/security/commoncap.c b/security/commoncap.c
>> > > index b7fbf77..6f05ec0 100644
>> > > --- a/security/commoncap.c
>> > > +++ b/security/commoncap.c
>> > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm
*bprm, bool has_cap, bool *effec
>> > > *effective = true;
>> > > }
>> > >
>> >
>> > It's subjective and so might be just me, but I think I'd find it
easier
>> > to read if it was cap_gained(source, target, field) and cap_grew(cred,
source, target)
>>
>> In more than one place, I wanted to put the parameter that I was trying
>> to read aloud closest to the function name to make reading it flow
>> better, leaving the parameters less critical to comprehension towards
>> the end.
>
> And I see that in the final patch it looks nicer the way you have it.
>
>> > This looks correct though, so either way
>> >
>> > Reviewed-by: Serge Hallyn <serge(a)hallyn.com>
>>
>> Thanks. Did you want to put this through, or send it through Paul's
>> audit tree?
>
> If Paul's around I'm happy to have it go through his tree.
Since Serge is okay with these I'll take a closer look and if it all
looks good I can pull it in to the audit tree (no objections from me
on the last revision, although I remember it being much smaller).
That said, since we are already at -rc6, I'm going to defer merging
this into audit/next *after* the upcoming merge window. We are right
at where I normally draw the line and considering the scope and nature
of this patchset I think having a full RC cycle in linux-next would be
a good thing.
Is this series based against -next with the changes that touch
commoncap.c?
Also, did you validate this with the existing LTP tests and selftests?
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=...
Another reason for keeping this in the queue a bit longer. Richard,
can you do this testing before the upcoming merge window closes?
--
paul moore
www.paul-moore.com