Re: [PATCH] capability: Remove unused has_capability

Dr. David Alan Gilbert
2024-12-19T14:19:53Z

* 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 |_______/