* oopses in kobjects in 2.6.0-test11 (was Re: kobject patch)
[not found] ` <20031208222526.GA31134@kroah.com>
@ 2003-12-08 22:48 ` Greg KH
2003-12-08 22:58 ` Greg KH
2003-12-28 12:38 ` Andrey Borzenkov
0 siblings, 2 replies; 6+ messages in thread
From: Greg KH @ 2003-12-08 22:48 UTC (permalink / raw)
To: Andrew Morton, maneesh, mgorse, linux-kernel
Cc: Andrey Borzenkov, Patrick Mochel
Ok, I'm ccing lkml and everyone else who has been in on this thread at
different times. This is based on a patch from Andrey that was/is in
the -mm tree for a while.
On Mon, Dec 08, 2003 at 02:25:26PM -0800, Greg KH wrote:
> On Thu, Oct 09, 2003 at 01:48:37AM -0700, Andrew Morton wrote:
> >
> > I've had this in -mm for a while. What to do with it?
>
> Heh, nothing like digging up something from the past that I insisted was
> not needed, but...
>
> > It is possible that parent is removed before child when child is in use.
> > Trivial example is mounted USB storage when you unplug it. The kobject for
> > USB device is removed but subordinate SCSI device remains. Then kernel oopses
> > on attempt to release child e.g. umount removed USB storage. This patch fixes
> > two problems:
>
> Yes, I now think this patch needs to be applied. I can easily cause a
> parent device in sysfs to go away, with the child still present:
> - plug in a usb-serial device
> - run 'cat /dev/ttyUSB0'
> - yank the device out.
> Now if you cancel the 'cat' program, lovely oopses...
>
> So, Andrew, very sorry about this, but this patch should be sent to
> Linus. I think Pat agrees with me, but he's on the road for a few days.
> You might want to wait for him.
Hm, wait, Pat objected to the patch to kobject.c (now that I went back
and read the whole thread.) And I agree with him, but I'm now getting
an oops in get_kobj_path_length if I do the above while loading down the
machine with other tasks when I cancel 'cat'.
So something else bad is happening here...
> > - kset_hotplug. It oopses in get_kobj_path_length because child->parent
> > points to nowhere - even if parent has not yet been overwritten, its name
> > is already freed.
> >
> > The patch moves kobject_put for parent from unlink() into
> > kobject_cleanup for child making sure reference to parents exists for as
> > long as child is there and may use it.
But you can't do this, as you need that kobject_put() in unlink() for
when it is called from kobject_add().
Hm, wait... I think we are close...
Ok, here's how a parent can be removed from the system without the child
going away:
- create parent and register it successfully.
- create child, call kobject_add() which increments the count of
the parent.
- call kobject_get() on the child.
- call kobject_del() on the parent. This will keep the parent
around, as the child still has a reference on it.
- call kobject_del() on the child. This will decrement the
count on the parent due to the call in unlink(). That will
free the parent up from memory. But this child still has a
incremented count (rightly, as it is in use).
- So the child now has a stale parent pointer, causing all sorts
of fun...
I'll work on a patch for kobject.c and post it in the next message, and
include the original message and patch below for others to see.
thanks,
greg k-h
> > - after this oops has been fixed I got next one now in sysfs. The
> > problem is sysfs_remove_dir would unlink all children including
> > directories for subordinate kobjects. Resulting in dget/dput mismatch.
> > I usually got oops due to the fact that d_delete in remove_dir would free
> > inode and then simple_rmdir would try to access it.
> >
> > The patch avoids calling extra d_delete/unlink on already-deleted
> > dentry. I hate this patch but anything better apparently requires
> > complete redesign of sysfs implementation. Unlinking busy directory is
> > otherwise impossible and I am afraid it will show itself somewhere else.
> >
> >
> >
> > 25-akpm/fs/sysfs/dir.c | 12 ++++++++++--
> > 25-akpm/lib/kobject.c | 4 ++--
> > 2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff -puN fs/sysfs/dir.c~kobject-oops-fixes fs/sysfs/dir.c
> > --- 25/fs/sysfs/dir.c~kobject-oops-fixes Thu Oct 9 01:46:51 2003
> > +++ 25-akpm/fs/sysfs/dir.c Thu Oct 9 01:46:51 2003
> > @@ -82,8 +82,16 @@ static void remove_dir(struct dentry * d
> > {
> > struct dentry * parent = dget(d->d_parent);
> > down(&parent->d_inode->i_sem);
> > - d_delete(d);
> > - simple_rmdir(parent->d_inode,d);
> > + /*
> > + * It is possible that parent has already been removed, in which
> > + * case directory is already unhashed and dput.
> > + * Note that this won't update parent->d_inode->i_nlink; OTOH
> > + * parent should already be dead
> > + */
> > + if (!d_unhashed(d)) {
> > + d_delete(d);
> > + simple_rmdir(parent->d_inode,d);
> > + }
> >
> > pr_debug(" o %s removing done (%d)\n",d->d_name.name,
> > atomic_read(&d->d_count));
> > diff -puN lib/kobject.c~kobject-oops-fixes lib/kobject.c
> > --- 25/lib/kobject.c~kobject-oops-fixes Thu Oct 9 01:46:51 2003
> > +++ 25-akpm/lib/kobject.c Thu Oct 9 01:46:51 2003
> > @@ -236,8 +236,6 @@ static void unlink(struct kobject * kobj
> > list_del_init(&kobj->entry);
> > up_write(&kobj->kset->subsys->rwsem);
> > }
> > - if (kobj->parent)
> > - kobject_put(kobj->parent);
> > kobject_put(kobj);
> > }
> >
> > @@ -457,6 +455,8 @@ void kobject_cleanup(struct kobject * ko
> > if (kobj->k_name != kobj->name)
> > kfree(kobj->k_name);
> > kobj->k_name = NULL;
> > + if (kobj->parent)
> > + kobject_put(kobj->parent);
> > if (t && t->release)
> > t->release(kobj);
> > if (s)
> >
> > _
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: oopses in kobjects in 2.6.0-test11 (was Re: kobject patch)
2003-12-08 22:48 ` oopses in kobjects in 2.6.0-test11 (was Re: kobject patch) Greg KH
@ 2003-12-08 22:58 ` Greg KH
2003-12-11 16:51 ` Patrick Mochel
2003-12-28 12:38 ` Andrey Borzenkov
1 sibling, 1 reply; 6+ messages in thread
From: Greg KH @ 2003-12-08 22:58 UTC (permalink / raw)
To: Andrew Morton, maneesh, mgorse, linux-kernel, Andrey Borzenkov,
Patrick Mochel
On Mon, Dec 08, 2003 at 02:48:10PM -0800, Greg KH wrote:
>
> Ok, here's how a parent can be removed from the system without the child
> going away:
> - create parent and register it successfully.
> - create child, call kobject_add() which increments the count of
> the parent.
> - call kobject_get() on the child.
> - call kobject_del() on the parent. This will keep the parent
> around, as the child still has a reference on it.
> - call kobject_del() on the child. This will decrement the
> count on the parent due to the call in unlink(). That will
> free the parent up from memory. But this child still has a
> incremented count (rightly, as it is in use).
>
> - So the child now has a stale parent pointer, causing all sorts
> of fun...
>
> I'll work on a patch for kobject.c and post it in the next message, and
> include the original message and patch below for others to see.
Here's a patch for kobject.c that should fix this problem and keep
kobject parent's around until after the child is gone. Please can
someone verify that I didn't get this wrong...
thanks,
greg k-h
--- a/lib/kobject.c Mon Sep 29 15:13:44 2003
+++ b/lib/kobject.c Mon Dec 8 14:56:32 2003
@@ -236,8 +236,6 @@
list_del_init(&kobj->entry);
up_write(&kobj->kset->subsys->rwsem);
}
- if (kobj->parent)
- kobject_put(kobj->parent);
kobject_put(kobj);
}
@@ -274,9 +272,11 @@
kobj->parent = parent;
error = create_dir(kobj);
- if (error)
+ if (error) {
unlink(kobj);
- else {
+ if (parent)
+ kobject_put(parent);
+ } else {
/* If this kobj does not belong to a kset,
try to find a parent that does. */
top_kobj = kobj;
@@ -452,6 +452,7 @@
{
struct kobj_type * t = get_ktype(kobj);
struct kset * s = kobj->kset;
+ struct kobject * parent = kobj->parent;
pr_debug("kobject %s: cleaning up\n",kobject_name(kobj));
if (kobj->k_name != kobj->name)
@@ -461,6 +462,8 @@
t->release(kobj);
if (s)
kset_put(s);
+ if (parent)
+ kobject_put(parent);
}
/**
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: oopses in kobjects in 2.6.0-test11 (was Re: kobject patch)
2003-12-08 22:58 ` Greg KH
@ 2003-12-11 16:51 ` Patrick Mochel
2003-12-11 17:50 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Patrick Mochel @ 2003-12-11 16:51 UTC (permalink / raw)
To: Greg KH; +Cc: Andrew Morton, maneesh, mgorse, linux-kernel, Andrey Borzenkov
Sorry about the delay in getting back to you.
> Here's a patch for kobject.c that should fix this problem and keep
> kobject parent's around until after the child is gone. Please can
> someone verify that I didn't get this wrong...
The patch looks good, please forward it on to Linus/Andrew.
Thanks,
Pat
P.S. I've left OSDL to go work for a startup. Please use this email
address from now on.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: oopses in kobjects in 2.6.0-test11 (was Re: kobject patch)
2003-12-11 16:51 ` Patrick Mochel
@ 2003-12-11 17:50 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2003-12-11 17:50 UTC (permalink / raw)
To: Patrick Mochel
Cc: Andrew Morton, maneesh, mgorse, linux-kernel, Andrey Borzenkov
On Thu, Dec 11, 2003 at 08:51:58AM -0800, Patrick Mochel wrote:
>
> Sorry about the delay in getting back to you.
>
> > Here's a patch for kobject.c that should fix this problem and keep
> > kobject parent's around until after the child is gone. Please can
> > someone verify that I didn't get this wrong...
>
> The patch looks good, please forward it on to Linus/Andrew.
Will do, thanks for looking it over.
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: oopses in kobjects in 2.6.0-test11 (was Re: kobject patch)
2003-12-08 22:48 ` oopses in kobjects in 2.6.0-test11 (was Re: kobject patch) Greg KH
2003-12-08 22:58 ` Greg KH
@ 2003-12-28 12:38 ` Andrey Borzenkov
2003-12-30 0:32 ` Greg KH
1 sibling, 1 reply; 6+ messages in thread
From: Andrey Borzenkov @ 2003-12-28 12:38 UTC (permalink / raw)
To: Greg KH, Andrew Morton, maneesh, mgorse, linux-kernel; +Cc: Patrick Mochel
On Tuesday 09 December 2003 01:48, Greg KH wrote:
> Ok, I'm ccing lkml and everyone else who has been in on this thread at
> different times. This is based on a patch from Andrey that was/is in
> the -mm tree for a while.
>
what about second part in sysfs/dir.c? How relevant is it?
-andrey
>
> > > - after this oops has been fixed I got next one now in sysfs. The
> > > problem is sysfs_remove_dir would unlink all children including
> > > directories for subordinate kobjects. Resulting in dget/dput
> > > mismatch. I usually got oops due to the fact that d_delete in
> > > remove_dir would free inode and then simple_rmdir would try to access
> > > it.
> > >
> > > The patch avoids calling extra d_delete/unlink on already-deleted
> > > dentry. I hate this patch but anything better apparently requires
> > > complete redesign of sysfs implementation. Unlinking busy directory
> > > is otherwise impossible and I am afraid it will show itself somewhere
> > > else.
> > >
> > >
> > >
> > > 25-akpm/fs/sysfs/dir.c | 12 ++++++++++--
> > > 25-akpm/lib/kobject.c | 4 ++--
> > > 2 files changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff -puN fs/sysfs/dir.c~kobject-oops-fixes fs/sysfs/dir.c
> > > --- 25/fs/sysfs/dir.c~kobject-oops-fixes Thu Oct 9 01:46:51 2003
> > > +++ 25-akpm/fs/sysfs/dir.c Thu Oct 9 01:46:51 2003
> > > @@ -82,8 +82,16 @@ static void remove_dir(struct dentry * d
> > > {
> > > struct dentry * parent = dget(d->d_parent);
> > > down(&parent->d_inode->i_sem);
> > > - d_delete(d);
> > > - simple_rmdir(parent->d_inode,d);
> > > + /*
> > > + * It is possible that parent has already been removed, in which
> > > + * case directory is already unhashed and dput.
> > > + * Note that this won't update parent->d_inode->i_nlink; OTOH
> > > + * parent should already be dead
> > > + */
> > > + if (!d_unhashed(d)) {
> > > + d_delete(d);
> > > + simple_rmdir(parent->d_inode,d);
> > > + }
> > >
> > > pr_debug(" o %s removing done (%d)\n",d->d_name.name,
> > > atomic_read(&d->d_count));
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: oopses in kobjects in 2.6.0-test11 (was Re: kobject patch)
2003-12-28 12:38 ` Andrey Borzenkov
@ 2003-12-30 0:32 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2003-12-30 0:32 UTC (permalink / raw)
To: Andrey Borzenkov
Cc: Andrew Morton, maneesh, mgorse, linux-kernel, Patrick Mochel
On Sun, Dec 28, 2003 at 03:38:42PM +0300, Andrey Borzenkov wrote:
> On Tuesday 09 December 2003 01:48, Greg KH wrote:
> > Ok, I'm ccing lkml and everyone else who has been in on this thread at
> > different times. This is based on a patch from Andrey that was/is in
> > the -mm tree for a while.
> >
>
> what about second part in sysfs/dir.c? How relevant is it?
Very relevant, that's why it's in the -mm tree right now :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2003-12-30 0:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20031009014837.4ff71634.akpm@osdl.org>
[not found] ` <20031208222526.GA31134@kroah.com>
2003-12-08 22:48 ` oopses in kobjects in 2.6.0-test11 (was Re: kobject patch) Greg KH
2003-12-08 22:58 ` Greg KH
2003-12-11 16:51 ` Patrick Mochel
2003-12-11 17:50 ` Greg KH
2003-12-28 12:38 ` Andrey Borzenkov
2003-12-30 0:32 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).