On Fri, Jul 5, 2024 at 3:34 PM KP Singh [off-list ref] wrote:

On Fri, Jul 5, 2024 at 8:07 PM Paul Moore [off-list ref] wrote:

quoted

On Wed, Jul 3, 2024 at 7:08 PM KP Singh [off-list ref] wrote:

quoted

On Thu, Jul 4, 2024 at 12:52 AM Paul Moore [off-list ref] wrote:

quoted

On Wed, Jul 3, 2024 at 6:22 PM KP Singh [off-list ref] wrote:

quoted

On Wed, Jul 3, 2024 at 10:56 PM Paul Moore [off-list ref] wrote:

quoted

On Wed, Jul 3, 2024 at 12:55 PM KP Singh [off-list ref] wrote:

quoted

On Wed, Jul 3, 2024 at 2:07 AM Paul Moore [off-list ref] wrote:

quoted

On Jun 29, 2024 KP Singh [off-list ref] wrote:

quoted

LSM hooks are currently invoked from a linked list as indirect calls
which are invoked using retpolines as a mitigation for speculative
attacks (Branch History / Target injection) and add extra overhead which
is especially bad in kernel hot paths:

...

quoted

quoted

quoted

quoted

I'm not aware of any other existing problems relating to the LSM hook
default values, if there are any, we need to fix them independent of
this patchset.  The LSM framework should function properly if the
"default" values are used.

Patch 5 eliminates the possibilities of errors and subtle bugs all
together. The problem with subtle bugs is, well, they are subtle, if
you and I knew of the bugs, we would fix all of them, but we don't. I
really feel we ought to eliminate the class of issues and not just
whack-a-mole when we see the bugs.

Here's the thing, I don't really like patch 5/5.  To be honest, I
don't really like a lot of this patchset.  From my perspective, the
complexity of the code is likely going to mean more maintenance
headaches down the road, but Linus hath spoken so we're doing this
(although "this" is still a bit undefined as far as I'm concerned).
If you want me to merge patch 5/5 you've got to give me something real
and convincing that can't be fixed by any other means.  My current
opinion is that you're trying to use a previously fixed bug to scare
and/or coerce the merging of some changes I don't really want to
merge.  If you want me to take patch 5/5, you've got to give me a
reason that is far more compelling that what you've written thus far.

Paul, I am not scaring you, I am providing a solution that saves us
from headaches with side-effects and bugs in the future. It's safer by
design.

Perhaps I wasn't clear enough in my previous emails; instead of trying
to convince me that your solution is literally the best possible thing
to ever touch the kernel, convince me that there is a problem we need
to fix.  Right now, I'm not convinced there is a bug that requires all
of the extra code in patch 5/5 (all of which have the potential to
introduce new bugs).  As mentioned previously, the bugs that typically
have been used as examples of unwanted side effects with the LSM hooks
have been resolved, both in the specific and general case.  If you
want me to add more code/functionality to fix a bug, you must first
demonstrate the bug exists and the risk is real; you have not done
that as far as I'm concerned.

quoted

You say you have not reviewed it carefully ...

That may have been true of previous versions of this patchset, but I
did not say that about this current patchset.

quoted

... but you did ask me to move
the function from the BPF LSM layer to an LSM API, and we had a bunch
of discussion around naming in the subsequent revisions.

https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ (local)

That discussion predates commit 61df7b828204 ("lsm: fixup the inode
xattr capability handling") which is currently in the lsm/dev branch,
marked for stable, and will go up to Linus during the upcoming merge
window.

quoted

My reasons are:

1. It's safer, no side effects, guaranteed to be not buggy. Neither
you, nor me, can guarantee that a default value will be safe in the
LSM layer.

In the first sentence above you "guarantee" that your code is not
buggy and then follow that up with a second sentence discussing how no
one can guarantee source code safety.  Regardless of whatever point
you were trying to make here, I maintain that *all* patches have the
potential for bugs, even those that are attempting to fix bugs.  WithD
that in mind, if you want me to merge more code to fix a bug (class),
a bug that I've mentioned several times now that I believe we've
already fixed, you first MUST convince me that the bug (class) still
exists.  You have not done that.

Paul, I am talking about eliminating a class of bugs, but you don't
seem to get the point and you are fixated on the very instance of this
bug class.

I do understand that you are trying to eliminate a class of bugs, the
point I'm trying to make is that I believe we have addressed that
already with the patches I've previously cited.

quoted

quoted

2. Performance, no extra function call.

Convince me the bug still exists first and then we can discuss the
merits of whatever solutions are proposed.

This is independent of the bug!

Correctness first, maintainability second, performance third.  That's
my current priority and I feel the maintainability hit doesn't justify
the performance win at this point in time.  Besides, we're already
expecting a big performance boost simply by moving to static_calls.

As I said, If you don't want to modify the core LSM layer, it's okay,
I still want to go with changes local to the BPF LSM, If you really
don't agree with the changes local to the BPF LSM, we can have it go
via the BPF tree and seek Linus' help to resolve the conflict.

As the BPF maintainer you are always free to do whatever you like
within the scope of the LSM you maintain so long as it does not touch
or otherwise impact any of the other LSMs or the LSM framework.  If
you do affect the other LSMs, or the LSM framework, you need to get an
ACK from the associated maintainer.  That's pretty much how Linux
kernel development works.

-- 
paul-moore.com