linux-kernel - Re: [PATCH v3] slub: fix false-positive lockdep warning in free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140205161952.781e3a0e@gandalf.local.home>
Date:	Wed, 5 Feb 2014 16:19:52 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	David Rientjes <rientjes@...gle.com>
Cc:	Vladimir Davydov <vdavydov@...allels.com>,
	akpm@...ux-foundation.org, penberg@...nel.org, cl@...ux.com,
	linux-kernel@...r.kernel.org, a.p.zijlstra@...llo.nl
Subject: Re: [PATCH v3] slub: fix false-positive lockdep warning in
 free_partial()

On Wed, 5 Feb 2014 13:07:05 -0800 (PST)
David Rientjes <rientjes@...gle.com> wrote:

> The functions that manipulate the partial lists was modified by 
> c65c1877bd68 ("slub: use lockdep_assert_held") which replaced commentary 
> with runtime checking on debug kernels with lockdep enabled.  I'm not sure 
> adding more code to do the remove_partial() and __remove_partial() variant 
> is the right solution to just bypass the check; if anything, I think we 
> should accept the fact that the comment should have been "requires 
> n->list_lock if the slab cache can be accessed by other cpus" that makes 
> it clear we don't need it for init and destroy paths.

Then add the comment that clears this up. But lets not add spinlocks
just to quiet something if they truly are not needed.

We use "__" variants all the time. That's really not extra code.

Heck, if you want, call it remove_freed_partial() that shows that this
version skips the check because it is freed.

And if you don't want to have remove_freed_partial() being called by
remove_partial() than still keep the "__" variant, add a
"__always_inline" to it, and then do:

static __always_inline
__remove_partial(struct kmem_cache_node *n, struct page *page)
{
        list_del(&page->lru);
        n->nr_partial--;
}

static inline remove_partial(struct kmem_cache_node *n,
                             struct page *page)
{
        lockdep_assert_held(&n->list_lock);
        __remove_partial(n, page);
}


static inline remove_freed_partial(struct kmem_cache_node *n,
                             struct page *page)
{
        __remove_partial(n, page);
}

The naming like this documents itself.

-- Steve

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