Message ID | 20230309-kobj_release-gendisk_integrity-v1-1-a240f54eac60@weissschuh.net |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp506064wrd; Thu, 9 Mar 2023 12:38:05 -0800 (PST) X-Google-Smtp-Source: AK7set8j6TNAI9entwZVw1IFofcMmdtgZUjyU01pIx0ESlc0Dz4CKiVWSXx1/nwOI0GTJNLjigSc X-Received: by 2002:a05:6a20:8e28:b0:cd:7fcf:11a6 with SMTP id y40-20020a056a208e2800b000cd7fcf11a6mr23065818pzj.48.1678394284975; Thu, 09 Mar 2023 12:38:04 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1678394284; cv=none; d=google.com; s=arc-20160816; b=Rty2jQFM+Ma2MTe9t/vFJaLxm9pRsp88VEK7HX/oDVB5osopsLfQ6cfEQdq7v0Wu2I 3CT5uiWa/8v3GBuw7n0X5HYnTnotjAk9oxuywW484exLVHiUY3FwLiCysvknNl+MzauJ HhQ01JvUDY9QI9w2id6x6MsZgIARMyVVLclzo0up/rFGvGJT9zXR6hZYBdRi09bCp09p p7ZNkwrJDjOzW1QYNHHW3NrvIBA5koQARIRmmBX9VmUFS0SySMLQXxGg3e9tVN+6H9Nj wChABwfsDqah0KSir4LsB58D6OTjqtp02BTw4B0K8OjG/+pbIwUT0JXR+HYi35bK2Bv/ RCDw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:message-id:content-transfer-encoding :mime-version:subject:date:dkim-signature:from; bh=6moGp1X9uPUKHlaxdtE7G5JegeOwRO2Iku+NuELnE/0=; b=CD4l2x2eiE/NqblKJ/sTg1oeooJmXwnwRjYacg0ePi78LNFE2eKc7VmD/wNBoKq8RY 8L+hvncy3qJAYKZAca/P6/vFk34ogi1HR6Ea60r4GMtE3OldbJrs7TFobO+NUKBZ2ebl CrGBJQHfjOPd1L24DHB6faLDUkTWTENMY2Ak0Wuq8c0Fq5Y2wGw5gebNzzwpD7rQEJjb NBiXPP4IBbI+6zOGiIAlv7eZ2LinJuHVQV/T045j9417z9YlVlYArWOgeC/34gUOA9b4 HDomHJg3qQa+JOndnRsrCYhgbQR7wUH2yDm/UPvOsF7NBA4HJJEJPJrKOkGOBLscmmDV 3zfg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@weissschuh.net header.s=mail header.b=hlHN4mXU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id w24-20020aa79558000000b005afdbef35d3si87460pfq.56.2023.03.09.12.37.49; Thu, 09 Mar 2023 12:38:04 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@weissschuh.net header.s=mail header.b=hlHN4mXU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229754AbjCIUX7 (ORCPT <rfc822;carlos.wei.hk@gmail.com> + 99 others); Thu, 9 Mar 2023 15:23:59 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50556 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229521AbjCIUX5 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 9 Mar 2023 15:23:57 -0500 Received: from todd.t-8ch.de (todd.t-8ch.de [IPv6:2a01:4f8:c010:41de::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6186C61A8E; Thu, 9 Mar 2023 12:23:55 -0800 (PST) From: =?utf-8?q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=weissschuh.net; s=mail; t=1678393432; bh=V4DiMXUAlaGpfb/ouDcwZ5s91T1/n4kCKJbIR1SgzHI=; h=From:Date:Subject:To:Cc:From; b=hlHN4mXUUUBTbBJFqaJF7k+nJCiPDj/Z3QEkvw6FEn9xdqnWK+A2HNT3usVShSIDT 71P3alBdfZEaxv8ACbID5ASh3W8+GfRbve3rltWGMovSp14my2J8kwyY7ZeugODuYi spAOLYdNQCu1RGW+OIQi18JV2fNKLr/HO+iG4vmY= Date: Thu, 09 Mar 2023 20:23:41 +0000 Subject: [PATCH] block: don't embed integrity_kobj into gendisk MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Message-Id: <20230309-kobj_release-gendisk_integrity-v1-1-a240f54eac60@weissschuh.net> X-B4-Tracking: v=1; b=H4sIAExACmQC/x2NUQrCMBAFr1L220CaQkGvIlKS9JmuDVvZtKKU3 t3o5wyPeTsVKKPQpdlJ8eLCi1RoTw3FyUuC4bEyOes629mzmZfwGBQZvsAkyMhlHlhWJOX1Y+D 6aENsXe891Uj4zYJ6iVPNyJZzlU/Fnd//1+vtOL7NDnmNhQAAAA== To: Jens Axboe <axboe@kernel.dk> Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>, =?utf-8?q?Thomas_Wei?= =?utf-8?q?=C3=9Fschuh?= <linux@weissschuh.net> X-Mailer: b4 0.12.1 X-Developer-Signature: v=1; a=ed25519-sha256; t=1678393429; l=4201; i=linux@weissschuh.net; s=20221212; h=from:subject:message-id; bh=V4DiMXUAlaGpfb/ouDcwZ5s91T1/n4kCKJbIR1SgzHI=; b=W6X3J/kXyiZzwSPLstGMD/Bk9b0badxTht4HARLoN7LcMySEjbAfIwmwC95nozbZYhwcI6Tto 9WqQsHMJNL4AtTbo+NoleObSHXdhGjBpBg73pBPmNyNn+BVAnfgUNds X-Developer-Key: i=linux@weissschuh.net; a=ed25519; pk=KcycQgFPX2wGR5azS7RhpBqedglOZVgRPfdFSPB1LNw= X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1759923965372049771?= X-GMAIL-MSGID: =?utf-8?q?1759923965372049771?= |
Series |
block: don't embed integrity_kobj into gendisk
|
|
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
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
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.
+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;
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,
On Thu, Mar 09, 2023 at 09:23:24PM +0000, Thomas Weißschuh wrote: > +Cc Andy > > Answer below. Thank you for sharing!
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
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.
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 <oliver.sang@intel.com> | Link: https://lore.kernel.org/oe-lkp/202303141612.8a6b6b04-oliver.sang@intel.com [ 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.
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