Re: kmemcheck caught read from freed memory (cfq_free_io_context)

From: Paul E. McKenney
Date: Wed Apr 02 2008 - 14:24:18 EST


On Wed, Apr 02, 2008 at 07:32:26PM +0300, Pekka J Enberg wrote:
> Hi Vegard,
>
> On Wed, Apr 2, 2008 at 6:08 PM, Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx> wrote:
> > > Would the following be an appropriate fix? It seems to me to be in
> > > the same spirit as the existing check for s->ctor.
>
> On Wed, 2 Apr 2008, Vegard Nossum wrote:
> > In my opinion, no.
> >
> > It would fix the false positives, but would in fact also hide cases
> > such as this one with cfq, e.g. the real cases of mis-use.
>
> Yes, but we might as well put Paul's patch in now and remove that later
> when we have a proper fix, no?
>
> On Wed, 2 Apr 2008, Vegard Nossum wrote:
> > Peter Zijlstra suggested this:
> > > It would have to register an call_rcu callback itself in order to mark
> > > it freed - and handle the race with the object being handed out again.
> >
> > I will try to look into this -- for now, I need to understand RCU
> > first (I've seen your LWN articles -- great work! :-))
>
> Well, maybe we can add two new states: RCU_FREED and RCU_VALIDATED? The
> object is flagged with the first one as soon as an object is handed over
> to kmem_cache_free() and the latter needs to hook to the validation phase
> of RCU (how is that done btw?). Then kmemcheck could even give a better
> error message: "RCU-freed object used without validation."
>
> And with delayed free for kmemcheck we discussed before, we'd hold on to
> the objects long enough to actually see these error conditions.

Well, one approach would be to add an rcu_head to the kmem_cache
structure, along with a flag stating that the rcu_head is in use. I hope
that there is a better approach, as this introduces a lock roundtrip
into kmemcheck_slab_free(). Is there a better place to put the rcu_head?
Perhaps into the per-CPU allocator? But then we have to track which
CPU has which mark pending, and there are only so many bits in a byte,
as the SGI guys would be quick to point out

Which is why I chickened out and submitted the earlier crude patch.

Anyway, here is a -very- rough sketch of the stupid lock-based approach.

Thanx, Paul

struct kmem_cache {

. . . /* existing fields */

struct rcu_head rcu;
int rcu_available; /* rcu_head above is available for use. */
spinlock_t rcu_lock; /* which of course must be initialized. */
};

Then we need to add a couple of values to the enum shadow:

enum shadow {
... /* existing values */
SHADOW_RCU_FREED,
SHADOW_RCU_FREED_PENDING,
};

Then we have:

void
kmemcheck_slab_free(struct kmem_cache *s, void *object)
{
unsigned long flags;

if (s->ctor)
return;
if (likely(!(s->flags & SLAB_DESTROY_BY_RCU)))
kmemcheck_mark_freed(object, s->objsize);
spin_lock_irqsave(&s->rcu_lock, flags);
if (s->rcu_available) {
kmemcheck_mark_rcu_freed(object, s->objsize);
/* record the address somewhere... */
call_rcu(&s->rcu, kmemcheck_slab_free_rcu);
} else {
kmemcheck_mark_rcu_pending(object, s->objsize);
/* record the address somewhere... */
}
spin_unlock_irqrestore(&s->rcu_lock, flags);
}

void kmemcheck_slab_free_rcu(struct rcu_head *rcu)
{
unsigned long flags;
struct kmem_cache *s = container_of(rcu, struct kmem_cache, rcu);
void *shadow;

spin_lock_irqsave(&s->rcu_lock, flags);
/* recover the previously recorded object address. somehow */
kmemcheck_mark_freed(object, s->objsize);
if (/* there are pending requests */) {
/* get the previously recorded object addresses, somehow */
kmemcheck_mark_rcu_freed(object, s->objsize);
call_rcu(&s->rcu, kmemcheck_slab_free_rcu);
}
spin_unlock_irqrestore(&s->rcu_lock, flags);
}
--
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/