From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
Cc: "Thomas Weißschuh" <linux@weissschuh.net>,
"Jens Axboe" <axboe@kernel.dk>,
linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
Subject: Re: [PATCH] block: don't embed integrity_kobj into gendisk
Date: Fri, 10 Mar 2023 15:42:42 +0200 [thread overview]
Message-ID: <ZAsz0lvdS60nFwJx@smile.fi.intel.com> (raw)
In-Reply-To: <dc4f7684-2bec-ece2-17bd-ecaaaa5d5c69@alu.unizg.hr>
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
next prev parent reply other threads:[~2023-03-10 13:42 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-09 20:23 [PATCH] block: don't embed integrity_kobj into gendisk Thomas Weißschuh
2023-03-09 21:05 ` Mirsad Goran Todorovac
2023-03-09 21:23 ` Thomas Weißschuh
2023-03-09 21:46 ` Mirsad Goran Todorovac
2023-03-09 23:26 ` Thomas Weißschuh
2023-03-10 8:52 ` Mirsad Todorovac
2023-03-10 13:42 ` Andy Shevchenko [this message]
2023-03-09 21:47 ` Andy Shevchenko
2023-03-09 21:20 ` Martin K. Petersen
2023-03-14 9:15 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZAsz0lvdS60nFwJx@smile.fi.intel.com \
--to=andriy.shevchenko@linux.intel.com \
--cc=axboe@kernel.dk \
--cc=gregkh@linuxfoundation.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@weissschuh.net \
--cc=mirsad.todorovac@alu.unizg.hr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.