lkml.org 
[lkml]   [2008]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: kmemcheck caught read from freed memory (cfq_free_io_context)
    > From: Peter Zijlstra <a.p.zijlstra@chello.nl>
    > Date: Wed, Apr 02, 2008 12:59:21PM +0200
    >
    > On Wed, 2008-04-02 at 03:55 -0700, Paul E. McKenney wrote:
    > > On Wed, Apr 02, 2008 at 09:28:46AM +0200, Ingo Molnar wrote:
    > > >
    > > > * Jens Axboe <jens.axboe@oracle.com> wrote:
    > > >
    > > > > On Wed, Apr 02 2008, Pekka J Enberg wrote:
    > > > > > On Wed, 2 Apr 2008, Jens Axboe wrote:
    > > > > > > Good catch, I wonder why it didn't complain in my testing. I've added a
    > > > > > > patch to fix that, please see it here:
    > > > > >
    > > > > > You probably don't have kmemcheck in your kernel ;-)
    > > > >
    > > > > Ehm no, you are right :)
    > > >
    > > > ... and you can get kmemcheck by testing on x86.git/latest:
    > > >
    > > > http://people.redhat.com/mingo/x86.git/README
    > > >
    > > > ;-)
    > >
    > > I will check this when I get back to some bandwidth -- but in the meantime,
    > > does kmemcheck special-case SLAB_DESTROY_BY_RCU? It is legal to access
    > > newly-freed items in that case, as long as you did rcu_read_lock()
    > > before gaining a reference to them and don't hold the reference past
    > > the matching rcu_read_unlock().
    >
    > I don't think it does.
    >
    > 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 had the same problem while debugging a cfq-derived i/o scheduler,
    and I found nothing preventing the reuse of the freed memory.
    The patch below seemed to fix the logic.

    Signed-off-by: Fabio Checconi <fabio@gandalf.sssup.it>
    ---
    diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
    index 0f962ec..f26da2b 100644
    --- a/block/cfq-iosched.c
    +++ b/block/cfq-iosched.c
    @@ -1143,24 +1143,37 @@ static void cfq_put_queue(struct cfq_queue *cfqq)
    }

    /*
    - * Call func for each cic attached to this ioc. Returns number of cic's seen.
    + * Call func for each cic attached to this ioc.
    */
    -static unsigned int
    +static void
    call_for_each_cic(struct io_context *ioc,
    void (*func)(struct io_context *, struct cfq_io_context *))
    {
    struct cfq_io_context *cic;
    struct hlist_node *n;
    - int called = 0;

    rcu_read_lock();
    - hlist_for_each_entry_rcu(cic, n, &ioc->cic_list, cic_list) {
    + hlist_for_each_entry_rcu(cic, n, &ioc->cic_list, cic_list)
    func(ioc, cic);
    - called++;
    - }
    rcu_read_unlock();
    +}
    +
    +static void cfq_cic_free_rcu(struct rcu_head *head)
    +{
    + struct cfq_io_context *cic;
    +
    + cic = container_of(head, struct cfq_io_context, rcu_head);
    +
    + kmem_cache_free(cfq_ioc_pool, cic);
    + elv_ioc_count_dec(ioc_count);
    +
    + if (ioc_gone && !elv_ioc_count_read(ioc_count))
    + complete(ioc_gone);
    +}

    - return called;
    +static void cfq_cic_free(struct cfq_io_context *cic)
    +{
    + call_rcu(&cic->rcu_head, cfq_cic_free_rcu);
    }

    static void cic_free_func(struct io_context *ioc, struct cfq_io_context *cic)
    @@ -1174,24 +1187,18 @@ static void cic_free_func(struct io_context *ioc, struct cfq_io_context *cic)
    hlist_del_rcu(&cic->cic_list);
    spin_unlock_irqrestore(&ioc->lock, flags);

    - kmem_cache_free(cfq_ioc_pool, cic);
    + cfq_cic_free(cic);
    }

    static void cfq_free_io_context(struct io_context *ioc)
    {
    - int freed;
    -
    /*
    - * ioc->refcount is zero here, so no more cic's are allowed to be
    - * linked into this ioc. So it should be ok to iterate over the known
    - * list, we will see all cic's since no new ones are added.
    + * ioc->refcount is zero here, or we are called from elv_unregister(),
    + * so no more cic's are allowed to be linked into this ioc. So it
    + * should be ok to iterate over the known list, we will see all cic's
    + * since no new ones are added.
    */
    - freed = call_for_each_cic(ioc, cic_free_func);
    -
    - elv_ioc_count_mod(ioc_count, -freed);
    -
    - if (ioc_gone && !elv_ioc_count_read(ioc_count))
    - complete(ioc_gone);
    + call_for_each_cic(ioc, cic_free_func);
    }

    static void cfq_exit_cfqq(struct cfq_data *cfqd, struct cfq_queue *cfqq)
    @@ -1458,15 +1465,6 @@ cfq_get_queue(struct cfq_data *cfqd, int is_sync, struct io_context *ioc,
    return cfqq;
    }

    -static void cfq_cic_free(struct cfq_io_context *cic)
    -{
    - kmem_cache_free(cfq_ioc_pool, cic);
    - elv_ioc_count_dec(ioc_count);
    -
    - if (ioc_gone && !elv_ioc_count_read(ioc_count))
    - complete(ioc_gone);
    -}
    -
    /*
    * We drop cfq io contexts lazily, so we may find a dead one.
    */
    @@ -2138,7 +2136,7 @@ static int __init cfq_slab_setup(void)
    if (!cfq_pool)
    goto fail;

    - cfq_ioc_pool = KMEM_CACHE(cfq_io_context, SLAB_DESTROY_BY_RCU);
    + cfq_ioc_pool = KMEM_CACHE(cfq_io_context, 0);
    if (!cfq_ioc_pool)
    goto fail;

    @@ -2286,7 +2284,6 @@ static void __exit cfq_exit(void)
    smp_wmb();
    if (elv_ioc_count_read(ioc_count))
    wait_for_completion(ioc_gone);
    - synchronize_rcu();
    cfq_slab_kill();
    }

    diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
    index 1b4ccf2..50e448c 100644
    --- a/include/linux/iocontext.h
    +++ b/include/linux/iocontext.h
    @@ -54,6 +54,8 @@ struct cfq_io_context {

    void (*dtor)(struct io_context *); /* destructor */
    void (*exit)(struct io_context *); /* called on task exit */
    +
    + struct rcu_head rcu_head;
    };

    /*

    \
     
     \ /
      Last update: 2008-04-02 13:35    [W:0.031 / U:0.184 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site