Re: Question about (or bug in?) the kobject implementation

From: Alan Stern (stern_at_rowland.harvard.edu)
Date: 02/28/04

  • Next message: Maurice van der Stee: "Re: 2.6.4-rc1 oops on HPFS filesystem file rename" 
    Date:	Sat, 28 Feb 2004 12:09:00 -0500 (EST)
To: Michael Frank <mhf@linuxmail.org>

    On Sat, 28 Feb 2004, Michael Frank wrote:

    > On Fri, 27 Feb 2004 23:02:34 -0500 (EST), Alan Stern <stern@rowland.harvard.edu> wrote:
    > >
    > > This really is a programming error. It means that kobject_get() has been
    > > passed a possibly stale pointer. Ipso facto, the call to kobject_put()
    > > that decremented the refcount to 0 was made too early, while there were
    > > still active pointers to the kobject floating around.
    > >
    > > It's impossible to prevent people from making programming errors or
    > > dereferencing stale pointers. It doesn't matter _what_ code you put in
    > > kobject_get() -- it will crash when given a pointer to a kobject whose
    > > cleanup routine has already run and deallocated the storage.
    > >
    > > The best you can do is call people's attention to such errors and fail the
    > > operation gracefully whenever possible (i.e., when it doesn't generate an
    > > addressing error). My personal choice would be to change kobject_get() as
    > > follows:
    > >
    > > struct kobject * kobject_get(struct kobject * kobj)
    > > {
    > > if (kobj) {
    > > if (atomic_read(&kobj->refcount) == 0) {
    > > WARN_ON(1);
    > > return NULL;
    > > }
    > > atomic_inc(&kobj->refcount);
    > > }
    > > return kobj;
    > > }
    > >
    > > I think that's about the best you can do.
    >
    > This is too ugly :-(

    It's cleaner than your proposal below. It's not so different from the
    code that's there now. And it does what that code _ought_ to do, namely,
    return a NULL pointer when the kobject is no longer available.

    > > And what's the answer to A'?
    >
    > The weakness is really in that the refcount is stored dynamically.
    >
    > What about a new struct to hold the pointer to the kobj and it's refcount:
    >
    > struct kobjectref {
    > struct kobject *kobj;
    > int refcount;
    > };
    > ...
    >
    > struct kobjectref rkobj;

    Since kobjects are allocated dynamically, you will have to allocate
    kobjectrefs dynamically as well.

    > Using refkobj eliminates all problems as the pointer to the refcount can't
    > be invalid.

    Only until you deallocate the kobjectref. And when you do, you then face
    exactly the same set of problems: pointers to the kobjectref will become
    stale. If you don't ever deallocate kobjectrefs then you have a memory
    leak. So this proposal doesn't solve anything, it just adds an extra
    layer of indirection.

    Alan Stern

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  • Next message: Maurice van der Stee: "Re: 2.6.4-rc1 oops on HPFS filesystem file rename"