Re: [PATCH] capability: Remove unused has_capability

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

* 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 :-)

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.
'

(It's curious how rarely most of these are used...)

> Otherwise, this seems fine to me.
> 
> Reviewed-by: Paul Moore <paul@paul-moore.com>

Thanks,

Dave

> > Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
> > ---
> >  include/linux/capability.h |  5 -----
> >  kernel/capability.c        | 16 ----------------
> >  2 files changed, 21 deletions(-)
> >
> > diff --git a/include/linux/capability.h b/include/linux/capability.h
> > index 0c356a517991..1fb08922552c 100644
> > --- a/include/linux/capability.h
> > +++ b/include/linux/capability.h
> > @@ -139,7 +139,6 @@ static inline kernel_cap_t cap_raise_nfsd_set(const kernel_cap_t a,
> >  }
> >
> >  #ifdef CONFIG_MULTIUSER
> > -extern bool has_capability(struct task_struct *t, int cap);
> >  extern bool has_ns_capability(struct task_struct *t,
> >                               struct user_namespace *ns, int cap);
> >  extern bool has_capability_noaudit(struct task_struct *t, int cap);
> > @@ -150,10 +149,6 @@ extern bool ns_capable(struct user_namespace *ns, int cap);
> >  extern bool ns_capable_noaudit(struct user_namespace *ns, int cap);
> >  extern bool ns_capable_setid(struct user_namespace *ns, int cap);
> >  #else
> > -static inline bool has_capability(struct task_struct *t, int cap)
> > -{
> > -       return true;
> > -}
> >  static inline bool has_ns_capability(struct task_struct *t,
> >                               struct user_namespace *ns, int cap)
> >  {
> > diff --git a/kernel/capability.c b/kernel/capability.c
> > index dac4df77e376..67094b628ea9 100644
> > --- a/kernel/capability.c
> > +++ b/kernel/capability.c
> > @@ -289,22 +289,6 @@ bool has_ns_capability(struct task_struct *t,
> >         return (ret == 0);
> >  }
> >
> > -/**
> > - * has_capability - Does a task have a capability in init_user_ns
> > - * @t: The task in question
> > - * @cap: The capability to be tested for
> > - *
> > - * Return true if the specified task has the given superior capability
> > - * currently in effect to the initial user namespace, false if not.
> > - *
> > - * Note that this does not set PF_SUPERPRIV on the task.
> > - */
> > -bool has_capability(struct task_struct *t, int cap)
> > -{
> > -       return has_ns_capability(t, &init_user_ns, cap);
> > -}
> > -EXPORT_SYMBOL(has_capability);
> > -
> >  /**
> >   * has_ns_capability_noaudit - Does a task have a capability (unaudited)
> >   * in a specific user ns.
> > --
> > 2.47.1
> 
> -- 
> 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   |_______/