lkml.org 
[lkml]   [2008]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: kmemcheck caught read from freed memory (cfq_free_io_context)
    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@linux.vnet.ibm.com> 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);
    }


    \
     
     \ /
      Last update: 2008-04-02 20:27    [W:2.762 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site