On Fri, Jul 05, 2024 at 09:34:20PM +0200, KP Singh wrote: > On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@xxxxxxxxxx> wrote: > > > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > > On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@xxxxxxxxxx> wrote: > > > > > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > > > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@xxxxxxxxxx> wrote: > > > > > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > > > > > > On Jun 29, 2024 KP Singh <kpsingh@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > 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: > > > > ... > > > > > > > > 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. > > > > > 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. > > > > > ... 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@xxxxxxxxxxxxxx/ > > > > 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. > > > > > 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. Let's take this one step at a time. I think patches 1-4 are fine and stand alone and solve a specific problem without creating any new immediate problems. After 1-4 is accepted, we can come back around to what patch 5 is trying to do, and work on whatever issues may remain at that time. -- Kees Cook
