| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 9 Mar 2023 21:23:24 +0000
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
Cc: Jens Axboe <axboe@...nel.dk>, linux-block@...r.kernel.org,
linux-kernel@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH] block: don't embed integrity_kobj into gendisk
+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@....unizg.hr>
> > Link: https://lore.kernel.org/lkml/60b2b66c-22c9-1d38-ed1c-7b7d95e32720@alu.unizg.hr/
> > Signed-off-by: Thomas Weißschuh <linux@...ssschuh.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;
Powered by blists - more mailing lists