A struct kobject is only supposed to be embedded into objects which
lifetime it will manage.
Objects of type struct gendisk however are refcounted by their part0
block_device.
Therefore the integrity_kobj should not be embedded but split into its
own independently managed object.
This will also provide a proper .release function for the ktype which
avoid warnings like the following:
kobject: 'integrity' (000000005198bea8): does not have a release() function, it is broken and must be fixed.
While modifying blk_integrity_del() also drop the explicit call to
kobject_uevent(KOBJ_REMOVE) as the driver care will do this
automatically.
Reported-by: Mirsad Todorovac <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Thomas Weißschuh <[email protected]>
---
block/blk-integrity.c | 32 ++++++++++++++++++++++++--------
include/linux/blkdev.h | 2 +-
2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 8f01d786f5cb..40adf33f5535 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -218,10 +218,15 @@ struct integrity_sysfs_entry {
ssize_t (*store)(struct blk_integrity *, const char *, size_t);
};
+static inline struct gendisk *integrity_kobj_to_disk(struct kobject *kobj)
+{
+ return dev_to_disk(kobj_to_dev(kobj->parent));
+}
+
static ssize_t integrity_attr_show(struct kobject *kobj, struct attribute *attr,
char *page)
{
- struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
+ struct gendisk *disk = integrity_kobj_to_disk(kobj);
struct blk_integrity *bi = &disk->queue->integrity;
struct integrity_sysfs_entry *entry =
container_of(attr, struct integrity_sysfs_entry, attr);
@@ -233,7 +238,7 @@ static ssize_t integrity_attr_store(struct kobject *kobj,
struct attribute *attr, const char *page,
size_t count)
{
- struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
+ struct gendisk *disk = integrity_kobj_to_disk(kobj);
struct blk_integrity *bi = &disk->queue->integrity;
struct integrity_sysfs_entry *entry =
container_of(attr, struct integrity_sysfs_entry, attr);
@@ -356,9 +361,15 @@ static const struct sysfs_ops integrity_ops = {
.store = &integrity_attr_store,
};
+static void integrity_release(struct kobject *kobj)
+{
+ kfree(kobj);
+}
+
static const struct kobj_type integrity_ktype = {
.default_groups = integrity_groups,
.sysfs_ops = &integrity_ops,
+ .release = integrity_release,
};
static blk_status_t blk_integrity_nop_fn(struct blk_integrity_iter *iter)
@@ -442,16 +453,21 @@ int blk_integrity_add(struct gendisk *disk)
{
int ret;
- ret = kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
+ disk->integrity_kobj = kmalloc(sizeof(*disk->integrity_kobj), GFP_KERNEL);
+ if (!disk->integrity_kobj)
+ return -ENOMEM;
+
+ ret = kobject_init_and_add(disk->integrity_kobj, &integrity_ktype,
&disk_to_dev(disk)->kobj, "%s", "integrity");
- if (!ret)
- kobject_uevent(&disk->integrity_kobj, KOBJ_ADD);
+ if (ret)
+ kobject_put(disk->integrity_kobj);
+ else
+ kobject_uevent(disk->integrity_kobj, KOBJ_ADD);
+
return ret;
}
void blk_integrity_del(struct gendisk *disk)
{
- kobject_uevent(&disk->integrity_kobj, KOBJ_REMOVE);
- kobject_del(&disk->integrity_kobj);
- kobject_put(&disk->integrity_kobj);
+ kobject_put(disk->integrity_kobj);
}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index d1aee08f8c18..2fbfb3277a2b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -164,7 +164,7 @@ struct gendisk {
atomic_t sync_io; /* RAID */
struct disk_events *ev;
#ifdef CONFIG_BLK_DEV_INTEGRITY
- struct kobject integrity_kobj;
+ struct kobject *integrity_kobj;
#endif /* CONFIG_BLK_DEV_INTEGRITY */
#ifdef CONFIG_BLK_DEV_ZONED
---
base-commit: 44889ba56cbb3d51154660ccd15818bc77276696
change-id: 20230309-kobj_release-gendisk_integrity-e26c0bc126aa
Best regards,
--
Thomas Weißschuh <[email protected]>
On 09. 03. 2023. 21:23, Thomas Weißschuh wrote:
> A struct kobject is only supposed to be embedded into objects which
> lifetime it will manage.
> Objects of type struct gendisk however are refcounted by their part0
> block_device.
> Therefore the integrity_kobj should not be embedded but split into its
> own independently managed object.
>
> This will also provide a proper .release function for the ktype which
> avoid warnings like the following:
> kobject: 'integrity' (000000005198bea8): does not have a release() function, it is broken and must be fixed.
>
> While modifying blk_integrity_del() also drop the explicit call to
> kobject_uevent(KOBJ_REMOVE) as the driver care will do this
> automatically.
>
> Reported-by: Mirsad Todorovac <[email protected]>
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> block/blk-integrity.c | 32 ++++++++++++++++++++++++--------
> include/linux/blkdev.h | 2 +-
> 2 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/block/blk-integrity.c b/block/blk-integrity.c
> index 8f01d786f5cb..40adf33f5535 100644
> --- a/block/blk-integrity.c
> +++ b/block/blk-integrity.c
> @@ -218,10 +218,15 @@ struct integrity_sysfs_entry {
> ssize_t (*store)(struct blk_integrity *, const char *, size_t);
> };
>
> +static inline struct gendisk *integrity_kobj_to_disk(struct kobject *kobj)
> +{
> + return dev_to_disk(kobj_to_dev(kobj->parent));
> +}
> +
> static ssize_t integrity_attr_show(struct kobject *kobj, struct attribute *attr,
> char *page)
> {
> - struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
> + struct gendisk *disk = integrity_kobj_to_disk(kobj);
> struct blk_integrity *bi = &disk->queue->integrity;
> struct integrity_sysfs_entry *entry =
> container_of(attr, struct integrity_sysfs_entry, attr);
> @@ -233,7 +238,7 @@ static ssize_t integrity_attr_store(struct kobject *kobj,
> struct attribute *attr, const char *page,
> size_t count)
> {
> - struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
> + struct gendisk *disk = integrity_kobj_to_disk(kobj);
> struct blk_integrity *bi = &disk->queue->integrity;
> struct integrity_sysfs_entry *entry =
> container_of(attr, struct integrity_sysfs_entry, attr);
> @@ -356,9 +361,15 @@ static const struct sysfs_ops integrity_ops = {
> .store = &integrity_attr_store,
> };
>
> +static void integrity_release(struct kobject *kobj)
> +{
> + kfree(kobj);
> +}
> +
> static const struct kobj_type integrity_ktype = {
> .default_groups = integrity_groups,
> .sysfs_ops = &integrity_ops,
> + .release = integrity_release,
> };
>
> static blk_status_t blk_integrity_nop_fn(struct blk_integrity_iter *iter)
> @@ -442,16 +453,21 @@ int blk_integrity_add(struct gendisk *disk)
> {
> int ret;
>
> - ret = kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
> + disk->integrity_kobj = kmalloc(sizeof(*disk->integrity_kobj), GFP_KERNEL);
> + if (!disk->integrity_kobj)
> + return -ENOMEM;
> +
> + ret = kobject_init_and_add(disk->integrity_kobj, &integrity_ktype,
> &disk_to_dev(disk)->kobj, "%s", "integrity");
> - if (!ret)
> - kobject_uevent(&disk->integrity_kobj, KOBJ_ADD);
> + if (ret)
> + kobject_put(disk->integrity_kobj);
> + else
> + kobject_uevent(disk->integrity_kobj, KOBJ_ADD);
> +
> return ret;
> }
>
> void blk_integrity_del(struct gendisk *disk)
> {
> - kobject_uevent(&disk->integrity_kobj, KOBJ_REMOVE);
> - kobject_del(&disk->integrity_kobj);
> - kobject_put(&disk->integrity_kobj);
> + kobject_put(disk->integrity_kobj);
> }
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index d1aee08f8c18..2fbfb3277a2b 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -164,7 +164,7 @@ struct gendisk {
> atomic_t sync_io; /* RAID */
> struct disk_events *ev;
> #ifdef CONFIG_BLK_DEV_INTEGRITY
> - struct kobject integrity_kobj;
> + struct kobject *integrity_kobj;
> #endif /* CONFIG_BLK_DEV_INTEGRITY */
>
> #ifdef CONFIG_BLK_DEV_ZONED
>
> ---
> base-commit: 44889ba56cbb3d51154660ccd15818bc77276696
> change-id: 20230309-kobj_release-gendisk_integrity-e26c0bc126aa
>
> Best regards,
Hello Thomas,
Thank you for Cc:-ing me on this.
The patch applied successfully against 6.3-rc1 commit 44889ba56cbb Merge tag 'net-6.3-rc2'
and I'm just in a build with
CONFIG_DEBUG_KOBJECT=y
CONFIG_DEBUG_KOBJECT_RELEASE=y
However, I can see remotely whether the kernel is operating, but not these special messages
that are emitted past rsyslog's end of lifetime. It looks like it will require physical
access to the box tomorrow morning, Lord willing.
I hate to object to your solution, still, I fail to see how it releases
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 };
It is declared here:
26 static struct kmem_cache *iint_cache __read_mostly;
so it ought to be kmem_cache_destroy()-ed from this module, unless there is something that
is not obvious nor transparent.
Can you help me see what I'm doing wrong?
(Many thanks to Mr. Andy Schevchenko from Intel who narrowed the search for the bug to
security/integrity/iint.c.)
Best 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
The European Union
Hi Thomas!
> A struct kobject is only supposed to be embedded into objects which
> lifetime it will manage. Objects of type struct gendisk however are
> refcounted by their part0 block_device. Therefore the integrity_kobj
> should not be embedded but split into its own independently managed
> object.
That's how we originally did it. However, this caused problems for a
couple of subsystems, NVMe and DM, if I remember correctly.
Managing the lifetime of request_queue vs. gendisk vs. blk_integrity
proved to be tricky. Basically at the time things were allocated we
didn't yet have all the information required to complete the block
device setup. We had to be able to send commands to the drive to finish
probing for all the relevant parameters. That dependency was the
rationale behind inlining the blk_integrity into gendisk so it was
always available. Hazy on the details, this was a long time ago.
Another option would be to reshuffle the sysfs bits. The kobj really
isn't used for anything else.
--
Martin K. Petersen Oracle Linux Engineering
+Cc Andy
Answer below.
On Thu, Mar 09, 2023 at 10:05:32PM +0100, Mirsad Goran Todorovac wrote:
> On 09. 03. 2023. 21:23, Thomas Weißschuh wrote:
> > A struct kobject is only supposed to be embedded into objects which
> > lifetime it will manage.
> > Objects of type struct gendisk however are refcounted by their part0
> > block_device.
> > Therefore the integrity_kobj should not be embedded but split into its
> > own independently managed object.
> >
> > This will also provide a proper .release function for the ktype which
> > avoid warnings like the following:
> > kobject: 'integrity' (000000005198bea8): does not have a release() function, it is broken and must be fixed.
> >
> > While modifying blk_integrity_del() also drop the explicit call to
> > kobject_uevent(KOBJ_REMOVE) as the driver care will do this
> > automatically.
> >
> > Reported-by: Mirsad Todorovac <[email protected]>
> > Link: https://lore.kernel.org/lkml/[email protected]/
> > Signed-off-by: Thomas Weißschuh <[email protected]>
> > ---
> > block/blk-integrity.c | 32 ++++++++++++++++++++++++--------
> > include/linux/blkdev.h | 2 +-
> > 2 files changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/block/blk-integrity.c b/block/blk-integrity.c
> > index 8f01d786f5cb..40adf33f5535 100644
> > --- a/block/blk-integrity.c
> > +++ b/block/blk-integrity.c
> > @@ -218,10 +218,15 @@ struct integrity_sysfs_entry {
> > ssize_t (*store)(struct blk_integrity *, const char *, size_t);
> > };
> >
> > +static inline struct gendisk *integrity_kobj_to_disk(struct kobject *kobj)
> > +{
> > + return dev_to_disk(kobj_to_dev(kobj->parent));
> > +}
> > +
> > static ssize_t integrity_attr_show(struct kobject *kobj, struct attribute *attr,
> > char *page)
> > {
> > - struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
> > + struct gendisk *disk = integrity_kobj_to_disk(kobj);
> > struct blk_integrity *bi = &disk->queue->integrity;
> > struct integrity_sysfs_entry *entry =
> > container_of(attr, struct integrity_sysfs_entry, attr);
> > @@ -233,7 +238,7 @@ static ssize_t integrity_attr_store(struct kobject *kobj,
> > struct attribute *attr, const char *page,
> > size_t count)
> > {
> > - struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
> > + struct gendisk *disk = integrity_kobj_to_disk(kobj);
> > struct blk_integrity *bi = &disk->queue->integrity;
> > struct integrity_sysfs_entry *entry =
> > container_of(attr, struct integrity_sysfs_entry, attr);
> > @@ -356,9 +361,15 @@ static const struct sysfs_ops integrity_ops = {
> > .store = &integrity_attr_store,
> > };
> >
> > +static void integrity_release(struct kobject *kobj)
> > +{
> > + kfree(kobj);
> > +}
> > +
> > static const struct kobj_type integrity_ktype = {
> > .default_groups = integrity_groups,
> > .sysfs_ops = &integrity_ops,
> > + .release = integrity_release,
> > };
> >
> > static blk_status_t blk_integrity_nop_fn(struct blk_integrity_iter *iter)
> > @@ -442,16 +453,21 @@ int blk_integrity_add(struct gendisk *disk)
> > {
> > int ret;
> >
> > - ret = kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
> > + disk->integrity_kobj = kmalloc(sizeof(*disk->integrity_kobj), GFP_KERNEL);
> > + if (!disk->integrity_kobj)
> > + return -ENOMEM;
> > +
> > + ret = kobject_init_and_add(disk->integrity_kobj, &integrity_ktype,
> > &disk_to_dev(disk)->kobj, "%s", "integrity");
> > - if (!ret)
> > - kobject_uevent(&disk->integrity_kobj, KOBJ_ADD);
> > + if (ret)
> > + kobject_put(disk->integrity_kobj);
> > + else
> > + kobject_uevent(disk->integrity_kobj, KOBJ_ADD);
> > +
> > return ret;
> > }
> >
> > void blk_integrity_del(struct gendisk *disk)
> > {
> > - kobject_uevent(&disk->integrity_kobj, KOBJ_REMOVE);
> > - kobject_del(&disk->integrity_kobj);
> > - kobject_put(&disk->integrity_kobj);
> > + kobject_put(disk->integrity_kobj);
> > }
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index d1aee08f8c18..2fbfb3277a2b 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -164,7 +164,7 @@ struct gendisk {
> > atomic_t sync_io; /* RAID */
> > struct disk_events *ev;
> > #ifdef CONFIG_BLK_DEV_INTEGRITY
> > - struct kobject integrity_kobj;
> > + struct kobject *integrity_kobj;
> > #endif /* CONFIG_BLK_DEV_INTEGRITY */
> >
> > #ifdef CONFIG_BLK_DEV_ZONED
> >
> > ---
> > base-commit: 44889ba56cbb3d51154660ccd15818bc77276696
> > change-id: 20230309-kobj_release-gendisk_integrity-e26c0bc126aa
> >
> > Best regards,
>
> Hello Thomas,
>
> Thank you for Cc:-ing me on this.
>
> The patch applied successfully against 6.3-rc1 commit 44889ba56cbb Merge tag 'net-6.3-rc2'
> and I'm just in a build with
>
> CONFIG_DEBUG_KOBJECT=y
> CONFIG_DEBUG_KOBJECT_RELEASE=y
>
> However, I can see remotely whether the kernel is operating, but not these special messages
> that are emitted past rsyslog's end of lifetime. It looks like it will require physical
> access to the box tomorrow morning, Lord willing.
>
> I hate to object to your solution, still, I fail to see how it releases
>
> 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 };
>
> It is declared here:
>
> 26 static struct kmem_cache *iint_cache __read_mostly;
>
> so it ought to be kmem_cache_destroy()-ed from this module, unless there is something that
> is not obvious nor transparent.
>
> Can you help me see what I'm doing wrong?
I think this has nothing to do with security/integrity/iint.c.
Instead the problem are these snippets from block/blk-integrity.c:
/* line 359: kobj_type without .release */
static const struct kobj_type integrity_ktype = {
.default_groups = integrity_groups,
.sysfs_ops = &integrity_ops,
};
/* line 445: registration of kobject "integrity" with that type */
ret = kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
&disk_to_dev(disk)->kobj, "%s", "integrity");
These kobjects represent the directories /sys/block/*/integrity/.
The patch below can be used to easily diagnose this during kobject
initialization instead of cleanup, which seems much more useful.
It probably makes sense to move the
pr_debug("does not have a release() function");
to kobject_init() in general.
diff --git a/lib/kobject.c b/lib/kobject.c
index 6e2f0bee3560..2708f8344e9b 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -341,6 +341,8 @@ void kobject_init(struct kobject *kobj, const struct kobj_type *ktype)
dump_stack();
}
+ WARN(!ktype->release, "KOBJECT %p, %p: no release function\n", kobj, ktype);
+
kobject_init_internal(kobj);
kobj->ktype = ktype;
return;
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?
BTW, moving check for !ktype->release to kobject_init() is great for it
might make such problems noticed in dmesg, rather than taking screenshots.
Regards,
--
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
The European Union
On Thu, Mar 09, 2023 at 09:23:24PM +0000, Thomas Wei?schuh wrote:
> +Cc Andy
>
> Answer below.
Thank you for sharing!
--
With Best Regards,
Andy Shevchenko
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.
At least it's not a problem with kobjects as those are not used here.
> BTW, moving check for !ktype->release to kobject_init() is great for it
> might make such problems noticed in dmesg, rather than taking screenshots.
>
> Regards,
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
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
Greeting,
FYI, we noticed WARNING:at_lib/kobject.c:#kobject_get due to commit (built with gcc-11):
commit: a3e3d566c4472dd5079a8f99e6b8ae259bcfe429 ("[PATCH] block: don't embed integrity_kobj into gendisk")
url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Wei-schuh/block-don-t-embed-integrity_kobj-into-gendisk/20230310-042440
patch link: https://lore.kernel.org/all/20230309-kobj_release-gendisk_integrity-v1-1-a240f54eac60@weissschuh.net/
patch subject: [PATCH] block: don't embed integrity_kobj into gendisk
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-lkp/[email protected]
[ 20.061287][ T135] ------------[ cut here ]------------
[ 20.062355][ T135] kobject: '(null)' (00000000a80e27c2): is not initialized, yet kobject_get() is being called.
[ 20.063941][ T135] WARNING: CPU: 0 PID: 135 at lib/kobject.c:632 kobject_get (kbuild/src/x86_64/lib/kobject.c:632)
[ 20.065168][ T135] Modules linked in: sr_mod(+) cdrom intel_rapl_msr bochs(+) intel_rapl_common sg crct10dif_pclmul drm_vram_helper crc32_pclmul ata_generic drm_ttm_helper crc32c_intel ppdev ghash_clmulni_intel ttm drm_kms_helper ata_piix sha512_ssse3 joydev rapl libata parport_pc i2c_piix4 syscopyarea serio_raw sysfillrect sysimgblt ipmi_devintf ipmi_msghandler parport drm fuse ip_tables
[ 20.070566][ T135] CPU: 0 PID: 135 Comm: systemd-udevd Not tainted 6.3.0-rc1-00107-ga3e3d566c447 #1
[ 20.071982][ T135] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-5 04/01/2014
[ 20.073618][ T135] RIP: 0010:kobject_get (kbuild/src/x86_64/lib/kobject.c:632)
[ 20.074487][ T135] Code: 44 24 38 85 c0 74 3b 8d 50 01 09 c2 78 20 4c 89 e0 41 5c c3 cc cc cc cc 48 8b 37 48 89 fa 48 c7 c7 68 86 74 82 e8 74 1c 23 ff <0f> 0b eb c4 be 01 00 00 00 e8 a6 30 83 ff 4c 89 e0 41 5c c3 cc cc
All code
========
0: 44 24 38 rex.R and $0x38,%al
3: 85 c0 test %eax,%eax
5: 74 3b je 0x42
7: 8d 50 01 lea 0x1(%rax),%edx
a: 09 c2 or %eax,%edx
c: 78 20 js 0x2e
e: 4c 89 e0 mov %r12,%rax
11: 41 5c pop %r12
13: c3 retq
14: cc int3
15: cc int3
16: cc int3
17: cc int3
18: 48 8b 37 mov (%rdi),%rsi
1b: 48 89 fa mov %rdi,%rdx
1e: 48 c7 c7 68 86 74 82 mov $0xffffffff82748668,%rdi
25: e8 74 1c 23 ff callq 0xffffffffff231c9e
2a:* 0f 0b ud2 <-- trapping instruction
2c: eb c4 jmp 0xfffffffffffffff2
2e: be 01 00 00 00 mov $0x1,%esi
33: e8 a6 30 83 ff callq 0xffffffffff8330de
38: 4c 89 e0 mov %r12,%rax
3b: 41 5c pop %r12
3d: c3 retq
3e: cc int3
3f: cc int3
Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: eb c4 jmp 0xffffffffffffffc8
4: be 01 00 00 00 mov $0x1,%esi
9: e8 a6 30 83 ff callq 0xffffffffff8330b4
e: 4c 89 e0 mov %r12,%rax
11: 41 5c pop %r12
13: c3 retq
14: cc int3
15: cc int3
[ 20.077668][ T135] RSP: 0018:ffffc900005b3b60 EFLAGS: 00010282
[ 20.078759][ T135] RAX: 0000000000000000 RBX: ffff888100419308 RCX: 0000000000000000
[ 20.080086][ T135] RDX: ffff88842fc28800 RSI: ffff88842fc1c700 RDI: ffff88842fc1c700
[ 20.081443][ T135] RBP: ffff888129a7cc40 R08: 0000000000000000 R09: 00000000ffff7fff
[ 20.082794][ T135] R10: ffffc900005b3a00 R11: ffffffff82bd8d88 R12: ffff888129a7c358
[ 20.083956][ T135] R13: ffff888129a7cc40 R14: ffff888129a7cc48 R15: ffff888100419308
[ 20.085254][ T135] FS: 00007fad8b3ea8c0(0000) GS:ffff88842fc00000(0000) knlGS:0000000000000000
[ 20.086675][ T135] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 20.087790][ T135] CR2: 00007f6231713000 CR3: 000000010095a000 CR4: 00000000000006f0
[ 20.090711][ T135] Call Trace:
[ 20.092700][ T135] <TASK>
[ 20.094808][ T135] kobject_add_internal (kbuild/src/x86_64/include/linux/spinlock.h:350 kbuild/src/x86_64/lib/kobject.c:171 kbuild/src/x86_64/lib/kobject.c:222)
[ 20.097699][ T135] kobject_init_and_add (kbuild/src/x86_64/lib/kobject.c:366 kbuild/src/x86_64/lib/kobject.c:449)
[ 20.100311][ T135] blk_integrity_add (kbuild/src/x86_64/block/blk-integrity.c:463)
[ 20.102625][ T135] device_add_disk (kbuild/src/x86_64/block/genhd.c:483)
[ 20.104734][ T135] sr_probe (kbuild/src/x86_64/drivers/scsi/sr.c:695) sr_mod
[ 20.107144][ T135] really_probe (kbuild/src/x86_64/drivers/base/dd.c:552 kbuild/src/x86_64/drivers/base/dd.c:631)
[ 20.109364][ T135] __driver_probe_device (kbuild/src/x86_64/drivers/base/dd.c:709 kbuild/src/x86_64/drivers/base/dd.c:766)
[ 20.111710][ T135] driver_probe_device (kbuild/src/x86_64/drivers/base/dd.c:798)
[ 20.113734][ T135] __driver_attach (kbuild/src/x86_64/drivers/base/dd.c:1185)
[ 20.115965][ T135] ? __pfx___driver_attach (kbuild/src/x86_64/drivers/base/dd.c:1125)
[ 20.118091][ T135] bus_for_each_dev (kbuild/src/x86_64/drivers/base/bus.c:368)
[ 20.120112][ T135] bus_add_driver (kbuild/src/x86_64/drivers/base/bus.c:673)
[ 20.122149][ T135] driver_register (kbuild/src/x86_64/drivers/base/driver.c:246)
[ 20.124546][ T135] ? __pfx_init_module (kbuild/src/x86_64/drivers/scsi/sr.c:147) sr_mod
[ 20.126992][ T135] init_sr (kbuild/src/x86_64/drivers/scsi/sr.c:157) sr_mod
[ 20.129130][ T135] ? __pfx_init_module (kbuild/src/x86_64/drivers/scsi/sr.c:147) sr_mod
[ 20.131362][ T135] do_one_initcall (kbuild/src/x86_64/init/main.c:1306)
[ 20.133391][ T135] ? kmalloc_trace (kbuild/src/x86_64/mm/slab_common.c:1064)
[ 20.135626][ T135] do_init_module (kbuild/src/x86_64/kernel/module/main.c:2464)
[ 20.137569][ T135] __do_sys_finit_module (kbuild/src/x86_64/kernel/module/main.c:2973)
[ 20.139730][ T135] do_syscall_64 (kbuild/src/x86_64/arch/x86/entry/common.c:50 kbuild/src/x86_64/arch/x86/entry/common.c:80)
[ 20.141556][ T135] entry_SYSCALL_64_after_hwframe (kbuild/src/x86_64/arch/x86/entry/entry_64.S:120)
[ 20.144045][ T135] RIP: 0033:0x7fad8b8a39b9
[ 20.145886][ T135] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 54 0c 00 f7 d8 64 89 01 48
All code
========
0: 00 c3 add %al,%bl
2: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
9: 00 00 00
c: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
11: 48 89 f8 mov %rdi,%rax
14: 48 89 f7 mov %rsi,%rdi
17: 48 89 d6 mov %rdx,%rsi
1a: 48 89 ca mov %rcx,%rdx
1d: 4d 89 c2 mov %r8,%r10
20: 4d 89 c8 mov %r9,%r8
23: 4c 8b 4c 24 08 mov 0x8(%rsp),%r9
28: 0f 05 syscall
2a:* 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax <-- trapping instruction
30: 73 01 jae 0x33
32: c3 retq
33: 48 8b 0d a7 54 0c 00 mov 0xc54a7(%rip),%rcx # 0xc54e1
3a: f7 d8 neg %eax
3c: 64 89 01 mov %eax,%fs:(%rcx)
3f: 48 rex.W
Code starting with the faulting instruction
===========================================
0: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
6: 73 01 jae 0x9
8: c3 retq
9: 48 8b 0d a7 54 0c 00 mov 0xc54a7(%rip),%rcx # 0xc54b7
10: f7 d8 neg %eax
12: 64 89 01 mov %eax,%fs:(%rcx)
15: 48 rex.W
To reproduce:
# build kernel
cd linux
cp config-6.3.0-rc1-00107-ga3e3d566c447 .config
make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
