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, 2 Apr 2008, Peter Zijlstra wrote: > Does SLAB (as opposed to SLUB) do it differently? No it works the same in both. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of ...

+ Reply to Thread
Page 4 of 4 FirstFirst ... 2 3 4
Results 61 to 68 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, 2 Apr 2008, Peter Zijlstra wrote:

    > Does SLAB (as opposed to SLUB) do it differently?


    No it works the same in both.

    --
    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 2, 2008 at 6:59 PM, Paul E. McKenney
    wrote:
    > 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?


    Hm, no. Objects with a ctor should retain its "initializedness"
    between allocations of the same object. This is one of the features of
    slab allocation.

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

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


    Yes, I realize that it will be needed as a temporary fix. It _is_
    better to hide some real warnings in favour of the showing false ones.
    (But a TODO comment is in order, I believe.)

    Thanks.


    Vegard
    --
    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)

    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 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@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)

    Hi Paul,

    On Wed, Apr 02, 2008 at 07:32:26PM +0300, Pekka J Enberg wrote:
    > > 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.


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


    I suppose you haven't actually run kmemcheck on your machine? We're
    taking a page fault for _every_ memory access so a lock round-trip in
    the SLAB_RCU case is probably not that bad performance-wise :-).
    --
    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 10:53:53PM +0300, Pekka Enberg wrote:
    > Hi Paul,
    >
    > On Wed, Apr 02, 2008 at 07:32:26PM +0300, Pekka J Enberg wrote:
    > > > 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.

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

    >
    > I suppose you haven't actually run kmemcheck on your machine? We're
    > taking a page fault for _every_ memory access so a lock round-trip in
    > the SLAB_RCU case is probably not that bad performance-wise :-).


    Coward that I am, no I have not. ;-)

    The thing that worries me even more than the lock is the need to keep
    track of the addresses.

    Then again, if you are taking a page fault on every access, perhaps not
    such a big deal to allocate the memory and link it into a list...
    But yikes!!! ;-)

    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/

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

    On Wed, Apr 02, 2008 at 01:15:51PM -0700, Paul E. McKenney wrote:
    > On Wed, Apr 02, 2008 at 10:53:53PM +0300, Pekka Enberg wrote:
    > > I suppose you haven't actually run kmemcheck on your machine? We're
    > > taking a page fault for _every_ memory access so a lock round-trip in
    > > the SLAB_RCU case is probably not that bad performance-wise :-).

    >
    > Coward that I am, no I have not. ;-)
    >
    > The thing that worries me even more than the lock is the need to keep
    > track of the addresses.
    >
    > Then again, if you are taking a page fault on every access, perhaps not
    > such a big deal to allocate the memory and link it into a list...
    > But yikes!!! ;-)


    OK, so another approach would be to use a larger shadow block for
    SLAB_DESTROY_BY_RCU slabs, so that each shadow location would have enough
    room for an rcu_head and a size in addition to the flag. That would
    trivialize tracking, or, more accurately, delegate such tracking to the
    RCU infrastructure.

    Of course, the case where the block gets reallocated before the RCU
    grace period ends would also need to be handled (which my rough sketch
    yesterday did -not- handle, by the way...).

    There are a couple of ways of doing this. Probably the easiest approach
    is to add more state to the flag, so that the RCU callback would check
    to see if reallocation had already happened. If so, it would update the
    state to indicate that the rcu_head was again available, and would need to
    repost itself if the block had been freed again after being reallocated.

    The other approach would be to defer actually adding the block to the
    freelist until the grace period expired. This would be more accurate,
    but also quite a bit more intrusive.

    Thoughts?

    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)

    Hi Paul,

    On Thu, 3 Apr 2008, Paul E. McKenney wrote:
    > OK, so another approach would be to use a larger shadow block for
    > SLAB_DESTROY_BY_RCU slabs, so that each shadow location would have enough
    > room for an rcu_head and a size in addition to the flag. That would
    > trivialize tracking, or, more accurately, delegate such tracking to the
    > RCU infrastructure.


    Yeah, or just allocate some extra spaces for the RCU case and not
    overload the current shadow pages. But sounds good to me.

    On Thu, 3 Apr 2008, Paul E. McKenney wrote:
    > Of course, the case where the block gets reallocated before the RCU
    > grace period ends would also need to be handled (which my rough sketch
    > yesterday did -not- handle, by the way...).
    >
    > There are a couple of ways of doing this. Probably the easiest approach
    > is to add more state to the flag, so that the RCU callback would check
    > to see if reallocation had already happened. If so, it would update the
    > state to indicate that the rcu_head was again available, and would need to
    > repost itself if the block had been freed again after being reallocated.
    >
    > The other approach would be to defer actually adding the block to the
    > freelist until the grace period expired. This would be more accurate,
    > but also quite a bit more intrusive.


    We already talked about deferring the actual freeing in kmemcheck to
    better detect these use-after-free conditions with Vegard. So it's
    something that we probably want to do regardless of RCU.

    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/

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

    On Thu, Apr 03, 2008 at 10:49:23PM +0300, Pekka J Enberg wrote:
    > Hi Paul,
    >
    > On Thu, 3 Apr 2008, Paul E. McKenney wrote:
    > > OK, so another approach would be to use a larger shadow block for
    > > SLAB_DESTROY_BY_RCU slabs, so that each shadow location would have enough
    > > room for an rcu_head and a size in addition to the flag. That would
    > > trivialize tracking, or, more accurately, delegate such tracking to the
    > > RCU infrastructure.

    >
    > Yeah, or just allocate some extra spaces for the RCU case and not
    > overload the current shadow pages. But sounds good to me.


    As long as we have an rcu_head for each memory block in the slab, I am
    not to worried about where they are allocated.

    > On Thu, 3 Apr 2008, Paul E. McKenney wrote:
    > > Of course, the case where the block gets reallocated before the RCU
    > > grace period ends would also need to be handled (which my rough sketch
    > > yesterday did -not- handle, by the way...).
    > >
    > > There are a couple of ways of doing this. Probably the easiest approach
    > > is to add more state to the flag, so that the RCU callback would check
    > > to see if reallocation had already happened. If so, it would update the
    > > state to indicate that the rcu_head was again available, and would need to
    > > repost itself if the block had been freed again after being reallocated.
    > >
    > > The other approach would be to defer actually adding the block to the
    > > freelist until the grace period expired. This would be more accurate,
    > > but also quite a bit more intrusive.

    >
    > We already talked about deferring the actual freeing in kmemcheck to
    > better detect these use-after-free conditions with Vegard. So it's
    > something that we probably want to do regardless of RCU.


    Then it is especially important that the rcu_head be pre-allocated.
    Otherwise we could get into out-of-memory deadlocks where a free
    operation is blocked by an allocation operation. ;-)

    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/

+ Reply to Thread
Page 4 of 4 FirstFirst ... 2 3 4