Re: [PATCH] capability: Remove unused has_capability

["Dr. David Alan Gilbert" <linux@treblig.org>]

* Paul Moore (paul@paul-moore.com) wrote:
> On Wed, Dec 18, 2024 at 5:11 PM Dr. David Alan Gilbert
> <linux@treblig.org> wrote:
> > * Paul Moore (paul@paul-moore.com) wrote:
> > > On Sun, Dec 15, 2024 at 11:54 AM <linux@treblig.org> wrote:
> > > >
> > > > From: "Dr. David Alan Gilbert" <linux@treblig.org>
> > > >
> > > > The vanilla has_capability() function has been unused since 2018's
> > > > commit dcb569cf6ac9 ("Smack: ptrace capability use fixes")
> > > >
> > > > Remove it.
> > > >
> > > > (There is still mention in a comment in security/commoncap.c
> > > > but I suspect rather than removing the entry it might be better
> > > > to expand the comment to talk about the other
> > > > has_[ns_]capability[_noaudit] variants).
> >
> > Hi Paul,
> >   Thanks for the review,
> >
> > > I would suggest that this patch would be an excellent place to change
> > > that comment.  Without historical knowledge, the comment will be hard
> > > to understand after this patch is merged as inspecting
> > > has_capability() will be much more difficult, and including the
> > > comment change with the function removal will bind the two changes
> > > nicely in the git log.
> >
> > Yeh, how would you like it? The existing comment is:
> >
> > '
> >  * NOTE WELL: cap_has_capability() cannot be used like the kernel's capable()
> >  * and has_capability() functions.  That is, it has the reverse semantics:
> >  * cap_has_capability() returns 0 when a task has a capability, but the
> >  * kernel's capable() and has_capability() returns 1 for this case.
> > '
> >
> > For a start I think that's wrong; the function it's above is
> > 'cap_capable()' not 'cap_has_capability()' - and has been for 15 years :-)
> 
> The code in security/commoncap.c is fairly mature and stable, and I
> don't expect that many people spend a lot of time in that file, I know
> I don't.  An unfortunate side effect is that certain things that
> aren't caught by a compiler can easily go out of date, and stay that
> way for some time :/

There are 'many eyes' scared to look!

> > How about:
> > '
> >  * NOTE WELL: cap_capable() has reverse semantics to the other kernel
> >  * functions. That is cap_capable() returns 0 when a task has a capability,
> >  * the kernel's capable(), has_ns_capability(), has_ns_capability_noaudit(),
> >  * and has_capability_noaudit() return 1 for this case.
> > '
> 
> Two things come to mind when reading the suggested comment:
> 
> * I don't like the "... reverse semantics to the other kernel
> functions" text simply because the majority of kernel functions do
> follow the "0 on success, negative errno on failure" pattern that we
> see in cap_capable().  I would suggest something along the lines of
> "... reverse semantics of the capable() call".
> 
> * Most (all?) of the capable() family of functions, excluding
> cap_capable() of course, return a bool value, true/false, instead of
> non-zero/zero.  If we're going to complain about the existing comment,
> we probably should get this correct ;)
> 

OK, maybe:

* NOTE WELL: cap_capable() has reverse semantics to the capable() call
* and friends. That is cap_capable() returns an int 0 when a task has
* a capability, while the kernel's capable(), has_ns_capability(),
* has_ns_capability_noaudit(), and has_capability_noaudit() return a
* bool true (1) for this case.

Dave

> -- 
> paul-moore.com
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/