Re: [GIT PULL] capabilities

["Serge E. Hallyn" <serge@hallyn.com>]

On Wed, Nov 27, 2024 at 09:30:14AM -0800, Linus Torvalds wrote:
> On Mon, 18 Nov 2024 at 07:26, <sergeh@kernel.org> wrote:
> >
> > 2. Add a trace event for cap_capable (Jordan Rome).
> 
> So I've finally gotten around to this, but I absolutely detest how
> this was written.
> 
> It was oddly written before, but now it's absolutely illegible.  All
> just to have one single tracepoint.
> 
> And it's all *stupid*.
> 
> The "capable_ns" thing is entirely pointless.
> 
> Why? It always has exactly one value: 'cred->user_ns'. Lookie here,
> it's assigned exactly twice:
> 
>                 if (ns == cred->user_ns) {
>                         if (cap_raised(cred->cap_effective, cap)) {
>                                 capable_ns = ns;
> ...
>                 if ((ns->parent == cred->user_ns) && uid_eq(ns->owner,
> cred->euid)) {
>                         capable_ns = ns->parent;
> 
> and *both* times it's assigned something that we just checked is equal
> to cred->user_ns.
> 
> And for this useless value, the already odd for-loop was written to be
> even more odd, and the code added a new variable 'capable_ns'.
> 
> So I pulled this, tried to figure out _why_ it was written that oddly,
> decided that the "why" was "because it's being stupid", and I unpulled
> it again.
> 
> If we really need that trace point, I have a few requirements:
> 
>  - none of this crazy stuff
> 
>  - use a simple inline helper
> 
>  - make the pointers 'const', because there is no reason not to.
> 
> Something *UNTESTED* like the attached diff.
> 
> Again: very untested. But at least this generates good code, and
> doesn't have pointless crazy variables. Yes, I add that
> 
>         const struct user_namespace *cred_ns = cred->user_ns;
> 
> because while I think gcc may be smart enough to figure out that it's
> all the same value, I wanted to make sure.
> 
> Then the tracepoint would look something like
> 
>         trace_cap_capable(cred, targ_ns,  cred_ns, cap, opts, ret);
> 
> although I don't understand why you'd even trace that 'opts' value
> that is never used.

You mean cap_capable doesn't use opts?  Yeah, it's used only by other
LSMs.  I suppose knowing the value might in some cases help to figure
out caller state, but dropping it seems sensible.

Jordan is working on a new version based on your feedback.

thanks,
-serge