diff mbox series

block: don't embed integrity_kobj into gendisk

Message ID 20230309-kobj_release-gendisk_integrity-v1-1-a240f54eac60@weissschuh.net (mailing list archive)
State New, archived
Headers show
Series block: don't embed integrity_kobj into gendisk | expand

Commit Message

Thomas Weißschuh March 9, 2023, 8:23 p.m. UTC 
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 <mirsad.todorovac@alu.unizg.hr>
Link: https://lore.kernel.org/lkml/60b2b66c-22c9-1d38-ed1c-7b7d95e32720@alu.unizg.hr/
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 block/blk-integrity.c  | 32 ++++++++++++++++++++++++--------
 include/linux/blkdev.h |  2 +-
 2 files changed, 25 insertions(+), 9 deletions(-)


---
base-commit: 44889ba56cbb3d51154660ccd15818bc77276696
change-id: 20230309-kobj_release-gendisk_integrity-e26c0bc126aa

Best regards,

Comments

Mirsad Todorovac March 9, 2023, 9:05 p.m. UTC | #1 
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 <mirsad.todorovac@alu.unizg.hr>
> Link: https://lore.kernel.org/lkml/60b2b66c-22c9-1d38-ed1c-7b7d95e32720@alu.unizg.hr/
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  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
Martin K. Petersen March 9, 2023, 9:20 p.m. UTC | #2 
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.
Thomas Weißschuh March 9, 2023, 9:23 p.m. UTC | #3 
+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 <mirsad.todorovac@alu.unizg.hr>
> > Link: https://lore.kernel.org/lkml/60b2b66c-22c9-1d38-ed1c-7b7d95e32720@alu.unizg.hr/
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  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;
Mirsad Todorovac March 9, 2023, 9:46 p.m. UTC | #4 
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,
Andy Shevchenko March 9, 2023, 9:47 p.m. UTC | #5 
On Thu, Mar 09, 2023 at 09:23:24PM +0000, Thomas Weißschuh wrote:
> +Cc Andy
> 
> Answer below.

Thank you for sharing!
Thomas Weißschuh March 9, 2023, 11:26 p.m. UTC | #6 
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,
Mirsad Todorovac March 10, 2023, 8:52 a.m. UTC | #7 
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
Andy Shevchenko March 10, 2023, 1:42 p.m. UTC | #8 
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.
diff mbox series

Patch

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