| 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
| ||
|
Message-ID: <c4e36d110909240326k16bf40cdpfa4b818d1a24a25f@mail.gmail.com>
Date: Thu, 24 Sep 2009 12:26:47 +0200
From: Zdenek Kabelac <zdenek.kabelac@...il.com>
To: Li Zefan <lizf@...fujitsu.com>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
mbroz@...hat.com, tytso@....edu, jens.axboe@...cle.com,
mingo@...e.hu
Subject: Re: [PATCH] Add missing blk_trace_remove_sysfs to be in pair with
blk_trace_init_sysfs
2009/9/24 Li Zefan <lizf@...fujitsu.com>:
>>>> Subject: [PATCH] Add missing blk_trace_remove_sysfs to be in pair with
>>>> blk_trace_init_sysfs
>>>> From: Zdenek Kabelac <zkabelac@...hat.com>
>>>>
>>>> Adds missing blk_trace_remove_sysfs() to be in pair with
>>>> blk_trace_init_sysfs() introduced in commit
>>>> 1d54ad6da9192fed5dd3b60224d9f2dfea0dcd82.
>>>>
>>>> Problem was noticed via kmemleak backtrace when some sysfs entries
>>>> were note properly destroyed during device removal:
>>>>
>>> Thanks for reporting and fixing this!
>>>
>>>> @@ -465,6 +466,7 @@ void blk_unregister_queue(struct gendisk *disk)
>>>>
>>>> kobject_uevent(&q->kobj, KOBJ_REMOVE);
>>>> kobject_del(&q->kobj);
>>>> + blk_trace_remove_sysfs(disk_to_dev(disk));
>>> This should be moved outside of 'if'.
>>>
>>
>> I was not really sure about the proper place - if it could be placed
>> before if() or after the if(){} - as I've not checked in depth
>
> Just use the reverse order against blk_register_queue() should be fine.
Yes - I think the order of release is correct.
>
>> connection between kobj and sysfs. It's somewhat unclear why all the
>> kobject operation are only within this if(){} block - so I've thought
>> there is some reason...
>> IMHO only elv_unregister_queue() should be probably in the if(){} block.
>>
>
> Seems it's a bug to put kobject_put(dev->kobj) in the if block.
>
> I created a stacked device (mdadm) and kmemleak still reported leaks
> even after I fixed the blktrace issue. And then I moved kobejct_put()
> outside the if, no more leaks reports.
>
Ok - I didn't have a testcase where the request_fn would be NULL.
So in this case I propose this patch:
---
Add missing blk_trace_remove_sysfs to be in pair with blk_trace_init_sysfs
introduced in commit 1d54ad6da9192fed5dd3b60224d9f2dfea0dcd82.
Release kobject also in case the request_fn is NULL.
Problem was noticed via kmemleak backtrace when some sysfs entries were
note properly destroyed during device removal:
unreferenced object 0xffff88001aa76640 (size 80):
comm "lvcreate", pid 2120, jiffies 4294885144
hex dump (first 32 bytes):
01 00 00 00 00 00 00 00 f0 65 a7 1a 00 88 ff ff .........e......
90 66 a7 1a 00 88 ff ff 86 1d 53 81 ff ff ff ff .f........S.....
backtrace:
[<ffffffff813f9cc6>] kmemleak_alloc+0x26/0x60
[<ffffffff8111d693>] kmem_cache_alloc+0x133/0x1c0
[<ffffffff81195891>] sysfs_new_dirent+0x41/0x120
[<ffffffff81194b0c>] sysfs_add_file_mode+0x3c/0xb0
[<ffffffff81197c81>] internal_create_group+0xc1/0x1a0
[<ffffffff81197d93>] sysfs_create_group+0x13/0x20
[<ffffffff810d8004>] blk_trace_init_sysfs+0x14/0x20
[<ffffffff8123f45c>] blk_register_queue+0x3c/0xf0
[<ffffffff812447e4>] add_disk+0x94/0x160
[<ffffffffa00d8b08>] dm_create+0x598/0x6e0 [dm_mod]
[<ffffffffa00de951>] dev_create+0x51/0x350 [dm_mod]
[<ffffffffa00de823>] ctl_ioctl+0x1a3/0x240 [dm_mod]
[<ffffffffa00de8f2>] dm_compat_ctl_ioctl+0x12/0x20 [dm_mod]
[<ffffffff81177bfd>] compat_sys_ioctl+0xcd/0x4f0
[<ffffffff81036ed8>] sysenter_dispatch+0x7/0x2c
[<ffffffffffffffff>] 0xffffffffffffffff
Signed-off-by: Zdenek Kabelac <zkabelac@...hat.com>
---
block/blk-sysfs.c | 11 ++++++-----
include/linux/blktrace_api.h | 2 ++
kernel/trace/blktrace.c | 5 +++++
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index b78c9c3..8a6d81a 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -452,6 +452,7 @@ int blk_register_queue(struct gendisk *disk)
if (ret) {
kobject_uevent(&q->kobj, KOBJ_REMOVE);
kobject_del(&q->kobj);
+ blk_trace_remove_sysfs(disk_to_dev(disk));
return ret;
}
@@ -465,11 +466,11 @@ void blk_unregister_queue(struct gendisk *disk)
if (WARN_ON(!q))
return;
- if (q->request_fn) {
+ if (q->request_fn)
elv_unregister_queue(q);
- kobject_uevent(&q->kobj, KOBJ_REMOVE);
- kobject_del(&q->kobj);
- kobject_put(&disk_to_dev(disk)->kobj);
- }
+ kobject_uevent(&q->kobj, KOBJ_REMOVE);
+ kobject_del(&q->kobj);
+ blk_trace_remove_sysfs(disk_to_dev(disk));
+ kobject_put(&disk_to_dev(disk)->kobj);
}
diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index 7e4350e..622939a 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -198,6 +198,7 @@ extern int blk_trace_setup(struct request_queue
*q, char *name, dev_t dev,
char __user *arg);
extern int blk_trace_startstop(struct request_queue *q, int start);
extern int blk_trace_remove(struct request_queue *q);
+extern void blk_trace_remove_sysfs(struct device *dev);
extern int blk_trace_init_sysfs(struct device *dev);
extern struct attribute_group blk_trace_attr_group;
@@ -211,6 +212,7 @@ extern struct attribute_group blk_trace_attr_group;
# define blk_trace_startstop(q, start) (-ENOTTY)
# define blk_trace_remove(q) (-ENOTTY)
# define blk_add_trace_msg(q, fmt, ...) do { } while (0)
+# define blk_trace_remove_sysfs(struct device *dev) do { } while (0)
static inline int blk_trace_init_sysfs(struct device *dev)
{
return 0;
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 3eb159c..60b5c5a 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -1657,6 +1657,11 @@ int blk_trace_init_sysfs(struct device *dev)
return sysfs_create_group(&dev->kobj, &blk_trace_attr_group);
}
+void blk_trace_remove_sysfs(struct device *dev)
+{
+ sysfs_remove_group(&dev->kobj, &blk_trace_attr_group);
+}
+
#endif /* CONFIG_BLK_DEV_IO_TRACE */
#ifdef CONFIG_EVENT_TRACING
--
1.6.4.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists