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

From: Alan Stern
Date: Sat Feb 28 2004 - 12:12:50 EST


On Sat, 28 Feb 2004, Michael Frank wrote:

> On Fri, 27 Feb 2004 23:02:34 -0500 (EST), Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/