lkml.org 
[lkml]   [2009]   [Jan]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] lib/idr.c: Zero memory properly in idr_remove_all
    Andrew Morton wrote:
    > On Sat, 10 Jan 2009 10:03:33 +0100 Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
    >
    >> David Moore wrote:
    >>> From: David Moore <dcm@acm.org>
    >>>
    >>> The idr_remove_all() function returns unused slabs to the kmem cache,
    >>> but needs to zero them first or else they will be uninitialized upon
    >>> next use. This fixes crashes which have been observed in the firewire
    >>> subsystem.
    >>>
    >
    > hm.
    >
    >>> ---
    >>> lib/idr.c | 16 +++++++++++++++-
    >>> 1 files changed, 15 insertions(+), 1 deletions(-)
    >>>
    >>> diff --git a/lib/idr.c b/lib/idr.c
    >>> index 1c4f928..69c3455 100644
    >>> --- a/lib/idr.c
    >>> +++ b/lib/idr.c
    >>> @@ -65,6 +65,20 @@ static inline void free_layer(struct idr_layer *p)
    >>> call_rcu(&p->rcu_head, idr_layer_rcu_free);
    >>> }
    >>>
    >>> +static void idr_layer_rcu_free_zero(struct rcu_head *head)
    >>> +{
    >>> + struct idr_layer *layer;
    >>> +
    >>> + layer = container_of(head, struct idr_layer, rcu_head);
    >>> + memset(layer, 0, sizeof(struct idr_layer));
    >>> + kmem_cache_free(idr_layer_cache, layer);
    >>> +}
    >>> +
    >>> +static inline void free_layer_zero(struct idr_layer *p)
    >>> +{
    >>> + call_rcu(&p->rcu_head, idr_layer_rcu_free_zero);
    >>> +}
    >>> +
    >>> /* only called when idp->lock is held */
    >>> static void __move_to_free_list(struct idr *idp, struct idr_layer *p)
    >>> {
    >>> @@ -462,7 +476,7 @@ void idr_remove_all(struct idr *idp)
    >>> id += 1 << n;
    >>> while (n < fls(id)) {
    >>> if (p)
    >>> - free_layer(p);
    >>> + free_layer_zero(p);
    >>> n += IDR_BITS;
    >>> p = *--paa;
    >>> }
    >> Nadia,
    >>
    >> it appears as if post-2.6.26 commit
    >> cf481c20c476ad2c0febdace9ce23f5a4db19582 "idr: make idr_remove rcu-safe"
    >> was buggy as it removed a memset(...0...) from idr_remove_all() without
    >> any obvious replacement. And this patch fixes it. Is this correct?
    >>
    >> This was observed by David in Fedora 2.6.27.* kernels and in 2.6.28, and
    >> I have it seen in vanilla 2.6.28 --- but only after I disabled some
    >> debug kconfig options. The trigger for the bug is not the existing
    >> usage of idr in drivers/firewire/, but a new usage which is not yet in
    >> mainline. More details:
    >> http://marc.info/?l=linux1394-devel&m=123140439522563
    >>
    >> The symptom is that after a few destructions of idr trees (which involve
    >> idr_remove_all() of course), there appear spurious idr entries in
    >> subsequently newly created idr trees. These spurious entries then crash
    >> the driver when it iterates over them.

    ...
    > Are we sure that all the other callers of free_layer() are freeing
    > zeroed objects?
    >
    > It would be cleaner, safer and quite possibly faster to remove the
    > constructor altogether and use kmem_cache_zalloc() to allocate new
    > objects.

    Yes, it sounds at least safer if the allocation path should be fixed up.

    The zeroing was done in idr_remove_all() though since Kristian added it
    in 2.6.23, until 2.6.26 inclusive.

    Kristian, was there a deeper reason to do it at deallocation instead of
    allocation, and does the reason still apply today?
    --
    Stefan Richter
    -=====-==--= ---= -=-=-
    http://arcgraph.de/sr/


    \
     
     \ /
      Last update: 2009-01-10 11:09    [W:0.028 / U:65.324 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site