Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)

[Greg KH <greg@kroah.com>]

On Mon, Oct 29, 2007 at 08:13:17PM +0100, Kay Sievers wrote:
> On Mon, 2007-10-29 at 14:47 -0400, Alan Stern wrote:
> > On Mon, 29 Oct 2007, James Bottomley wrote:
> > 
> > > I'm not sure if we can do this patch.  If you kill a device, you see
> > > there's a processing state in scsi_prep_fn() for SDEV_DEL, which has a
> > > printk message I see quite often when I unplug devices while they're
> > > operating.
> > > 
> > > If you NULL out and free the request queue immediately after setting
> > > SDEV_DEL, without allowing the devices and filesystems to finish, are
> > > you sure we're not going to get a null deref on sdev->request_queue?
> > 
> > Let's get at this another way.  The patch below probably should be 
> > applied in any case.  It's essentially a reversion of this patch from 
> > 2003:
> > 
> > http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commit;h=10921a8f1305b8ec97794941db78b825db5839bc
> > 
> > which was written to compensate for problems in the SCSI stack.  The
> > idea was to avoid releasing a kobject before all its children were
> > released.  However as far as I can see, in general we should be able to
> > release a kobject as soon as all its children are removed and its
> > refcount drops to 0, without waiting for the children to be released.  
> > To put it another way, once a child is removed from visibility it
> > should not try (or need) to access its parent at all.  If it has to
> > then it should take an explicit reference to the parent.
> > 
> > Since the SCSI stack is now in much better shape, there doesn't seem to 
> > be any reason to keep the old code.  Do you agree that the patch below 
> > is worth merging?  I submitted it to Greg some time ago, but he didn't 
> > want to accept it without some assurance that it is now safe.
> > 
> > It would fix the problem Kay and saw with the circular references, 
> > because the references would all get dropped when the scsi_device is 
> > removed instead of waiting for a release that will never come.
> 
> It indeed fixes my problem. And it sounds you are right, the "fix" from
> 2003 is probably just a paper-over for a missing explicit reference that
> time.
> 
> I'm all for giving the reversion a try, and add explicit parent get/put
> if needed somewhere. If, for some reason, that will not happen, I'll
> need to do something like this in the block patch, which will then be a
> "fix for the paper-over solution for an unknown bug". :)
> 
> 
> --- a/fs/partitions/check.c
> +++ b/fs/partitions/check.c
> ...
> +       device_del(&disk->dev);
> +
> +       /*
> +        * disconnect from parent device, so the parent can clean up
> +        * without waiting for us to clean up
> +        *
> +        * the driver core took this reference while we added ourself as
> +        * a child of the parent device
> +        */
> +       parent = disk->dev.kobj.parent;

Save off the parent before calling device_del() otherwise it could be a
stale pointer, right?

> +       disk->dev.kobj.parent = NULL;
> +       disk->dev.parent = NULL;
> +       kobject_put(parent);

No, that's just wrong, if this is needed, something else really is
broken, and I don't think it's the driver core...

thanks,

greg k-h