On Mon, Mar 13, 2023 at 04:27:42PM -0400, Paul Moore wrote: > On Mon, Mar 13, 2023 at 5:33 AM Andy Shevchenko > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > On Sat, Mar 11, 2023 at 09:59:17AM -0500, Paul Moore wrote: > > > On Fri, Mar 10, 2023 at 5:53 PM Mirsad Goran Todorovac > > > <mirsad.todorovac@xxxxxxxxxxxx> wrote: ... > > > With that out of the way, I wanted to make a quick comment on the > > > patch itself. Currently LSMs do not support unloading, or dynamic > > > loading for that matter. There are several reasons for this, but > > > perhaps the most important is that in order to help meet the security > > > goals for several of the LSMs they need to be present in the kernel > > > from the very beginning and remain until the very end. Adding a > > > proper "release" method to a LSM is going to be far more complicated > > > than what you've done with this patchset, involving a lot of > > > discussion both for the LSM layer itself and all of the currently > > > supported LSMs, and ultimately I don't believe it is something we will > > > want to support. > > > > > > I appreciate your desire to help, and I want to thank you for your > > > patch and the effort behind it, but I don't believe the kobject memory > > > leak you saw at kernel shutdown was a real issue (it was only "leaked" > > > because the system was shutting down) and I'm not sure the current > > > behavior is something we want to change in the near future. > > > > Currently the security module so secure that even adds an unneeded noise to > > the debugging output. > > > > At very least it would be nice to add a stub and put a big comment > > (on your taste) explaining why we do not do anything there. > > > > Agree? > > No. Are you sure? I'm proposing to add a stub which is no-op, but with a comment inside explaining why. In such case we: 1) shut the kobject infra up; 2) keep the status quo in LSM; 3) put in the code a good explanation for others on what's going on. > At least not without a lot of additional work beyond what was > presented in this patchset. What about all of the other kobject > caches created by other LSMs, this is more than just the IMA > iint_cache. I'm also skeptical that this patchset was ever tested and > verified as the newly added release() method was never actually called > from anywhere that I could see. I'm not talking about this patchset, but you are right that it wasn't tested. > I think we would need to see a proper, verified fix before I could say > for certain. And continuing to spread the noise in the logs just because LSM is stubborn? > If you want to discuss potential designs, we can do that > too, but please remember the constraints that were already mentioned > about intentionally not allowing the LSMs to be unloaded (prior to > system shutdown). > > I don't know the answer to this, but I'm guessing the LSMs aren't the > only kernel subsystems to "leak" memory on system shutdown; working on > the assumption that this is the case, how are those "leaked" > allocations handled? Note, I'm full for the proper fix, but the current issue is logs flooding done by LSM that needs to be addressed. -- With Best Regards, Andy Shevchenko
