| Message ID | 20241215165352.186692-1-linux@treblig.org (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | capability: Remove unused has_capability | expand |
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). 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. Otherwise, this seems fine to me. Reviewed-by: Paul Moore <paul@paul-moore.com> > 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 (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 >
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 :/ > 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 ;)
* 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 >
On Thu, Dec 19, 2024 at 9:19 AM Dr. David Alan Gilbert <linux@treblig.org> wrote: > > 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. Works for me, thanks.
* Paul Moore (paul@paul-moore.com) wrote: > On Thu, Dec 19, 2024 at 9:19 AM Dr. David Alan Gilbert > <linux@treblig.org> wrote: > > > > 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. > > Works for me, thanks. Thanks, v2 sent - see 20241219172859.188117-1-linux@treblig.org Dave > -- > paul-moore.com >
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.