Message-ID: <dc4f7684-2bec-ece2-17bd-ecaaaa5d5c69@alu.unizg.hr>
Date:   Fri, 10 Mar 2023 09:52:51 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH] block: don't embed integrity_kobj into gendisk
To:     =?UTF-8?Q?Thomas_Wei=c3=9fschuh?= <linux@weissschuh.net>
Cc:     Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
References: <20230309-kobj_release-gendisk_integrity-v1-1-a240f54eac60@weissschuh.net>
 <d85ba503-d93c-3483-25e1-6043d4af444f@alu.unizg.hr>
 <0ac9777c-7586-494f-bbc5-87f14f645b12@t-8ch.de>
 <d10f18b4-56cf-8a55-b12f-79b1163d8841@alu.unizg.hr>
 <7f977a5d-8302-4a32-9dce-f6d7637b2555@t-8ch.de>
Content-Language: en-US, hr
From:   Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
In-Reply-To: <7f977a5d-8302-4a32-9dce-f6d7637b2555@t-8ch.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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? :-/

Regards,
Mirsad

-- 
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu

System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia