Messages in this thread Patch in this message | | | Date | Wed, 2 Apr 2008 13:33:27 +0200 | From | Fabio Checconi <> | Subject | Re: 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; }; /*
| |