kmemcheck caught read from freed memory (cfq_free_io_context) - Kernel

This is a discussion on kmemcheck caught read from freed memory (cfq_free_io_context) - Kernel ; On Wed, Apr 02 2008, Fabio Checconi wrote: > > From: Jens Axboe > > Date: Wed, Apr 02, 2008 02:36:39PM +0200 > > > > > Looks good and tests fine as well. I've applied it, on top of ...

+ Reply to Thread
Page 3 of 4 FirstFirst 1 2 3 4 LastLast
Results 41 to 60 of 68

Thread: kmemcheck caught read from freed memory (cfq_free_io_context)

  1. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02 2008, Fabio Checconi wrote:
    > > From: Jens Axboe
    > > Date: Wed, Apr 02, 2008 02:36:39PM +0200
    > >
    > > > Looks good and tests fine as well. I've applied it, on top of the
    > > > hlist_for_each_entry_safe_rcu() fix.
    > > >
    > > > http://git.kernel.dk/?p=linux-2.6-bl...6312545f126661
    > > >

    >
    > ok, thanks. anyway I don't think the hlist_for_each_entry_safe_rcu()
    > is needed at this point, since the pos->next pointer is still valid
    > (at least) until the next rcu_read_unlock(). am I wrong?


    it isn't, but it's still clearer. so either that or a nice comment, I
    just stuck with what I had already committed.

    --
    Jens Axboe

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02 2008, Jens Axboe wrote:
    > On Wed, Apr 02 2008, Fabio Checconi wrote:
    > > > From: Jens Axboe
    > > > Date: Wed, Apr 02, 2008 02:36:39PM +0200
    > > >
    > > > > Looks good and tests fine as well. I've applied it, on top of the
    > > > > hlist_for_each_entry_safe_rcu() fix.
    > > > >
    > > > > http://git.kernel.dk/?p=linux-2.6-bl...6312545f126661
    > > > >

    > >
    > > ok, thanks. anyway I don't think the hlist_for_each_entry_safe_rcu()
    > > is needed at this point, since the pos->next pointer is still valid
    > > (at least) until the next rcu_read_unlock(). am I wrong?

    >
    > it isn't, but it's still clearer. so either that or a nice comment, I
    > just stuck with what I had already committed.


    Christ, mutt is still dropping you. Sorry about that.

    --
    Jens Axboe

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    > From: Jens Axboe
    > Date: Wed, Apr 02, 2008 02:36:39PM +0200
    >
    > > Looks good and tests fine as well. I've applied it, on top of the
    > > hlist_for_each_entry_safe_rcu() fix.
    > >
    > > http://git.kernel.dk/?p=linux-2.6-bl...6312545f126661
    > >


    ok, thanks. anyway I don't think the hlist_for_each_entry_safe_rcu()
    is needed at this point, since the pos->next pointer is still valid
    (at least) until the next rcu_read_unlock(). am I wrong?
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02, 2008 at 02:13:42PM +0200, Peter Zijlstra wrote:
    > On Wed, 2008-04-02 at 13:53 +0200, Jens Axboe wrote:
    >
    > > > Yeah, SLAB_DESTROY_BY_RCU should have a _HUGE_ comment explaining it,
    > > > I'm sure this is not the first (nor the last) time people get that
    > > > wrong.

    > >
    > > It should, SLAB_DESTROY_BY_RCU is definitely useful, but it is expected
    > > to be an 'easier' way of doing the call_rcu() manually. So it definitely
    > > needs more documentation.
    > >

    >
    > Ok I gave it a go, how bad is this text?


    Looks good to me!

    Acked-by: Paul E. McKenney

    > Signed-off-by: Peter Zijlstra
    > ---
    > diff --git a/include/linux/slab.h b/include/linux/slab.h
    > index f950a89..e049ddc 100644
    > --- a/include/linux/slab.h
    > +++ b/include/linux/slab.h
    > @@ -25,6 +25,32 @@
    > #define SLAB_CACHE_DMA 0x00004000UL /* Use GFP_DMA memory */
    > #define SLAB_STORE_USER 0x00010000UL /* DEBUG: Store the last owner for bug hunting */
    > #define SLAB_PANIC 0x00040000UL /* Panic if kmem_cache_create() fails */
    > +/*
    > + * SLAB_DESTROY_BY_RCU - **WARNING** READ THIS!
    > + *
    > + * This delays freeing the SLAB page by a grace period, it does _NOT_
    > + * delay object freeing. This means that if you do kmem_cache_free()
    > + * that memory location is free to be reused at any time. Thus it may
    > + * be possible to see another object there in the same RCU grace period.
    > + *
    > + * This feature only ensures the memory location backing the object
    > + * stays valid, the trick to using this is relying on an independent
    > + * object validation pass. Something like:
    > + *
    > + * rcu_read_lock()
    > + * again:
    > + * obj = lockless_lookup(key);
    > + * if (obj) {
    > + * if (!try_get_ref(obj)) // might fail for free objects
    > + * goto again;
    > + *
    > + * if (obj->key != key) { // not the object we expected
    > + * put_ref(obj);
    > + * goto again;
    > + * }
    > + * }
    > + * rcu_read_unlock();
    > + */
    > #define SLAB_DESTROY_BY_RCU 0x00080000UL /* Defer freeing slabs to RCU */
    > #define SLAB_MEM_SPREAD 0x00100000UL /* Spread some memory over cpuset */
    > #define SLAB_TRACE 0x00200000UL /* Trace allocations and frees */
    >
    >

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02, 2008 at 01:33:27PM +0200, Fabio Checconi wrote:
    > > From: Peter Zijlstra
    > > 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 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.


    Looks good to me from a strictly RCU viewpoint -- I must confess great
    ignorance of the CFQ code. :-/

    Acked-by: Paul E. McKenney

    > Signed-off-by: Fabio Checconi
    > ---
    > 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;
    > };
    >
    > /*

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02, 2008 at 02:58:19PM +0200, Jens Axboe wrote:
    > On Wed, Apr 02 2008, Fabio Checconi wrote:
    > > > From: Jens Axboe
    > > > Date: Wed, Apr 02, 2008 02:36:39PM +0200
    > > >
    > > > > Looks good and tests fine as well. I've applied it, on top of the
    > > > > hlist_for_each_entry_safe_rcu() fix.
    > > > >
    > > > > http://git.kernel.dk/?p=linux-2.6-bl...6312545f126661
    > > > >

    > >
    > > ok, thanks. anyway I don't think the hlist_for_each_entry_safe_rcu()
    > > is needed at this point, since the pos->next pointer is still valid
    > > (at least) until the next rcu_read_unlock(). am I wrong?

    >
    > it isn't, but it's still clearer. so either that or a nice comment, I
    > just stuck with what I had already committed.


    Given Peter's comment, would you be willing to drop the
    hlist_for_each_entry_safe_rcu()? People tend to read too much into
    the "safe" sometimes. ;-)

    Thanx, Paul
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02 2008, Paul E. McKenney wrote:
    > On Wed, Apr 02, 2008 at 01:33:27PM +0200, Fabio Checconi wrote:
    > > > From: Peter Zijlstra
    > > > 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 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.

    >
    > Looks good to me from a strictly RCU viewpoint -- I must confess great
    > ignorance of the CFQ code. :-/


    That can always be rectified, given enough spare time :-)

    > Acked-by: Paul E. McKenney


    Thanks, added.

    --
    Jens Axboe

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    > From: Jens Axboe
    > Date: Wed, Apr 02, 2008 02:58:58PM +0200
    >
    > On Wed, Apr 02 2008, Jens Axboe wrote:
    > > On Wed, Apr 02 2008, Fabio Checconi wrote:
    > > > > From: Jens Axboe
    > > > > Date: Wed, Apr 02, 2008 02:36:39PM +0200
    > > > >
    > > > > > Looks good and tests fine as well. I've applied it, on top of the
    > > > > > hlist_for_each_entry_safe_rcu() fix.
    > > > > >
    > > > > > http://git.kernel.dk/?p=linux-2.6-bl...6312545f126661
    > > > > >
    > > >
    > > > ok, thanks. anyway I don't think the hlist_for_each_entry_safe_rcu()
    > > > is needed at this point, since the pos->next pointer is still valid
    > > > (at least) until the next rcu_read_unlock(). am I wrong?

    > >
    > > it isn't, but it's still clearer. so either that or a nice comment, I
    > > just stuck with what I had already committed.

    >


    ok I agree on that. the only remaining concern I have is that when
    I first looked at it it seemed to me that hlist_for_each_entry_safe_rcu()
    was missing by purpose from the list interface, since hlist_del_rcu()
    can be called anyway during the traversal from a concurrent context,
    so the semantics of *_safe_* have to be carried out by other means
    (i.e., call_rcu()).

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    Peter Zijlstra writes:
    >
    > Ok I gave it a go, how bad is this text?


    Should be also in the kernel doc description of kmem_cache_create(), so it
    appears in manpages etc.

    -Andi
    >
    > Signed-off-by: Peter Zijlstra
    > ---
    > diff --git a/include/linux/slab.h b/include/linux/slab.h
    > index f950a89..e049ddc 100644
    > --- a/include/linux/slab.h
    > +++ b/include/linux/slab.h
    > @@ -25,6 +25,32 @@
    > #define SLAB_CACHE_DMA 0x00004000UL /* Use GFP_DMA memory */
    > #define SLAB_STORE_USER 0x00010000UL /* DEBUG: Store the last owner for bug hunting */
    > #define SLAB_PANIC 0x00040000UL /* Panic if kmem_cache_create() fails */
    > +/*
    > + * SLAB_DESTROY_BY_RCU - **WARNING** READ THIS!

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02 2008, Paul E. McKenney wrote:
    > On Wed, Apr 02, 2008 at 02:58:19PM +0200, Jens Axboe wrote:
    > > On Wed, Apr 02 2008, Fabio Checconi wrote:
    > > > > From: Jens Axboe
    > > > > Date: Wed, Apr 02, 2008 02:36:39PM +0200
    > > > >
    > > > > > Looks good and tests fine as well. I've applied it, on top of the
    > > > > > hlist_for_each_entry_safe_rcu() fix.
    > > > > >
    > > > > > http://git.kernel.dk/?p=linux-2.6-bl...6312545f126661
    > > > > >
    > > >
    > > > ok, thanks. anyway I don't think the hlist_for_each_entry_safe_rcu()
    > > > is needed at this point, since the pos->next pointer is still valid
    > > > (at least) until the next rcu_read_unlock(). am I wrong?

    > >
    > > it isn't, but it's still clearer. so either that or a nice comment, I
    > > just stuck with what I had already committed.

    >
    > Given Peter's comment, would you be willing to drop the
    > hlist_for_each_entry_safe_rcu()? People tend to read too much into
    > the "safe" sometimes. ;-)


    Sure, let me rebase it... I already sent a pull request to Linus, and he
    just loves when I rebase and change the diffstats behind his back

    --
    Jens Axboe

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02, 2008 at 03:41:23PM +0200, Jens Axboe wrote:
    > On Wed, Apr 02 2008, Paul E. McKenney wrote:
    > > On Wed, Apr 02, 2008 at 02:58:19PM +0200, Jens Axboe wrote:
    > > > On Wed, Apr 02 2008, Fabio Checconi wrote:
    > > > > > From: Jens Axboe
    > > > > > Date: Wed, Apr 02, 2008 02:36:39PM +0200
    > > > > >
    > > > > > > Looks good and tests fine as well. I've applied it, on top of the
    > > > > > > hlist_for_each_entry_safe_rcu() fix.
    > > > > > >
    > > > > > > http://git.kernel.dk/?p=linux-2.6-bl...6312545f126661
    > > > > > >
    > > > >
    > > > > ok, thanks. anyway I don't think the hlist_for_each_entry_safe_rcu()
    > > > > is needed at this point, since the pos->next pointer is still valid
    > > > > (at least) until the next rcu_read_unlock(). am I wrong?
    > > >
    > > > it isn't, but it's still clearer. so either that or a nice comment, I
    > > > just stuck with what I had already committed.

    > >
    > > Given Peter's comment, would you be willing to drop the
    > > hlist_for_each_entry_safe_rcu()? People tend to read too much into
    > > the "safe" sometimes. ;-)

    >
    > Sure, let me rebase it... I already sent a pull request to Linus, and he
    > just loves when I rebase and change the diffstats behind his back


    Would it help if I submitted the patch to you to back it out later? ;-)

    Thanx, Paul
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  12. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02, 2008 at 02:01:13PM +0300, Pekka Enberg wrote:
    > Hi Paul,
    >
    > On Wed, Apr 2, 2008 at 1:55 PM, Paul E. McKenney
    > wrote:
    > > 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().

    >
    > No, kmemcheck is work in progress and does not know about
    > SLAB_DESTROY_BY_RCU yet. The reason I asked Vegard to post the warning
    > was because Peter, Vegard, and myself identified this particular
    > warning as a real problem. But yeah, kmemcheck can cause false
    > positives for RCU for now.


    Would the following be an appropriate fix? It seems to me to be in
    the same spirit as the existing check for s->ctor.

    Signed-off-by: Paul E. McKenney
    ---

    slub_kmemcheck.c | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)

    diff --git a/mm/slub_kmemcheck.c b/mm/slub_kmemcheck.c
    index 8620a8b..e07f62a 100644
    --- a/mm/slub_kmemcheck.c
    +++ b/mm/slub_kmemcheck.c
    @@ -93,6 +93,6 @@ kmemcheck_slab_alloc(struct kmem_cache *s, gfp_t gfpflags, void *object)
    void
    kmemcheck_slab_free(struct kmem_cache *s, void *object)
    {
    - if (!s->ctor)
    + if (!s->ctor && !(s->flags & SLAB_DESTROY_BY_RCU))
    kmemcheck_mark_freed(object, s->objsize);
    }
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  13. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02, 2008 at 03:16:25PM +0200, Fabio Checconi wrote:
    > > From: Jens Axboe
    > > Date: Wed, Apr 02, 2008 02:58:58PM +0200
    > >
    > > On Wed, Apr 02 2008, Jens Axboe wrote:
    > > > On Wed, Apr 02 2008, Fabio Checconi wrote:
    > > > > > From: Jens Axboe
    > > > > > Date: Wed, Apr 02, 2008 02:36:39PM +0200
    > > > > >
    > > > > > > Looks good and tests fine as well. I've applied it, on top of the
    > > > > > > hlist_for_each_entry_safe_rcu() fix.
    > > > > > >
    > > > > > > http://git.kernel.dk/?p=linux-2.6-bl...6312545f126661
    > > > > > >
    > > > >
    > > > > ok, thanks. anyway I don't think the hlist_for_each_entry_safe_rcu()
    > > > > is needed at this point, since the pos->next pointer is still valid
    > > > > (at least) until the next rcu_read_unlock(). am I wrong?
    > > >
    > > > it isn't, but it's still clearer. so either that or a nice comment, I
    > > > just stuck with what I had already committed.

    > >

    >
    > ok I agree on that. the only remaining concern I have is that when
    > I first looked at it it seemed to me that hlist_for_each_entry_safe_rcu()
    > was missing by purpose from the list interface, since hlist_del_rcu()
    > can be called anyway during the traversal from a concurrent context,
    > so the semantics of *_safe_* have to be carried out by other means
    > (i.e., call_rcu()).


    Exactly. ;-)

    Thanx, Paul
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  14. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02, 2008 at 03:40:46PM +0200, Jens Axboe wrote:
    > On Wed, Apr 02 2008, Paul E. McKenney wrote:
    > > On Wed, Apr 02, 2008 at 01:33:27PM +0200, Fabio Checconi wrote:
    > > > > From: Peter Zijlstra
    > > > > 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 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.

    > >
    > > Looks good to me from a strictly RCU viewpoint -- I must confess great
    > > ignorance of the CFQ code. :-/

    >
    > That can always be rectified, given enough spare time :-)


    Hey, I am not complaining about 2007 being gone already, because I am
    still wondering what the heck happened to 2005 and 2006!!! ;-)

    Thanx, Paul

    > > Acked-by: Paul E. McKenney

    >
    > Thanks, added.
    >
    > --
    > Jens Axboe
    >

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  15. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 2, 2008 at 6:08 PM, Paul E. McKenney
    wrote:
    > On Wed, Apr 02, 2008 at 02:01:13PM +0300, Pekka Enberg wrote:
    > > No, kmemcheck is work in progress and does not know about
    > > SLAB_DESTROY_BY_RCU yet. The reason I asked Vegard to post the warning
    > > was because Peter, Vegard, and myself identified this particular
    > > warning as a real problem. But yeah, kmemcheck can cause false
    > > positives for RCU for now.

    >
    > Would the following be an appropriate fix? It seems to me to be in
    > the same spirit as the existing check for s->ctor.


    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.

    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! :-))


    Kind regards,
    Vegard Nossum
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  16. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02 2008, Paul E. McKenney wrote:
    > On Wed, Apr 02, 2008 at 03:41:23PM +0200, Jens Axboe wrote:
    > > On Wed, Apr 02 2008, Paul E. McKenney wrote:
    > > > On Wed, Apr 02, 2008 at 02:58:19PM +0200, Jens Axboe wrote:
    > > > > On Wed, Apr 02 2008, Fabio Checconi wrote:
    > > > > > > From: Jens Axboe
    > > > > > > Date: Wed, Apr 02, 2008 02:36:39PM +0200
    > > > > > >
    > > > > > > > Looks good and tests fine as well. I've applied it, on top of the
    > > > > > > > hlist_for_each_entry_safe_rcu() fix.
    > > > > > > >
    > > > > > > > http://git.kernel.dk/?p=linux-2.6-bl...6312545f126661
    > > > > > > >
    > > > > >
    > > > > > ok, thanks. anyway I don't think the hlist_for_each_entry_safe_rcu()
    > > > > > is needed at this point, since the pos->next pointer is still valid
    > > > > > (at least) until the next rcu_read_unlock(). am I wrong?
    > > > >
    > > > > it isn't, but it's still clearer. so either that or a nice comment, I
    > > > > just stuck with what I had already committed.
    > > >
    > > > Given Peter's comment, would you be willing to drop the
    > > > hlist_for_each_entry_safe_rcu()? People tend to read too much into
    > > > the "safe" sometimes. ;-)

    > >
    > > Sure, let me rebase it... I already sent a pull request to Linus, and he
    > > just loves when I rebase and change the diffstats behind his back

    >
    > Would it help if I submitted the patch to you to back it out later? ;-)


    Hehe, I already wrote to Linus and explained why it changed, so what is
    done is done

    --
    Jens Axboe

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  17. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    Hi Vegard,

    On Wed, Apr 2, 2008 at 6:08 PM, Paul E. McKenney 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.

    Pekka
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  18. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02, 2008 at 06:15:52PM +0200, Vegard Nossum wrote:
    > On Wed, Apr 2, 2008 at 6:08 PM, Paul E. McKenney
    > wrote:
    > > On Wed, Apr 02, 2008 at 02:01:13PM +0300, Pekka Enberg wrote:
    > > > No, kmemcheck is work in progress and does not know about
    > > > SLAB_DESTROY_BY_RCU yet. The reason I asked Vegard to post the warning
    > > > was because Peter, Vegard, and myself identified this particular
    > > > warning as a real problem. But yeah, kmemcheck can cause false
    > > > positives for RCU for now.

    > >
    > > Would the following be an appropriate fix? It seems to me to be in
    > > the same spirit as the existing check for s->ctor.

    >
    > 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.


    Though this case apparently does not qualify as misuse until such
    time as CLONE_IO is implemented.

    And doesn't the current check for ->ctor also potentially hide misuse?

    > 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! :-))


    Glad you liked them!

    And Peter's suggested approach would indeed be more accurate. But I
    will still put my patch forward as a stopgap. ;-)

    Thanx, Paul
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  19. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, Apr 02, 2008 at 06:31:14PM +0200, Jens Axboe wrote:
    > On Wed, Apr 02 2008, Paul E. McKenney wrote:
    > > On Wed, Apr 02, 2008 at 03:41:23PM +0200, Jens Axboe wrote:
    > > > On Wed, Apr 02 2008, Paul E. McKenney wrote:
    > > > > On Wed, Apr 02, 2008 at 02:58:19PM +0200, Jens Axboe wrote:
    > > > > > On Wed, Apr 02 2008, Fabio Checconi wrote:
    > > > > > > > From: Jens Axboe
    > > > > > > > Date: Wed, Apr 02, 2008 02:36:39PM +0200
    > > > > > > >
    > > > > > > > > Looks good and tests fine as well. I've applied it, on top of the
    > > > > > > > > hlist_for_each_entry_safe_rcu() fix.
    > > > > > > > >
    > > > > > > > > http://git.kernel.dk/?p=linux-2.6-bl...6312545f126661
    > > > > > > > >
    > > > > > >
    > > > > > > ok, thanks. anyway I don't think the hlist_for_each_entry_safe_rcu()
    > > > > > > is needed at this point, since the pos->next pointer is still valid
    > > > > > > (at least) until the next rcu_read_unlock(). am I wrong?
    > > > > >
    > > > > > it isn't, but it's still clearer. so either that or a nice comment, I
    > > > > > just stuck with what I had already committed.
    > > > >
    > > > > Given Peter's comment, would you be willing to drop the
    > > > > hlist_for_each_entry_safe_rcu()? People tend to read too much into
    > > > > the "safe" sometimes. ;-)
    > > >
    > > > Sure, let me rebase it... I already sent a pull request to Linus, and he
    > > > just loves when I rebase and change the diffstats behind his back

    > >
    > > Would it help if I submitted the patch to you to back it out later? ;-)

    >
    > Hehe, I already wrote to Linus and explained why it changed, so what is
    > done is done


    ;-)

    Thanx, Paul
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  20. Re: kmemcheck caught read from freed memory (cfq_free_io_context)

    On Wed, 2 Apr 2008, Pekka Enberg wrote:

    > On Wed, Apr 2, 2008 at 2:08 PM, Peter Zijlstra wrote:
    > > but what holds off the slab allocator re-issueing that same object and
    > > someone else writing other stuff into it?

    >
    > Nothing. So you cannot access the object at all after you've called
    > kmem_cache_free(). SLAB_RCU or no SLAB_RCU.


    You can of course access the object. And if you establish a definite state
    of locks on free (and through a ctor) then its even okay to take locks.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread
Page 3 of 4 FirstFirst 1 2 3 4 LastLast