Re: [RFC PATCH v2 next]module: Fix mod->mkobj.kobj potentially freed too early
On Mon, Sep 02, 2013 at 09:21:55AM +0930, Rusty Russell wrote: > Greg KH writes: > > On Tue, Aug 27, 2013 at 02:08:27PM +0930, Rusty Russell wrote: > >> Greg KH writes: > >> > On Thu, Aug 22, 2013 at 03:37:55PM +0800, Li Zhong wrote: > >> >> DEBUG_KOBJECT_RELEASE helps to find the issue attached below. > >> > People are starting to hit these types of issues, and I'd like to take > >> > this one out of the picture. > >> > > >> > Rusty, any objection to me taking this through my driver-core tree, > >> > where this new config option shows up? > >> > >> The original fix was better. > >> > >> Moving the module_kobject out and giving it its own lifetime solves this > >> immediate issue, but it still means there's an accessible module_kobject > >> around referring to a module which doesn't exist any more. > > > > That's ok, it could happen before as well. What's wrong with that? > > > >> Original copied below, feel free to take it. > > > > You are just sitting and sleeping until someone drops the last reference > > to the module. What if userspace grabs a reference from sysfs? That > > could never return, I don't think you want to stall that out. > > In your scenario, what happens if userspace grabs a reference via sysfs? > It then tries to use module_kobj->mod which points into freed memory? > > eg. show_modinfo_##field or show_refcnt. The sysfs file will not be able to be "called" as Tejun fixed that up a long time ago, but yes, you are right, it really doesn't solve the issue. > Is there an owner field I'm missing somewhere which stops this from > happening? Otherwise, we can't unload the module until it's done. Good point. > > I'd prefer not having 2 things determining the lifecycle of a single > > object, that's messy, and not needed at all. > > Normally you'd grab a reference to the module via an owner pointer. > Doing that in kobject seems like overkill, so we're working around it > here... Ok, fair enough, your version is fine, feel free to add a: Acked-by: Greg Kroah-Hartman if you want it. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 next]module: Fix mod->mkobj.kobj potentially freed too early
Greg KH writes: > On Tue, Aug 27, 2013 at 02:08:27PM +0930, Rusty Russell wrote: >> Greg KH writes: >> > On Thu, Aug 22, 2013 at 03:37:55PM +0800, Li Zhong wrote: >> >> DEBUG_KOBJECT_RELEASE helps to find the issue attached below. >> > People are starting to hit these types of issues, and I'd like to take >> > this one out of the picture. >> > >> > Rusty, any objection to me taking this through my driver-core tree, >> > where this new config option shows up? >> >> The original fix was better. >> >> Moving the module_kobject out and giving it its own lifetime solves this >> immediate issue, but it still means there's an accessible module_kobject >> around referring to a module which doesn't exist any more. > > That's ok, it could happen before as well. What's wrong with that? > >> Original copied below, feel free to take it. > > You are just sitting and sleeping until someone drops the last reference > to the module. What if userspace grabs a reference from sysfs? That > could never return, I don't think you want to stall that out. In your scenario, what happens if userspace grabs a reference via sysfs? It then tries to use module_kobj->mod which points into freed memory? eg. show_modinfo_##field or show_refcnt. Is there an owner field I'm missing somewhere which stops this from happening? Otherwise, we can't unload the module until it's done. > I'd prefer not having 2 things determining the lifecycle of a single > object, that's messy, and not needed at all. Normally you'd grab a reference to the module via an owner pointer. Doing that in kobject seems like overkill, so we're working around it here... Hope that clarifies, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 next]module: Fix mod->mkobj.kobj potentially freed too early
On Tue, Aug 27, 2013 at 02:08:27PM +0930, Rusty Russell wrote: > Greg KH writes: > > On Thu, Aug 22, 2013 at 03:37:55PM +0800, Li Zhong wrote: > >> DEBUG_KOBJECT_RELEASE helps to find the issue attached below. > > People are starting to hit these types of issues, and I'd like to take > > this one out of the picture. > > > > Rusty, any objection to me taking this through my driver-core tree, > > where this new config option shows up? > > The original fix was better. > > Moving the module_kobject out and giving it its own lifetime solves this > immediate issue, but it still means there's an accessible module_kobject > around referring to a module which doesn't exist any more. That's ok, it could happen before as well. What's wrong with that? > Original copied below, feel free to take it. You are just sitting and sleeping until someone drops the last reference to the module. What if userspace grabs a reference from sysfs? That could never return, I don't think you want to stall that out. I'd prefer not having 2 things determining the lifecycle of a single object, that's messy, and not needed at all. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2 next]module: Fix mod->mkobj.kobj potentially freed too early
Greg KH writes:
> On Thu, Aug 22, 2013 at 03:37:55PM +0800, Li Zhong wrote:
>> DEBUG_KOBJECT_RELEASE helps to find the issue attached below.
> People are starting to hit these types of issues, and I'd like to take
> this one out of the picture.
>
> Rusty, any objection to me taking this through my driver-core tree,
> where this new config option shows up?
The original fix was better.
Moving the module_kobject out and giving it its own lifetime solves this
immediate issue, but it still means there's an accessible module_kobject
around referring to a module which doesn't exist any more.
Original copied below, feel free to take it.
Cheers,
Rusty.
From: Li Zhong
Subject: module: Fix mod->mkobj.kobj potentially freed too early
DEBUG_KOBJECT_RELEASE helps to find the issue attached below.
After some investigation, it seems the reason is:
The mod->mkobj.kobj(a01600d0 below) is freed together with mod
itself in free_module(). However, its children still hold references to
it, as the delay caused by DEBUG_KOBJECT_RELEASE. So when the
child(holders below) tries to decrease the reference count to its parent
in kobject_del(), BUG happens as it tries to access already freed memory.
This patch tries to fix it by waiting for the mod->mkobj.kobj to be
really released in the module removing process (and some error code
paths).
[ 1844.175287] kobject: 'holders' (88007c1f1600): kobject_release, parent
a01600d0 (delayed)
[ 1844.178991] kobject: 'notes' (8800370b2a00): kobject_release, parent
a01600d0 (delayed)
[ 1845.180118] kobject: 'holders' (88007c1f1600): kobject_cleanup, parent
a01600d0
[ 1845.182130] kobject: 'holders' (88007c1f1600): auto cleanup kobject_del
[ 1845.184120] BUG: unable to handle kernel paging request at a01601d0
[ 1845.185026] IP: [] kobject_put+0x11/0x60
[ 1845.185026] PGD 1a13067 PUD 1a14063 PMD 7bd30067 PTE 0
[ 1845.185026] Oops: [#1] PREEMPT
[ 1845.185026] Modules linked in: xfs libcrc32c [last unloaded: kprobe_example]
[ 1845.185026] CPU: 0 PID: 18 Comm: kworker/0:1 Tainted: G O
3.11.0-rc6-next-20130819+ #1
[ 1845.185026] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[ 1845.185026] Workqueue: events kobject_delayed_cleanup
[ 1845.185026] task: 88007ca51f00 ti: 88007ca5c000 task.ti:
88007ca5c000
[ 1845.185026] RIP: 0010:[] []
kobject_put+0x11/0x60
[ 1845.185026] RSP: 0018:88007ca5dd08 EFLAGS: 00010282
[ 1845.185026] RAX: 2000 RBX: a01600d0 RCX: 8177d638
[ 1845.185026] RDX: 88007ca5dc18 RSI: RDI: a01600d0
[ 1845.185026] RBP: 88007ca5dd18 R08: 824e9810 R09:
[ 1845.185026] R10: 8800 R11: dead4ead0001 R12: 81a95040
[ 1845.185026] R13: 88007b27a960 R14: 88007c1f1600 R15:
[ 1845.185026] FS: () GS:81a23000()
knlGS:
[ 1845.185026] CS: 0010 DS: ES: CR0: 8005003b
[ 1845.185026] CR2: a01601d0 CR3: 37207000 CR4: 06b0
[ 1845.185026] Stack:
[ 1845.185026] 88007c1f1600 88007c1f1600 88007ca5dd38
812cdb7e
[ 1845.185026] 88007c1f1640 88007ca5dd68
812cdbfe
[ 1845.185026] 88007c974800 88007c1f1640 88007ff61a00
[ 1845.185026] Call Trace:
[ 1845.185026] [] kobject_del+0x2e/0x40
[ 1845.185026] [] kobject_delayed_cleanup+0x6e/0x1d0
[ 1845.185026] [] process_one_work+0x1e5/0x670
[ 1845.185026] [] ? process_one_work+0x183/0x670
[ 1845.185026] [] worker_thread+0x113/0x370
[ 1845.185026] [] ? rescuer_thread+0x290/0x290
[ 1845.185026] [] kthread+0xda/0xe0
[ 1845.185026] [] ? _raw_spin_unlock_irq+0x30/0x60
[ 1845.185026] [] ? kthread_create_on_node+0x130/0x130
[ 1845.185026] [] ret_from_fork+0x7a/0xb0
[ 1845.185026] [] ? kthread_create_on_node+0x130/0x130
[ 1845.185026] Code: 81 48 c7 c7 28 95 ad 81 31 c0 e8 9b da 01 00 e9 4f ff ff
ff 66 0f 1f 44 00 00 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 85 ff 74 1d 87
00 01 00 00 01 74 1e 48 8d 7b 38 83 6b 38 01 0f 94 c0 84
[ 1845.185026] RIP [] kobject_put+0x11/0x60
[ 1845.185026] RSP
[ 1845.185026] CR2: a01601d0
[ 1845.185026] ---[ end trace 49a70afd109f5653 ]---
Signed-off-by: Li Zhong
Signed-off-by: Rusty Russell
---
include/linux/module.h | 1 +
kernel/module.c| 14 +++---
kernel/params.c| 7 +++
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 504035f..05f2447 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -42,6 +42,7 @@ struct module_kobject {
struct module *mod;
struct kobject *drivers_dir;
struct module_param_attrs *mp;
+ struct completion *kobj_completion;
};
struct module_attribute {
diff --git a/kernel/module.c b/kernel/module.c
index 40ee1dc..9f5ddae 100644
--- a/kernel/module.
Re: [RFC PATCH v2 next]module: Fix mod->mkobj.kobj potentially freed too early
On Thu, Aug 22, 2013 at 03:37:55PM +0800, Li Zhong wrote:
> DEBUG_KOBJECT_RELEASE helps to find the issue attached below.
>
> After some investigation, it seems the reason is:
> The mod->mkobj.kobj(a01600d0 below) is freed together with mod
> itself by module_free(mod, mod->module_core) in free_module(). However,
> its children still hold references to it, as the delay caused by
> DEBUG_KOBJECT_RELEASE. So when the child('holders' below) tries to
> decrease the reference count to its parent in kobject_del(), BUG happens
> as it tries to access already freed memory.
>
> v2:
> Greg suggested to make the kobject as a pointer. But it seems a little
> hard to make kobj(kobject) embedded in mkobj(module_kobject) a pointer,
> as that seems to cause getting the mkobj from the kobj impossible --
> to_module_kobject() is used in several places to derive mkobj from its
> member kobj.
>
> So in this version, I made the whole mkobj(module_kobject) as a pointer
> in mod(module).
>
> The mkobj is then allocated in mod_sysfs_init(), and freed in its member
> kobj's release function -- it seems that there is no mkobj usage after
> its kobj is released?
>
> [ 1844.175287] kobject: 'holders' (88007c1f1600): kobject_release, parent
> a01600d0 (delayed)
> [ 1844.178991] kobject: 'notes' (8800370b2a00): kobject_release, parent
> a01600d0 (delayed)
> [ 1845.180118] kobject: 'holders' (88007c1f1600): kobject_cleanup, parent
> a01600d0
> [ 1845.182130] kobject: 'holders' (88007c1f1600): auto cleanup kobject_del
> [ 1845.184120] BUG: unable to handle kernel paging request at a01601d0
> [ 1845.185026] IP: [] kobject_put+0x11/0x60
> [ 1845.185026] PGD 1a13067 PUD 1a14063 PMD 7bd30067 PTE 0
> [ 1845.185026] Oops: [#1] PREEMPT
> [ 1845.185026] Modules linked in: xfs libcrc32c [last unloaded:
> kprobe_example]
> [ 1845.185026] CPU: 0 PID: 18 Comm: kworker/0:1 Tainted: G O
> 3.11.0-rc6-next-20130819+ #1
> [ 1845.185026] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
> [ 1845.185026] Workqueue: events kobject_delayed_cleanup
> [ 1845.185026] task: 88007ca51f00 ti: 88007ca5c000 task.ti:
> 88007ca5c000
> [ 1845.185026] RIP: 0010:[] []
> kobject_put+0x11/0x60
> [ 1845.185026] RSP: 0018:88007ca5dd08 EFLAGS: 00010282
> [ 1845.185026] RAX: 2000 RBX: a01600d0 RCX:
> 8177d638
> [ 1845.185026] RDX: 88007ca5dc18 RSI: RDI:
> a01600d0
> [ 1845.185026] RBP: 88007ca5dd18 R08: 824e9810 R09:
>
> [ 1845.185026] R10: 8800 R11: dead4ead0001 R12:
> 81a95040
> [ 1845.185026] R13: 88007b27a960 R14: 88007c1f1600 R15:
>
> [ 1845.185026] FS: () GS:81a23000()
> knlGS:
> [ 1845.185026] CS: 0010 DS: ES: CR0: 8005003b
> [ 1845.185026] CR2: a01601d0 CR3: 37207000 CR4:
> 06b0
> [ 1845.185026] Stack:
> [ 1845.185026] 88007c1f1600 88007c1f1600 88007ca5dd38
> 812cdb7e
> [ 1845.185026] 88007c1f1640 88007ca5dd68
> 812cdbfe
> [ 1845.185026] 88007c974800 88007c1f1640 88007ff61a00
>
> [ 1845.185026] Call Trace:
> [ 1845.185026] [] kobject_del+0x2e/0x40
> [ 1845.185026] [] kobject_delayed_cleanup+0x6e/0x1d0
> [ 1845.185026] [] process_one_work+0x1e5/0x670
> [ 1845.185026] [] ? process_one_work+0x183/0x670
> [ 1845.185026] [] worker_thread+0x113/0x370
> [ 1845.185026] [] ? rescuer_thread+0x290/0x290
> [ 1845.185026] [] kthread+0xda/0xe0
> [ 1845.185026] [] ? _raw_spin_unlock_irq+0x30/0x60
> [ 1845.185026] [] ? kthread_create_on_node+0x130/0x130
> [ 1845.185026] [] ret_from_fork+0x7a/0xb0
> [ 1845.185026] [] ? kthread_create_on_node+0x130/0x130
> [ 1845.185026] Code: 81 48 c7 c7 28 95 ad 81 31 c0 e8 9b da 01 00 e9 4f ff ff
> ff 66 0f 1f 44 00 00 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 85 ff 74 1d
> 87 00 01 00 00 01 74 1e 48 8d 7b 38 83 6b 38 01 0f 94 c0 84
> [ 1845.185026] RIP [] kobject_put+0x11/0x60
> [ 1845.185026] RSP
> [ 1845.185026] CR2: a01601d0
> [ 1845.185026] ---[ end trace 49a70afd109f5653 ]---
>
> Signed-off-by: Li Zhong
> ---
> drivers/base/core.c| 2 +-
> drivers/base/module.c | 4 ++--
> include/linux/module.h | 2 +-
> kernel/module.c| 37 +
> kernel/params.c| 19 +--
> 5 files changed, 38 insertions(+), 26 deletions(-)
People are starting to hit these types of issues, and I'd like to take
this one out of the picture.
Rusty, any objection to me taking this through my driver-core tree,
where this new config option shows up?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 next]module: Fix mod->mkobj.kobj potentially freed too early
On Thu, Aug 22, 2013 at 03:37:55PM +0800, Li Zhong wrote:
> DEBUG_KOBJECT_RELEASE helps to find the issue attached below.
>
> After some investigation, it seems the reason is:
> The mod->mkobj.kobj(a01600d0 below) is freed together with mod
> itself by module_free(mod, mod->module_core) in free_module(). However,
> its children still hold references to it, as the delay caused by
> DEBUG_KOBJECT_RELEASE. So when the child('holders' below) tries to
> decrease the reference count to its parent in kobject_del(), BUG happens
> as it tries to access already freed memory.
>
> v2:
> Greg suggested to make the kobject as a pointer. But it seems a little
> hard to make kobj(kobject) embedded in mkobj(module_kobject) a pointer,
> as that seems to cause getting the mkobj from the kobj impossible --
> to_module_kobject() is used in several places to derive mkobj from its
> member kobj.
>
> So in this version, I made the whole mkobj(module_kobject) as a pointer
> in mod(module).
Thanks so much for doing this, it's the correct fix. I've added my
signed-off-by below, Rusty, will you be taking this in your tree for
3.12?
> The mkobj is then allocated in mod_sysfs_init(), and freed in its member
> kobj's release function -- it seems that there is no mkobj usage after
> its kobj is released?
>
> [ 1844.175287] kobject: 'holders' (88007c1f1600): kobject_release, parent
> a01600d0 (delayed)
> [ 1844.178991] kobject: 'notes' (8800370b2a00): kobject_release, parent
> a01600d0 (delayed)
> [ 1845.180118] kobject: 'holders' (88007c1f1600): kobject_cleanup, parent
> a01600d0
> [ 1845.182130] kobject: 'holders' (88007c1f1600): auto cleanup kobject_del
> [ 1845.184120] BUG: unable to handle kernel paging request at a01601d0
> [ 1845.185026] IP: [] kobject_put+0x11/0x60
> [ 1845.185026] PGD 1a13067 PUD 1a14063 PMD 7bd30067 PTE 0
> [ 1845.185026] Oops: [#1] PREEMPT
> [ 1845.185026] Modules linked in: xfs libcrc32c [last unloaded:
> kprobe_example]
> [ 1845.185026] CPU: 0 PID: 18 Comm: kworker/0:1 Tainted: G O
> 3.11.0-rc6-next-20130819+ #1
> [ 1845.185026] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
> [ 1845.185026] Workqueue: events kobject_delayed_cleanup
> [ 1845.185026] task: 88007ca51f00 ti: 88007ca5c000 task.ti:
> 88007ca5c000
> [ 1845.185026] RIP: 0010:[] []
> kobject_put+0x11/0x60
> [ 1845.185026] RSP: 0018:88007ca5dd08 EFLAGS: 00010282
> [ 1845.185026] RAX: 2000 RBX: a01600d0 RCX:
> 8177d638
> [ 1845.185026] RDX: 88007ca5dc18 RSI: RDI:
> a01600d0
> [ 1845.185026] RBP: 88007ca5dd18 R08: 824e9810 R09:
>
> [ 1845.185026] R10: 8800 R11: dead4ead0001 R12:
> 81a95040
> [ 1845.185026] R13: 88007b27a960 R14: 88007c1f1600 R15:
>
> [ 1845.185026] FS: () GS:81a23000()
> knlGS:
> [ 1845.185026] CS: 0010 DS: ES: CR0: 8005003b
> [ 1845.185026] CR2: a01601d0 CR3: 37207000 CR4:
> 06b0
> [ 1845.185026] Stack:
> [ 1845.185026] 88007c1f1600 88007c1f1600 88007ca5dd38
> 812cdb7e
> [ 1845.185026] 88007c1f1640 88007ca5dd68
> 812cdbfe
> [ 1845.185026] 88007c974800 88007c1f1640 88007ff61a00
>
> [ 1845.185026] Call Trace:
> [ 1845.185026] [] kobject_del+0x2e/0x40
> [ 1845.185026] [] kobject_delayed_cleanup+0x6e/0x1d0
> [ 1845.185026] [] process_one_work+0x1e5/0x670
> [ 1845.185026] [] ? process_one_work+0x183/0x670
> [ 1845.185026] [] worker_thread+0x113/0x370
> [ 1845.185026] [] ? rescuer_thread+0x290/0x290
> [ 1845.185026] [] kthread+0xda/0xe0
> [ 1845.185026] [] ? _raw_spin_unlock_irq+0x30/0x60
> [ 1845.185026] [] ? kthread_create_on_node+0x130/0x130
> [ 1845.185026] [] ret_from_fork+0x7a/0xb0
> [ 1845.185026] [] ? kthread_create_on_node+0x130/0x130
> [ 1845.185026] Code: 81 48 c7 c7 28 95 ad 81 31 c0 e8 9b da 01 00 e9 4f ff ff
> ff 66 0f 1f 44 00 00 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 85 ff 74 1d
> 87 00 01 00 00 01 74 1e 48 8d 7b 38 83 6b 38 01 0f 94 c0 84
> [ 1845.185026] RIP [] kobject_put+0x11/0x60
> [ 1845.185026] RSP
> [ 1845.185026] CR2: a01601d0
> [ 1845.185026] ---[ end trace 49a70afd109f5653 ]---
>
> Signed-off-by: Li Zhong
Signed-off-by: Greg Kroah-Hartman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
