LinuxLists.cc - [RFC PATCH next]module: Fix mod->mkobj.kobj potentially freed too early

2013-08-21 09:50:12

Subject: [RFC PATCH next]module: Fix mod->mkobj.kobj potentially freed too early

2013-08-21 16:18:22

Subject: Re: [RFC PATCH next]module: Fix mod->mkobj.kobj potentially freed too early

On Wed, Aug 21, 2013 at 05:49:58PM +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(ffffffffa01600d0 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.

Ah, thanks for tracking this down. I had seen this in my local testing,
but wasn't able to figure out the offending code.

> 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).

Nasty, we should just be freeing the structure in the release function,
why doesn't that work?

> [ 1844.175287] kobject: 'holders' (ffff88007c1f1600): kobject_release, parent ffffffffa01600d0 (delayed)
> [ 1844.178991] kobject: 'notes' (ffff8800370b2a00): kobject_release, parent ffffffffa01600d0 (delayed)
> [ 1845.180118] kobject: 'holders' (ffff88007c1f1600): kobject_cleanup, parent ffffffffa01600d0
> [ 1845.182130] kobject: 'holders' (ffff88007c1f1600): auto cleanup kobject_del
> [ 1845.184120] BUG: unable to handle kernel paging request at ffffffffa01601d0
> [ 1845.185026] IP: [<ffffffff812cda81>] kobject_put+0x11/0x60
> [ 1845.185026] PGD 1a13067 PUD 1a14063 PMD 7bd30067 PTE 0
> [ 1845.185026] Oops: 0000 [#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: ffff88007ca51f00 ti: ffff88007ca5c000 task.ti: ffff88007ca5c000
> [ 1845.185026] RIP: 0010:[<ffffffff812cda81>] [<ffffffff812cda81>] kobject_put+0x11/0x60
> [ 1845.185026] RSP: 0018:ffff88007ca5dd08 EFLAGS: 00010282
> [ 1845.185026] RAX: 0000000000002000 RBX: ffffffffa01600d0 RCX: ffffffff8177d638
> [ 1845.185026] RDX: ffff88007ca5dc18 RSI: 0000000000000000 RDI: ffffffffa01600d0
> [ 1845.185026] RBP: ffff88007ca5dd18 R08: ffffffff824e9810 R09: ffffffffffffffff
> [ 1845.185026] R10: ffff8800ffffffff R11: dead4ead00000001 R12: ffffffff81a95040
> [ 1845.185026] R13: ffff88007b27a960 R14: ffff88007c1f1600 R15: 0000000000000000
> [ 1845.185026] FS: 0000000000000000(0000) GS:ffffffff81a23000(0000) knlGS:0000000000000000
> [ 1845.185026] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [ 1845.185026] CR2: ffffffffa01601d0 CR3: 0000000037207000 CR4: 00000000000006b0
> [ 1845.185026] Stack:
> [ 1845.185026] ffff88007c1f1600 ffff88007c1f1600 ffff88007ca5dd38 ffffffff812cdb7e
> [ 1845.185026] 0000000000000000 ffff88007c1f1640 ffff88007ca5dd68 ffffffff812cdbfe
> [ 1845.185026] ffff88007c974800 ffff88007c1f1640 ffff88007ff61a00 0000000000000000
> [ 1845.185026] Call Trace:
> [ 1845.185026] [<ffffffff812cdb7e>] kobject_del+0x2e/0x40
> [ 1845.185026] [<ffffffff812cdbfe>] kobject_delayed_cleanup+0x6e/0x1d0
> [ 1845.185026] [<ffffffff81063a45>] process_one_work+0x1e5/0x670
> [ 1845.185026] [<ffffffff810639e3>] ? process_one_work+0x183/0x670
> [ 1845.185026] [<ffffffff810642b3>] worker_thread+0x113/0x370
> [ 1845.185026] [<ffffffff810641a0>] ? rescuer_thread+0x290/0x290
> [ 1845.185026] [<ffffffff8106bfba>] kthread+0xda/0xe0
> [ 1845.185026] [<ffffffff814ff0f0>] ? _raw_spin_unlock_irq+0x30/0x60
> [ 1845.185026] [<ffffffff8106bee0>] ? kthread_create_on_node+0x130/0x130
> [ 1845.185026] [<ffffffff8150751a>] ret_from_fork+0x7a/0xb0
> [ 1845.185026] [<ffffffff8106bee0>] ? 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 <f6> 87 00 01 00 00 01 74 1e 48 8d 7b 38 83 6b 38 01 0f 94 c0 84
> [ 1845.185026] RIP [<ffffffff812cda81>] kobject_put+0x11/0x60
> [ 1845.185026] RSP <ffff88007ca5dd08>
> [ 1845.185026] CR2: ffffffffa01601d0
> [ 1845.185026] ---[ end trace 49a70afd109f5653 ]---
>
> Signed-off-by: Li Zhong <[email protected]>
> ---
> 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 2069158..b5e2baa 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1611,6 +1611,14 @@ static void module_remove_modinfo_attrs(struct module *mod)
> kfree(mod->modinfo_attrs);
> }
>
> +static void mod_kobject_put(struct module *mod)
> +{
> + DECLARE_COMPLETION_ONSTACK(c);
> + mod->mkobj.kobj_completion = &c;
> + kobject_put(&mod->mkobj.kobj);
> + wait_for_completion(&c);
> +}
> +
> static int mod_sysfs_init(struct module *mod)
> {
> int err;
> @@ -1638,7 +1646,7 @@ static int mod_sysfs_init(struct module *mod)
> err = kobject_init_and_add(&mod->mkobj.kobj, &module_ktype, NULL,
> "%s", mod->name);
> if (err)
> - kobject_put(&mod->mkobj.kobj);
> + mod_kobject_put(mod);
>
> /* delay uevent until full sysfs population */
> out:
> @@ -1682,7 +1690,7 @@ out_unreg_param:
> out_unreg_holders:
> kobject_put(mod->holders_dir);
> out_unreg:
> - kobject_put(&mod->mkobj.kobj);
> + mod_kobject_put(mod);
> out:
> return err;
> }
> @@ -1691,7 +1699,7 @@ static void mod_sysfs_fini(struct module *mod)
> {
> remove_notes_attrs(mod);
> remove_sect_attrs(mod);
> - kobject_put(&mod->mkobj.kobj);
> + mod_kobject_put(mod);
> }
>
> #else /* !CONFIG_SYSFS */
> diff --git a/kernel/params.c b/kernel/params.c
> index 1f228a3..b8cab65 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -912,7 +912,14 @@ static const struct kset_uevent_ops module_uevent_ops = {
> struct kset *module_kset;
> int module_sysfs_initialized;
>
> +static void module_kobj_release(struct kobject *kobj)
> +{
> + struct module_kobject *mk = to_module_kobject(kobj);
> + complete(mk->kobj_completion);
> +}
> +
> struct kobj_type module_ktype = {
> + .release = module_kobj_release,
> .sysfs_ops = &module_sysfs_ops,
> };
>
>

Wait, as there is no release function here for the kobject (a different
problem), why is the deferred release function causing any problems?
There is no release function to call, so what is causing the oops?

confused,

greg k-h

2013-08-22 02:34:20

by Li Zhong

[permalink] [raw]

Subject: Re: [RFC PATCH next]module: Fix mod->mkobj.kobj potentially freed too early

On Wed, 2013-08-21 at 09:18 -0700, Greg KH wrote:
> On Wed, Aug 21, 2013 at 05:49:58PM +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(ffffffffa01600d0 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.
>
> Ah, thanks for tracking this down. I had seen this in my local testing,
> but wasn't able to figure out the offending code.
>
> > 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).
>
> Nasty, we should just be freeing the structure in the release function,
> why doesn't that work?

Hi Greg,

It seems I didn't describe it clearly in the previous mail. I'm trying
to do it better below:

mod->mkobj.kobj is embedded in module_kobject(not a pointer):
struct module_kobject {
struct kobject kobj;
...

and allocated with the module memory. So we could see the parent below
ffffffffa01600d0 is between MODULES_VADDR (ffffffffa0000000) and
MODULES_END(ffffffffff000000).

It seem to me that the mkobj.kobj is freed by module_free(mod,
mod->module_core).

So in free_module(), we need delay the call of module_free(), until
mkobj.kobj finishes its cleanup.

> > [ 1844.175287] kobject: 'holders' (ffff88007c1f1600): kobject_release, parent ffffffffa01600d0 (delayed)
> > [ 1844.178991] kobject: 'notes' (ffff8800370b2a00): kobject_release, parent ffffffffa01600d0 (delayed)
> > [ 1845.180118] kobject: 'holders' (ffff88007c1f1600): kobject_cleanup, parent ffffffffa01600d0
> > [ 1845.182130] kobject: 'holders' (ffff88007c1f1600): auto cleanup kobject_del
> > [ 1845.184120] BUG: unable to handle kernel paging request at ffffffffa01601d0
> > [ 1845.185026] IP: [<ffffffff812cda81>] kobject_put+0x11/0x60
> > [ 1845.185026] PGD 1a13067 PUD 1a14063 PMD 7bd30067 PTE 0
> > [ 1845.185026] Oops: 0000 [#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: ffff88007ca51f00 ti: ffff88007ca5c000 task.ti: ffff88007ca5c000
> > [ 1845.185026] RIP: 0010:[<ffffffff812cda81>] [<ffffffff812cda81>] kobject_put+0x11/0x60
> > [ 1845.185026] RSP: 0018:ffff88007ca5dd08 EFLAGS: 00010282
> > [ 1845.185026] RAX: 0000000000002000 RBX: ffffffffa01600d0 RCX: ffffffff8177d638
> > [ 1845.185026] RDX: ffff88007ca5dc18 RSI: 0000000000000000 RDI: ffffffffa01600d0
> > [ 1845.185026] RBP: ffff88007ca5dd18 R08: ffffffff824e9810 R09: ffffffffffffffff
> > [ 1845.185026] R10: ffff8800ffffffff R11: dead4ead00000001 R12: ffffffff81a95040
> > [ 1845.185026] R13: ffff88007b27a960 R14: ffff88007c1f1600 R15: 0000000000000000
> > [ 1845.185026] FS: 0000000000000000(0000) GS:ffffffff81a23000(0000) knlGS:0000000000000000
> > [ 1845.185026] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> > [ 1845.185026] CR2: ffffffffa01601d0 CR3: 0000000037207000 CR4: 00000000000006b0
> > [ 1845.185026] Stack:
> > [ 1845.185026] ffff88007c1f1600 ffff88007c1f1600 ffff88007ca5dd38 ffffffff812cdb7e
> > [ 1845.185026] 0000000000000000 ffff88007c1f1640 ffff88007ca5dd68 ffffffff812cdbfe
> > [ 1845.185026] ffff88007c974800 ffff88007c1f1640 ffff88007ff61a00 0000000000000000
> > [ 1845.185026] Call Trace:
> > [ 1845.185026] [<ffffffff812cdb7e>] kobject_del+0x2e/0x40
> > [ 1845.185026] [<ffffffff812cdbfe>] kobject_delayed_cleanup+0x6e/0x1d0
> > [ 1845.185026] [<ffffffff81063a45>] process_one_work+0x1e5/0x670
> > [ 1845.185026] [<ffffffff810639e3>] ? process_one_work+0x183/0x670
> > [ 1845.185026] [<ffffffff810642b3>] worker_thread+0x113/0x370
> > [ 1845.185026] [<ffffffff810641a0>] ? rescuer_thread+0x290/0x290
> > [ 1845.185026] [<ffffffff8106bfba>] kthread+0xda/0xe0
> > [ 1845.185026] [<ffffffff814ff0f0>] ? _raw_spin_unlock_irq+0x30/0x60
> > [ 1845.185026] [<ffffffff8106bee0>] ? kthread_create_on_node+0x130/0x130
> > [ 1845.185026] [<ffffffff8150751a>] ret_from_fork+0x7a/0xb0
> > [ 1845.185026] [<ffffffff8106bee0>] ? 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 <f6> 87 00 01 00 00 01 74 1e 48 8d 7b 38 83 6b 38 01 0f 94 c0 84
> > [ 1845.185026] RIP [<ffffffff812cda81>] kobject_put+0x11/0x60
> > [ 1845.185026] RSP <ffff88007ca5dd08>
> > [ 1845.185026] CR2: ffffffffa01601d0
> > [ 1845.185026] ---[ end trace 49a70afd109f5653 ]---
> >
> > Signed-off-by: Li Zhong <[email protected]>
> > ---
> > 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 2069158..b5e2baa 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -1611,6 +1611,14 @@ static void module_remove_modinfo_attrs(struct module *mod)
> > kfree(mod->modinfo_attrs);
> > }
> >
> > +static void mod_kobject_put(struct module *mod)
> > +{
> > + DECLARE_COMPLETION_ONSTACK(c);
> > + mod->mkobj.kobj_completion = &c;
> > + kobject_put(&mod->mkobj.kobj);
> > + wait_for_completion(&c);
> > +}
> > +
> > static int mod_sysfs_init(struct module *mod)
> > {
> > int err;
> > @@ -1638,7 +1646,7 @@ static int mod_sysfs_init(struct module *mod)
> > err = kobject_init_and_add(&mod->mkobj.kobj, &module_ktype, NULL,
> > "%s", mod->name);
> > if (err)
> > - kobject_put(&mod->mkobj.kobj);
> > + mod_kobject_put(mod);
> >
> > /* delay uevent until full sysfs population */
> > out:
> > @@ -1682,7 +1690,7 @@ out_unreg_param:
> > out_unreg_holders:
> > kobject_put(mod->holders_dir);
> > out_unreg:
> > - kobject_put(&mod->mkobj.kobj);
> > + mod_kobject_put(mod);
> > out:
> > return err;
> > }
> > @@ -1691,7 +1699,7 @@ static void mod_sysfs_fini(struct module *mod)
> > {
> > remove_notes_attrs(mod);
> > remove_sect_attrs(mod);
> > - kobject_put(&mod->mkobj.kobj);
> > + mod_kobject_put(mod);
> > }
> >
> > #else /* !CONFIG_SYSFS */
> > diff --git a/kernel/params.c b/kernel/params.c
> > index 1f228a3..b8cab65 100644
> > --- a/kernel/params.c
> > +++ b/kernel/params.c
> > @@ -912,7 +912,14 @@ static const struct kset_uevent_ops module_uevent_ops = {
> > struct kset *module_kset;
> > int module_sysfs_initialized;
> >
> > +static void module_kobj_release(struct kobject *kobj)
> > +{
> > + struct module_kobject *mk = to_module_kobject(kobj);
> > + complete(mk->kobj_completion);
> > +}
> > +
> > struct kobj_type module_ktype = {
> > + .release = module_kobj_release,
> > .sysfs_ops = &module_sysfs_ops,
> > };
> >
> >
>
>
> Wait, as there is no release function here for the kobject (a different
> problem), why is the deferred release function causing any problems?
> There is no release function to call, so what is causing the oops?
>
I think that's because, kobject_cleanup() is delayed,
which needs access to the contents in the kobj, and also needs to
kobject_put() its parent's reference counter (in kobject_del()).

But mkobj.kobj is already freed by module_free(mod, mod->module_core).

So its children have problem calling kobject_put(kobj->parent), and
itself has problem calling kobject_cleanup().

Thanks, Zhong

> confused,
>
> greg k-h
>

2013-08-22 04:01:41

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [RFC PATCH next]module: Fix mod->mkobj.kobj potentially freed too early

On Thu, Aug 22, 2013 at 10:34:06AM +0800, Li Zhong wrote:
> On Wed, 2013-08-21 at 09:18 -0700, Greg KH wrote:
> > On Wed, Aug 21, 2013 at 05:49:58PM +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(ffffffffa01600d0 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.
> >
> > Ah, thanks for tracking this down. I had seen this in my local testing,
> > but wasn't able to figure out the offending code.
> >
> > > 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).
> >
> > Nasty, we should just be freeing the structure in the release function,
> > why doesn't that work?
>
> Hi Greg,
>
> It seems I didn't describe it clearly in the previous mail. I'm trying
> to do it better below:
>
> mod->mkobj.kobj is embedded in module_kobject(not a pointer):
> struct module_kobject {
> struct kobject kobj;
> ...
>
> and allocated with the module memory. So we could see the parent below
> ffffffffa01600d0 is between MODULES_VADDR (ffffffffa0000000) and
> MODULES_END(ffffffffff000000).
>
> It seem to me that the mkobj.kobj is freed by module_free(mod,
> mod->module_core).

Ick, you are right. If a kobject is being embedded in an object, it
should control the lifespan of the object, not somewhere else like is
happening here.

The best solution for this is to make the kobject a pointer, not
embedded in the structure, that will fix this issue, right?

thanks,

greg k-h

2013-08-22 05:38:10

by Li Zhong

[permalink] [raw]

Subject: Re: [RFC PATCH next]module: Fix mod->mkobj.kobj potentially freed too early

On Wed, 2013-08-21 at 21:03 -0700, Greg KH wrote:
> On Thu, Aug 22, 2013 at 10:34:06AM +0800, Li Zhong wrote:
> > On Wed, 2013-08-21 at 09:18 -0700, Greg KH wrote:
> > > On Wed, Aug 21, 2013 at 05:49:58PM +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(ffffffffa01600d0 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.
> > >
> > > Ah, thanks for tracking this down. I had seen this in my local testing,
> > > but wasn't able to figure out the offending code.
> > >
> > > > 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).
> > >
> > > Nasty, we should just be freeing the structure in the release function,
> > > why doesn't that work?
> >
> > Hi Greg,
> >
> > It seems I didn't describe it clearly in the previous mail. I'm trying
> > to do it better below:
> >
> > mod->mkobj.kobj is embedded in module_kobject(not a pointer):
> > struct module_kobject {
> > struct kobject kobj;
> > ...
> >
> > and allocated with the module memory. So we could see the parent below
> > ffffffffa01600d0 is between MODULES_VADDR (ffffffffa0000000) and
> > MODULES_END(ffffffffff000000).
> >
> > It seem to me that the mkobj.kobj is freed by module_free(mod,
> > mod->module_core).
>
> Ick, you are right. If a kobject is being embedded in an object, it
> should control the lifespan of the object, not somewhere else like is
> happening here.
>
> The best solution for this is to make the kobject a pointer, not
> embedded in the structure, that will fix this issue, right?

Yes, I think so. I'll try to write a fix using this way, thanks for your
suggestion.

Thanks, Zhong

>
> thanks,
>
> greg k-h
>

2013-08-22 07:11:19

by Rusty Russell

[permalink] [raw]

Subject: Re: [RFC PATCH next]module: Fix mod->mkobj.kobj potentially freed too early

Greg KH <[email protected]> writes:
> On Wed, Aug 21, 2013 at 05:49:58PM +0800, Li Zhong wrote:
> > struct kobj_type module_ktype = {
> > + .release = module_kobj_release,
> > .sysfs_ops = &module_sysfs_ops,
> > };
>
> Wait, as there is no release function here for the kobject (a different
> problem), why is the deferred release function causing any problems?
> There is no release function to call, so what is causing the oops?

Because DEBUG_KOBJECT_RELEASE does the kobject_put() sometime later,
which is what causes the oops.

Since kobjects don't have an owner field, AFAICT someone *could* grab
one in a module which is unloading, then put it after unload. So this
fixes a real bug, albeit not one seen in the real world.

Applied,
Rusty.

2013-08-22 07:38:09

by Li Zhong

[permalink] [raw]

Subject: [RFC PATCH v2 next]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(ffffffffa01600d0 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' (ffff88007c1f1600): kobject_release, parent ffffffffa01600d0 (delayed)
[ 1844.178991] kobject: 'notes' (ffff8800370b2a00): kobject_release, parent ffffffffa01600d0 (delayed)
[ 1845.180118] kobject: 'holders' (ffff88007c1f1600): kobject_cleanup, parent ffffffffa01600d0
[ 1845.182130] kobject: 'holders' (ffff88007c1f1600): auto cleanup kobject_del
[ 1845.184120] BUG: unable to handle kernel paging request at ffffffffa01601d0
[ 1845.185026] IP: [<ffffffff812cda81>] kobject_put+0x11/0x60
[ 1845.185026] PGD 1a13067 PUD 1a14063 PMD 7bd30067 PTE 0
[ 1845.185026] Oops: 0000 [#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: ffff88007ca51f00 ti: ffff88007ca5c000 task.ti: ffff88007ca5c000
[ 1845.185026] RIP: 0010:[<ffffffff812cda81>] [<ffffffff812cda81>] kobject_put+0x11/0x60
[ 1845.185026] RSP: 0018:ffff88007ca5dd08 EFLAGS: 00010282
[ 1845.185026] RAX: 0000000000002000 RBX: ffffffffa01600d0 RCX: ffffffff8177d638
[ 1845.185026] RDX: ffff88007ca5dc18 RSI: 0000000000000000 RDI: ffffffffa01600d0
[ 1845.185026] RBP: ffff88007ca5dd18 R08: ffffffff824e9810 R09: ffffffffffffffff
[ 1845.185026] R10: ffff8800ffffffff R11: dead4ead00000001 R12: ffffffff81a95040
[ 1845.185026] R13: ffff88007b27a960 R14: ffff88007c1f1600 R15: 0000000000000000
[ 1845.185026] FS: 0000000000000000(0000) GS:ffffffff81a23000(0000) knlGS:0000000000000000
[ 1845.185026] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 1845.185026] CR2: ffffffffa01601d0 CR3: 0000000037207000 CR4: 00000000000006b0
[ 1845.185026] Stack:
[ 1845.185026] ffff88007c1f1600 ffff88007c1f1600 ffff88007ca5dd38 ffffffff812cdb7e
[ 1845.185026] 0000000000000000 ffff88007c1f1640 ffff88007ca5dd68 ffffffff812cdbfe
[ 1845.185026] ffff88007c974800 ffff88007c1f1640 ffff88007ff61a00 0000000000000000
[ 1845.185026] Call Trace:
[ 1845.185026] [<ffffffff812cdb7e>] kobject_del+0x2e/0x40
[ 1845.185026] [<ffffffff812cdbfe>] kobject_delayed_cleanup+0x6e/0x1d0
[ 1845.185026] [<ffffffff81063a45>] process_one_work+0x1e5/0x670
[ 1845.185026] [<ffffffff810639e3>] ? process_one_work+0x183/0x670
[ 1845.185026] [<ffffffff810642b3>] worker_thread+0x113/0x370
[ 1845.185026] [<ffffffff810641a0>] ? rescuer_thread+0x290/0x290
[ 1845.185026] [<ffffffff8106bfba>] kthread+0xda/0xe0
[ 1845.185026] [<ffffffff814ff0f0>] ? _raw_spin_unlock_irq+0x30/0x60
[ 1845.185026] [<ffffffff8106bee0>] ? kthread_create_on_node+0x130/0x130
[ 1845.185026] [<ffffffff8150751a>] ret_from_fork+0x7a/0xb0
[ 1845.185026] [<ffffffff8106bee0>] ? 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 <f6> 87 00 01 00 00 01 74 1e 48 8d 7b 38 83 6b 38 01 0f 94 c0 84
[ 1845.185026] RIP [<ffffffff812cda81>] kobject_put+0x11/0x60
[ 1845.185026] RSP <ffff88007ca5dd08>
[ 1845.185026] CR2: ffffffffa01601d0
[ 1845.185026] ---[ end trace 49a70afd109f5653 ]---

Signed-off-by: Li Zhong <[email protected]>
---
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(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 09a99d6..b8a1fc6 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1629,7 +1629,7 @@ struct device *__root_device_register(const char *name, struct module *owner)

#ifdef CONFIG_MODULES /* gotta find a "cleaner" way to do this */
if (owner) {
- struct module_kobject *mk = &owner->mkobj;
+ struct module_kobject *mk = owner->mkobj;

err = sysfs_create_link(&root->dev.kobj, &mk->kobj, "module");
if (err) {
diff --git a/drivers/base/module.c b/drivers/base/module.c
index db930d3..7b7bd5a 100644
--- a/drivers/base/module.c
+++ b/drivers/base/module.c
@@ -40,7 +40,7 @@ void module_add_driver(struct module *mod, struct device_driver *drv)
return;

if (mod)
- mk = &mod->mkobj;
+ mk = mod->mkobj;
else if (drv->mod_name) {
struct kobject *mkobj;

@@ -80,7 +80,7 @@ void module_remove_driver(struct device_driver *drv)
sysfs_remove_link(&drv->p->kobj, "module");

if (drv->owner)
- mk = &drv->owner->mkobj;
+ mk = drv->owner->mkobj;
else if (drv->p->mkobj)
mk = drv->p->mkobj;
if (mk && mk->drivers_dir) {
diff --git a/include/linux/module.h b/include/linux/module.h
index 504035f..a499db6 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -236,7 +236,7 @@ struct module
char name[MODULE_NAME_LEN];

/* Sysfs stuff. */
- struct module_kobject mkobj;
+ struct module_kobject *mkobj;
struct module_attribute *modinfo_attrs;
const char *version;
const char *srcversion;
diff --git a/kernel/module.c b/kernel/module.c
index 2069158..48060b4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1402,7 +1402,7 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info)
}
*gattr = NULL;

- if (sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp))
+ if (sysfs_create_group(&mod->mkobj->kobj, &sect_attrs->grp))
goto out;

mod->sect_attrs = sect_attrs;
@@ -1414,7 +1414,7 @@ static void add_sect_attrs(struct module *mod, const struct load_info *info)
static void remove_sect_attrs(struct module *mod)
{
if (mod->sect_attrs) {
- sysfs_remove_group(&mod->mkobj.kobj,
+ sysfs_remove_group(&mod->mkobj->kobj,
&mod->sect_attrs->grp);
/* We are positive that no one is using any sect attrs
* at this point. Deallocate immediately. */
@@ -1499,7 +1499,7 @@ static void add_notes_attrs(struct module *mod, const struct load_info *info)
++loaded;
}

- notes_attrs->dir = kobject_create_and_add("notes", &mod->mkobj.kobj);
+ notes_attrs->dir = kobject_create_and_add("notes", &mod->mkobj->kobj);
if (!notes_attrs->dir)
goto out;

@@ -1551,7 +1551,7 @@ static void add_usage_links(struct module *mod)
mutex_lock(&module_mutex);
list_for_each_entry(use, &mod->target_list, target_list) {
nowarn = sysfs_create_link(use->target->holders_dir,
- &mod->mkobj.kobj, mod->name);
+ &mod->mkobj->kobj, mod->name);
}
mutex_unlock(&module_mutex);
#endif
@@ -1588,7 +1588,8 @@ static int module_add_modinfo_attrs(struct module *mod)
(attr->test && attr->test(mod))) {
memcpy(temp_attr, attr, sizeof(*temp_attr));
sysfs_attr_init(&temp_attr->attr);
- error = sysfs_create_file(&mod->mkobj.kobj,&temp_attr->attr);
+ error = sysfs_create_file(&mod->mkobj->kobj,
+ &temp_attr->attr);
++temp_attr;
}
}
@@ -1604,7 +1605,7 @@ static void module_remove_modinfo_attrs(struct module *mod)
/* pick a field to test for end of list */
if (!attr->attr.name)
break;
- sysfs_remove_file(&mod->mkobj.kobj,&attr->attr);
+ sysfs_remove_file(&mod->mkobj->kobj, &attr->attr);
if (attr->free)
attr->free(mod);
}
@@ -1630,15 +1631,19 @@ static int mod_sysfs_init(struct module *mod)
err = -EINVAL;
goto out;
}
+ mod->mkobj = kzalloc(sizeof(*mod->mkobj), GFP_KERNEL);
+ if (!mod->mkobj) {
+ err = -ENOMEM;
+ goto out;
+ }

- mod->mkobj.mod = mod;
+ mod->mkobj->mod = mod;

- memset(&mod->mkobj.kobj, 0, sizeof(mod->mkobj.kobj));
- mod->mkobj.kobj.kset = module_kset;
- err = kobject_init_and_add(&mod->mkobj.kobj, &module_ktype, NULL,
+ mod->mkobj->kobj.kset = module_kset;
+ err = kobject_init_and_add(&mod->mkobj->kobj, &module_ktype, NULL,
"%s", mod->name);
if (err)
- kobject_put(&mod->mkobj.kobj);
+ kobject_put(&mod->mkobj->kobj);

/* delay uevent until full sysfs population */
out:
@@ -1656,7 +1661,7 @@ static int mod_sysfs_setup(struct module *mod,
if (err)
goto out;

- mod->holders_dir = kobject_create_and_add("holders", &mod->mkobj.kobj);
+ mod->holders_dir = kobject_create_and_add("holders", &mod->mkobj->kobj);
if (!mod->holders_dir) {
err = -ENOMEM;
goto out_unreg;
@@ -1674,7 +1679,7 @@ static int mod_sysfs_setup(struct module *mod,
add_sect_attrs(mod, info);
add_notes_attrs(mod, info);

- kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD);
+ kobject_uevent(&mod->mkobj->kobj, KOBJ_ADD);
return 0;

out_unreg_param:
@@ -1682,7 +1687,7 @@ out_unreg_param:
out_unreg_holders:
kobject_put(mod->holders_dir);
out_unreg:
- kobject_put(&mod->mkobj.kobj);
+ kobject_put(&mod->mkobj->kobj);
out:
return err;
}
@@ -1691,7 +1696,7 @@ static void mod_sysfs_fini(struct module *mod)
{
remove_notes_attrs(mod);
remove_sect_attrs(mod);
- kobject_put(&mod->mkobj.kobj);
+ kobject_put(&mod->mkobj->kobj);
}

#else /* !CONFIG_SYSFS */
@@ -1723,7 +1728,7 @@ static void mod_sysfs_teardown(struct module *mod)
del_usage_links(mod);
module_remove_modinfo_attrs(mod);
module_param_sysfs_remove(mod);
- kobject_put(mod->mkobj.drivers_dir);
+ kobject_put(mod->mkobj->drivers_dir);
kobject_put(mod->holders_dir);
mod_sysfs_fini(mod);
}
diff --git a/kernel/params.c b/kernel/params.c
index 1f228a3..6bce9db 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -684,7 +684,7 @@ int module_param_sysfs_setup(struct module *mod,
for (i = 0; i < num_params; i++) {
if (kparam[i].perm == 0)
continue;
- err = add_sysfs_param(&mod->mkobj, &kparam[i], kparam[i].name);
+ err = add_sysfs_param(mod->mkobj, &kparam[i], kparam[i].name);
if (err)
return err;
params = true;
@@ -694,9 +694,9 @@ int module_param_sysfs_setup(struct module *mod,
return 0;

/* Create the param group. */
- err = sysfs_create_group(&mod->mkobj.kobj, &mod->mkobj.mp->grp);
+ err = sysfs_create_group(&mod->mkobj->kobj, &mod->mkobj->mp->grp);
if (err)
- free_module_param_attrs(&mod->mkobj);
+ free_module_param_attrs(mod->mkobj);
return err;
}

@@ -709,11 +709,11 @@ int module_param_sysfs_setup(struct module *mod,
*/
void module_param_sysfs_remove(struct module *mod)
{
- if (mod->mkobj.mp) {
- sysfs_remove_group(&mod->mkobj.kobj, &mod->mkobj.mp->grp);
+ if (mod->mkobj->mp) {
+ sysfs_remove_group(&mod->mkobj->kobj, &mod->mkobj->mp->grp);
/* We are positive that no one is using any param
* attrs at this point. Deallocate immediately. */
- free_module_param_attrs(&mod->mkobj);
+ free_module_param_attrs(mod->mkobj);
}
}
#endif
@@ -912,7 +912,14 @@ static const struct kset_uevent_ops module_uevent_ops = {
struct kset *module_kset;
int module_sysfs_initialized;

+static void module_kobj_release(struct kobject *kobj)
+{
+ struct module_kobject *mk = to_module_kobject(kobj);
+ kfree(mk);
+}
+
struct kobj_type module_ktype = {
+ .release = module_kobj_release,
.sysfs_ops = &module_sysfs_ops,
};

2013-08-22 07:50:49

by Li Zhong

[permalink] [raw]

Subject: Re: [RFC PATCH next]module: Fix mod->mkobj.kobj potentially freed too early

On Thu, 2013-08-22 at 16:30 +0930, Rusty Russell wrote:
> Greg KH <[email protected]> writes:
> > On Wed, Aug 21, 2013 at 05:49:58PM +0800, Li Zhong wrote:
> > > struct kobj_type module_ktype = {
> > > + .release = module_kobj_release,
> > > .sysfs_ops = &module_sysfs_ops,
> > > };
> >
> > Wait, as there is no release function here for the kobject (a different
> > problem), why is the deferred release function causing any problems?
> > There is no release function to call, so what is causing the oops?
>
> Because DEBUG_KOBJECT_RELEASE does the kobject_put() sometime later,
> which is what causes the oops.
>
> Since kobjects don't have an owner field, AFAICT someone *could* grab
> one in a module which is unloading, then put it after unload. So this
> fixes a real bug, albeit not one seen in the real world.
>
> Applied,

Oh, thank you, Rusty.

I just sent out another version... which fix it in another way as Greg
suggested, could you please also help to take a look at it?

Thanks, Zhong

> Rusty.
>

2013-08-22 21:58:12

by Greg Kroah-Hartman

[permalink] [raw]

Subject: 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(ffffffffa01600d0 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' (ffff88007c1f1600): kobject_release, parent ffffffffa01600d0 (delayed)
> [ 1844.178991] kobject: 'notes' (ffff8800370b2a00): kobject_release, parent ffffffffa01600d0 (delayed)
> [ 1845.180118] kobject: 'holders' (ffff88007c1f1600): kobject_cleanup, parent ffffffffa01600d0
> [ 1845.182130] kobject: 'holders' (ffff88007c1f1600): auto cleanup kobject_del
> [ 1845.184120] BUG: unable to handle kernel paging request at ffffffffa01601d0
> [ 1845.185026] IP: [<ffffffff812cda81>] kobject_put+0x11/0x60
> [ 1845.185026] PGD 1a13067 PUD 1a14063 PMD 7bd30067 PTE 0
> [ 1845.185026] Oops: 0000 [#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: ffff88007ca51f00 ti: ffff88007ca5c000 task.ti: ffff88007ca5c000
> [ 1845.185026] RIP: 0010:[<ffffffff812cda81>] [<ffffffff812cda81>] kobject_put+0x11/0x60
> [ 1845.185026] RSP: 0018:ffff88007ca5dd08 EFLAGS: 00010282
> [ 1845.185026] RAX: 0000000000002000 RBX: ffffffffa01600d0 RCX: ffffffff8177d638
> [ 1845.185026] RDX: ffff88007ca5dc18 RSI: 0000000000000000 RDI: ffffffffa01600d0
> [ 1845.185026] RBP: ffff88007ca5dd18 R08: ffffffff824e9810 R09: ffffffffffffffff
> [ 1845.185026] R10: ffff8800ffffffff R11: dead4ead00000001 R12: ffffffff81a95040
> [ 1845.185026] R13: ffff88007b27a960 R14: ffff88007c1f1600 R15: 0000000000000000
> [ 1845.185026] FS: 0000000000000000(0000) GS:ffffffff81a23000(0000) knlGS:0000000000000000
> [ 1845.185026] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [ 1845.185026] CR2: ffffffffa01601d0 CR3: 0000000037207000 CR4: 00000000000006b0
> [ 1845.185026] Stack:
> [ 1845.185026] ffff88007c1f1600 ffff88007c1f1600 ffff88007ca5dd38 ffffffff812cdb7e
> [ 1845.185026] 0000000000000000 ffff88007c1f1640 ffff88007ca5dd68 ffffffff812cdbfe
> [ 1845.185026] ffff88007c974800 ffff88007c1f1640 ffff88007ff61a00 0000000000000000
> [ 1845.185026] Call Trace:
> [ 1845.185026] [<ffffffff812cdb7e>] kobject_del+0x2e/0x40
> [ 1845.185026] [<ffffffff812cdbfe>] kobject_delayed_cleanup+0x6e/0x1d0
> [ 1845.185026] [<ffffffff81063a45>] process_one_work+0x1e5/0x670
> [ 1845.185026] [<ffffffff810639e3>] ? process_one_work+0x183/0x670
> [ 1845.185026] [<ffffffff810642b3>] worker_thread+0x113/0x370
> [ 1845.185026] [<ffffffff810641a0>] ? rescuer_thread+0x290/0x290
> [ 1845.185026] [<ffffffff8106bfba>] kthread+0xda/0xe0
> [ 1845.185026] [<ffffffff814ff0f0>] ? _raw_spin_unlock_irq+0x30/0x60
> [ 1845.185026] [<ffffffff8106bee0>] ? kthread_create_on_node+0x130/0x130
> [ 1845.185026] [<ffffffff8150751a>] ret_from_fork+0x7a/0xb0
> [ 1845.185026] [<ffffffff8106bee0>] ? 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 <f6> 87 00 01 00 00 01 74 1e 48 8d 7b 38 83 6b 38 01 0f 94 c0 84
> [ 1845.185026] RIP [<ffffffff812cda81>] kobject_put+0x11/0x60
> [ 1845.185026] RSP <ffff88007ca5dd08>
> [ 1845.185026] CR2: ffffffffa01601d0
> [ 1845.185026] ---[ end trace 49a70afd109f5653 ]---
>
> Signed-off-by: Li Zhong <[email protected]>

Signed-off-by: Greg Kroah-Hartman <[email protected]>

2013-08-25 04:05:27

by Greg Kroah-Hartman

[permalink] [raw]

Subject: 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(ffffffffa01600d0 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' (ffff88007c1f1600): kobject_release, parent ffffffffa01600d0 (delayed)
> [ 1844.178991] kobject: 'notes' (ffff8800370b2a00): kobject_release, parent ffffffffa01600d0 (delayed)
> [ 1845.180118] kobject: 'holders' (ffff88007c1f1600): kobject_cleanup, parent ffffffffa01600d0
> [ 1845.182130] kobject: 'holders' (ffff88007c1f1600): auto cleanup kobject_del
> [ 1845.184120] BUG: unable to handle kernel paging request at ffffffffa01601d0
> [ 1845.185026] IP: [<ffffffff812cda81>] kobject_put+0x11/0x60
> [ 1845.185026] PGD 1a13067 PUD 1a14063 PMD 7bd30067 PTE 0
> [ 1845.185026] Oops: 0000 [#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: ffff88007ca51f00 ti: ffff88007ca5c000 task.ti: ffff88007ca5c000
> [ 1845.185026] RIP: 0010:[<ffffffff812cda81>] [<ffffffff812cda81>] kobject_put+0x11/0x60
> [ 1845.185026] RSP: 0018:ffff88007ca5dd08 EFLAGS: 00010282
> [ 1845.185026] RAX: 0000000000002000 RBX: ffffffffa01600d0 RCX: ffffffff8177d638
> [ 1845.185026] RDX: ffff88007ca5dc18 RSI: 0000000000000000 RDI: ffffffffa01600d0
> [ 1845.185026] RBP: ffff88007ca5dd18 R08: ffffffff824e9810 R09: ffffffffffffffff
> [ 1845.185026] R10: ffff8800ffffffff R11: dead4ead00000001 R12: ffffffff81a95040
> [ 1845.185026] R13: ffff88007b27a960 R14: ffff88007c1f1600 R15: 0000000000000000
> [ 1845.185026] FS: 0000000000000000(0000) GS:ffffffff81a23000(0000) knlGS:0000000000000000
> [ 1845.185026] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [ 1845.185026] CR2: ffffffffa01601d0 CR3: 0000000037207000 CR4: 00000000000006b0
> [ 1845.185026] Stack:
> [ 1845.185026] ffff88007c1f1600 ffff88007c1f1600 ffff88007ca5dd38 ffffffff812cdb7e
> [ 1845.185026] 0000000000000000 ffff88007c1f1640 ffff88007ca5dd68 ffffffff812cdbfe
> [ 1845.185026] ffff88007c974800 ffff88007c1f1640 ffff88007ff61a00 0000000000000000
> [ 1845.185026] Call Trace:
> [ 1845.185026] [<ffffffff812cdb7e>] kobject_del+0x2e/0x40
> [ 1845.185026] [<ffffffff812cdbfe>] kobject_delayed_cleanup+0x6e/0x1d0
> [ 1845.185026] [<ffffffff81063a45>] process_one_work+0x1e5/0x670
> [ 1845.185026] [<ffffffff810639e3>] ? process_one_work+0x183/0x670
> [ 1845.185026] [<ffffffff810642b3>] worker_thread+0x113/0x370
> [ 1845.185026] [<ffffffff810641a0>] ? rescuer_thread+0x290/0x290
> [ 1845.185026] [<ffffffff8106bfba>] kthread+0xda/0xe0
> [ 1845.185026] [<ffffffff814ff0f0>] ? _raw_spin_unlock_irq+0x30/0x60
> [ 1845.185026] [<ffffffff8106bee0>] ? kthread_create_on_node+0x130/0x130
> [ 1845.185026] [<ffffffff8150751a>] ret_from_fork+0x7a/0xb0
> [ 1845.185026] [<ffffffff8106bee0>] ? 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 <f6> 87 00 01 00 00 01 74 1e 48 8d 7b 38 83 6b 38 01 0f 94 c0 84
> [ 1845.185026] RIP [<ffffffff812cda81>] kobject_put+0x11/0x60
> [ 1845.185026] RSP <ffff88007ca5dd08>
> [ 1845.185026] CR2: ffffffffa01601d0
> [ 1845.185026] ---[ end trace 49a70afd109f5653 ]---
>
> Signed-off-by: Li Zhong <[email protected]>
> ---
> 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

2013-08-27 05:48:06

by Rusty Russell

[permalink] [raw]

Subject: Re: [RFC PATCH v2 next]module: Fix mod->mkobj.kobj potentially freed too early

Greg KH <[email protected]> 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 <[email protected]>
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(ffffffffa01600d0 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' (ffff88007c1f1600): kobject_release, parent ffffffffa01600d0 (delayed)
[ 1844.178991] kobject: 'notes' (ffff8800370b2a00): kobject_release, parent ffffffffa01600d0 (delayed)
[ 1845.180118] kobject: 'holders' (ffff88007c1f1600): kobject_cleanup, parent ffffffffa01600d0
[ 1845.182130] kobject: 'holders' (ffff88007c1f1600): auto cleanup kobject_del
[ 1845.184120] BUG: unable to handle kernel paging request at ffffffffa01601d0
[ 1845.185026] IP: [<ffffffff812cda81>] kobject_put+0x11/0x60
[ 1845.185026] PGD 1a13067 PUD 1a14063 PMD 7bd30067 PTE 0
[ 1845.185026] Oops: 0000 [#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: ffff88007ca51f00 ti: ffff88007ca5c000 task.ti: ffff88007ca5c000
[ 1845.185026] RIP: 0010:[<ffffffff812cda81>] [<ffffffff812cda81>] kobject_put+0x11/0x60
[ 1845.185026] RSP: 0018:ffff88007ca5dd08 EFLAGS: 00010282
[ 1845.185026] RAX: 0000000000002000 RBX: ffffffffa01600d0 RCX: ffffffff8177d638
[ 1845.185026] RDX: ffff88007ca5dc18 RSI: 0000000000000000 RDI: ffffffffa01600d0
[ 1845.185026] RBP: ffff88007ca5dd18 R08: ffffffff824e9810 R09: ffffffffffffffff
[ 1845.185026] R10: ffff8800ffffffff R11: dead4ead00000001 R12: ffffffff81a95040
[ 1845.185026] R13: ffff88007b27a960 R14: ffff88007c1f1600 R15: 0000000000000000
[ 1845.185026] FS: 0000000000000000(0000) GS:ffffffff81a23000(0000) knlGS:0000000000000000
[ 1845.185026] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 1845.185026] CR2: ffffffffa01601d0 CR3: 0000000037207000 CR4: 00000000000006b0
[ 1845.185026] Stack:
[ 1845.185026] ffff88007c1f1600 ffff88007c1f1600 ffff88007ca5dd38 ffffffff812cdb7e
[ 1845.185026] 0000000000000000 ffff88007c1f1640 ffff88007ca5dd68 ffffffff812cdbfe
[ 1845.185026] ffff88007c974800 ffff88007c1f1640 ffff88007ff61a00 0000000000000000
[ 1845.185026] Call Trace:
[ 1845.185026] [<ffffffff812cdb7e>] kobject_del+0x2e/0x40
[ 1845.185026] [<ffffffff812cdbfe>] kobject_delayed_cleanup+0x6e/0x1d0
[ 1845.185026] [<ffffffff81063a45>] process_one_work+0x1e5/0x670
[ 1845.185026] [<ffffffff810639e3>] ? process_one_work+0x183/0x670
[ 1845.185026] [<ffffffff810642b3>] worker_thread+0x113/0x370
[ 1845.185026] [<ffffffff810641a0>] ? rescuer_thread+0x290/0x290
[ 1845.185026] [<ffffffff8106bfba>] kthread+0xda/0xe0
[ 1845.185026] [<ffffffff814ff0f0>] ? _raw_spin_unlock_irq+0x30/0x60
[ 1845.185026] [<ffffffff8106bee0>] ? kthread_create_on_node+0x130/0x130
[ 1845.185026] [<ffffffff8150751a>] ret_from_fork+0x7a/0xb0
[ 1845.185026] [<ffffffff8106bee0>] ? 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 <f6> 87 00 01 00 00 01 74 1e 48 8d 7b 38 83 6b 38 01 0f 94 c0 84
[ 1845.185026] RIP [<ffffffff812cda81>] kobject_put+0x11/0x60
[ 1845.185026] RSP <ffff88007ca5dd08>
[ 1845.185026] CR2: ffffffffa01601d0
[ 1845.185026] ---[ end trace 49a70afd109f5653 ]---

Signed-off-by: Li Zhong <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
---
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.c
+++ b/kernel/module.c
@@ -1612,6 +1612,14 @@ static void module_remove_modinfo_attrs(struct module *mod)
kfree(mod->modinfo_attrs);
}

+static void mod_kobject_put(struct module *mod)
+{
+ DECLARE_COMPLETION_ONSTACK(c);
+ mod->mkobj.kobj_completion = &c;
+ kobject_put(&mod->mkobj.kobj);
+ wait_for_completion(&c);
+}
+
static int mod_sysfs_init(struct module *mod)
{
int err;
@@ -1639,7 +1647,7 @@ static int mod_sysfs_init(struct module *mod)
err = kobject_init_and_add(&mod->mkobj.kobj, &module_ktype, NULL,
"%s", mod->name);
if (err)
- kobject_put(&mod->mkobj.kobj);
+ mod_kobject_put(mod);

/* delay uevent until full sysfs population */
out:
@@ -1683,7 +1691,7 @@ out_unreg_param:
out_unreg_holders:
kobject_put(mod->holders_dir);
out_unreg:
- kobject_put(&mod->mkobj.kobj);
+ mod_kobject_put(mod);
out:
return err;
}
@@ -1692,7 +1700,7 @@ static void mod_sysfs_fini(struct module *mod)
{
remove_notes_attrs(mod);
remove_sect_attrs(mod);
- kobject_put(&mod->mkobj.kobj);
+ mod_kobject_put(mod);
}

#else /* !CONFIG_SYSFS */
diff --git a/kernel/params.c b/kernel/params.c
index e5f8f17..501bde4 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -915,7 +915,14 @@ static const struct kset_uevent_ops module_uevent_ops = {
struct kset *module_kset;
int module_sysfs_initialized;

+static void module_kobj_release(struct kobject *kobj)
+{
+ struct module_kobject *mk = to_module_kobject(kobj);
+ complete(mk->kobj_completion);
+}
+
struct kobj_type module_ktype = {
+ .release = module_kobj_release,
.sysfs_ops = &module_sysfs_ops,
};

2013-08-27 06:19:20

by Greg Kroah-Hartman

[permalink] [raw]

Subject: 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 <[email protected]> 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

2013-09-02 00:12:23

by Rusty Russell

[permalink] [raw]

Subject: Re: [RFC PATCH v2 next]module: Fix mod->mkobj.kobj potentially freed too early

Greg KH <[email protected]> writes:
> On Tue, Aug 27, 2013 at 02:08:27PM +0930, Rusty Russell wrote:
>> Greg KH <[email protected]> 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.

2013-09-02 00:43:55

by Greg Kroah-Hartman

[permalink] [raw]

Subject: 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 <[email protected]> writes:
> > On Tue, Aug 27, 2013 at 02:08:27PM +0930, Rusty Russell wrote:
> >> Greg KH <[email protected]> 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 <[email protected]>

if you want it.

thanks,

greg k-h