+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@xxxxxxxxxxxx> > > Link: https://lore.kernel.org/lkml/60b2b66c-22c9-1d38-ed1c-7b7d95e32720@xxxxxxxxxxxx/ > > Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx> > > --- > > 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;
