On Fri, Mar 10, 2023 at 09:52:51AM +0100, Mirsad Todorovac wrote: > Hi, Thomas, > > The good news is that the > > "kobject: 'integrity' (000000001aa7f87a): does not have a release() function" > > is now removed: > > https://domac.alu.hr/~mtodorov/linux/bugreports/integrity/fix/20230310_082429.jpg > > On 3/10/23 00:26, Thomas Weißschuh wrote: > > On Thu, Mar 09, 2023 at 10:46:50PM +0100, Mirsad Goran Todorovac wrote: > > > On 09. 03. 2023. 22:23, Thomas Weißschuh wrote: > > > > > > Very well, but who then destroys the cache crated here: > > > > > > security/integrity/iint.c:177-179 > > > > 177 iint_cache = > > > > 178 kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache), > > > > 179 0, SLAB_PANIC, init_once); > > > > > > I assumed that it must have been done from iint.c because iint_cache is > > > static? > > > > It doesn't seem like anything destroys this cache. > > > > I'm not sure this is a problem though as iint.c can not be built as module. > > Maybe I was just scolded when I relied on exit() to do the memory deallocation > from heap automatically in userspace programs. :-/ > > I suppose this cache is destroyed when virtual Linux machine is shutdown. > > Still, it might seem prudent for all of the objects to be destroyed before shutting > down the kernel, assuring the call of proper destructors, and in the right order. > > > At least it's not a problem with kobjects as those are not used here. > > I begin to understand. > > security/integrity/iint.c: > 175 static int __init integrity_iintcache_init(void) > 176 { > 177 iint_cache = > 178 kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache), > 179 0, SLAB_PANIC, init_once); > 180 return 0; > 181 } > 182 DEFINE_LSM(integrity) = { > 183 .name = "integrity", > 184 .init = integrity_iintcache_init, > 185 }; > > and struct lsm_info > > include/linux/lsm_hooks.h: > 1721 struct lsm_info { > 1722 const char *name; /* Required. */ > 1723 enum lsm_order order; /* Optional: default is LSM_ORDER_MUTABLE */ > 1724 unsigned long flags; /* Optional: flags describing LSM */ > 1725 int *enabled; /* Optional: controlled by CONFIG_LSM */ > 1726 int (*init)(void); /* Required. */ > 1727 struct lsm_blob_sizes *blobs; /* Optional: for blob sharing. */ > 1728 }; > > Here a proper destructor might be prudent to add. > > Naive try could be like this: > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 6e156d2acffc..d5a6ab9b5eb2 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -1724,6 +1724,7 @@ struct lsm_info { > unsigned long flags; /* Optional: flags describing LSM */ > int *enabled; /* Optional: controlled by CONFIG_LSM */ > int (*init)(void); /* Required. */ > + int (*release)(void); /* Release associated resources */ > struct lsm_blob_sizes *blobs; /* Optional: for blob sharing. */ > }; > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c > index 8638976f7990..3f69eb702b2e 100644 > --- a/security/integrity/iint.c > +++ b/security/integrity/iint.c > @@ -179,9 +179,16 @@ static int __init integrity_iintcache_init(void) > 0, SLAB_PANIC, init_once); > return 0; > } > + > +static int __exit integrity_iintcache_release(void) > +{ > + kmem_cache_destroy(iint_cache); > +} > + > DEFINE_LSM(integrity) = { > .name = "integrity", > .init = integrity_iintcache_init, > + .release = integrity_iintcache_release, > }; > > However, I lack insight of who should invoke .release() on 'integrity', > in 10.7 million lines of *.c and *.h files. Obviously, now no one is > expecting .release in LSM modules. But there might be a need for the > proper cleanup. > > For it is not a kobject as you already observed? :-/ I think you may prepare a formal patch and Cc to Greg KH, who knows the kobject code well and this warning in particular. I believe, even if a dead code, the destructor to have is a good code behaviour, since it is might be called on the __exitcall. -- With Best Regards, Andy Shevchenko
