linux-kernel - Re: [PATCH 2/2] Introduce __cond_lock

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] [day] [month] [year] [list]

Message-ID: <20171230071720.GE27959@bombadil.infradead.org>
Date:   Fri, 29 Dec 2017 23:17:20 -0800
From:   Matthew Wilcox <willy@...radead.org>
To:     Luc Van Oostenryck <luc.vanoostenryck@...il.com>
Cc:     Josh Triplett <josh@...htriplett.org>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        linux-kernel@...r.kernel.org, Dave Hansen <dave.hansen@...el.com>,
        linux-mm@...ck.org, Matthew Wilcox <mawilcox@...rosoft.com>
Subject: Re: [PATCH 2/2] Introduce __cond_lock_err

On Wed, Dec 27, 2017 at 03:28:54PM +0100, Luc Van Oostenryck wrote:
> On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote:
> > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote:
> > 
> > While I've got you, I've been looking at some other sparse warnings from
> > this file.  There are several caused by sparse being unable to handle
> > the following construct:
> > 
> > 	if (foo)
> > 		x = NULL;
> > 	else {
> > 		x = bar;
> > 		__acquire(bar);
> > 	}
> > 	if (!x)
> > 		return -ENOMEM;
> > 
> > Writing it as:
> > 
> > 	if (foo)
> > 		return -ENOMEM;
> > 	else {
> > 		x = bar;
> > 		__acquire(bar);
> > 	}
> > 
> > works just fine.  ie this removes the warning:
> 
> It must be noted that these two versions are not equivalent
> (in the first version, it also returns with -ENOMEM if bar
> is NULL/zero).

They happen to be equivalent in the original; I was providing a simplified
version.  Here's the construct sparse can't understand:

        dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl);
        if (!dst_pte)
                return -ENOMEM;

with:

#define pte_alloc(mm, pmd, address)                     \
        (unlikely(pmd_none(*(pmd))) && __pte_alloc(mm, pmd, address))

#define pte_offset_map_lock(mm, pmd, address, ptlp)     \
({                                                      \
        spinlock_t *__ptl = pte_lockptr(mm, pmd);       \
        pte_t *__pte = pte_offset_map(pmd, address);    \
        *(ptlp) = __ptl;                                \
        spin_lock(__ptl);                               \
        __pte;                                          \
})

#define pte_alloc_map_lock(mm, pmd, address, ptlp)      \
        (pte_alloc(mm, pmd, address) ?                  \
                 NULL : pte_offset_map_lock(mm, pmd, address, ptlp))

If pte_alloc() succeeds, pte_offset_map_lock() will return non-NULL.
Manually inlining pte_alloc_map_lock() into the caller like so:

        if (pte_alloc(dst_mm, dst_pmd, addr)
		return -ENOMEM;
        dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, addr, ptlp);

causes sparse to not warn.

> > Is there any chance sparse's dataflow analysis will be improved in the
> > near future?
> 
> A lot of functions in the kernel have this context imbalance,
> really a lot. For example, any function doing conditional locking
> is a problem here. Happily when these functions are inlined,
> sparse, thanks to its optimizations, can remove some paths and
> merge some others. 
> So yes, by adding some smartness to sparse, some of the false
> warnings will be removed, however:
> 1) some __must_hold()/__acquires()/__releases() annotations are
>    missing, making sparse's job impossible.

Partly there's a documentation problem here.  I'd really like to see a
document explaining how to add sparse annotations to a function which
intentionally does conditional locking.  For example, should we be
annotating the function as __acquires, and then marking the exits which
don't acquire the lock with __acquire(), or should we not annotate
the function, and annotate the exits which _do_ acquire the lock as
__release() with a comment like /* Caller will release */

> 2) a lot of the 'false warnings' are not so false because there is
>    indeed two possible paths with different lock state
> 3) it has its limits (at the end, giving the correct warning is
>    equivalent to the halting problem).
> 
> Now, to answer to your question, I'm not aware of any effort that would
> make a significant differences (it would need, IMO, code hoisting & 
> value range propagation).

That's fair.  I wonder if we were starting from scratch whether we'd
choose to make sparse a GCC plugin today.