linux-kernel - Re: [PATCH] fs: Safe rcu access to hlist.

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: <20171120204253.GA8933@bombadil.infradead.org>
Date:   Mon, 20 Nov 2017 12:42:53 -0800
From:   Matthew Wilcox <willy@...radead.org>
To:     Luc Van Oostenryck <luc.vanoostenryck@...il.com>
Cc:     Tim Hansen <devtimhansen@...il.com>,
        Al Viro <viro@...IV.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        alexander.levin@....verizon.com,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH] fs: Safe rcu access to hlist.

On Mon, Nov 20, 2017 at 09:01:32PM +0100, Luc Van Oostenryck wrote:
> [not knowing much about RCU's needs here but knowing quite a bit
>  about sparse]
> 
> I think the issue here is mainly about the use of the address space.
> For kernel space vs. __user vs. __iomem, address space works quite
> well: a pointer points either to a kernel address or to userland
> or to some device or bus memory. It's an exclusive thing.
> So you can annotate the pointer with __user or __iomem, it's a 
> kind of extension of the typing system, and you can let sparse
> do its job.
> For the endianness annotations, it's very similar: a variable
> either points to a native value or to a big or little endian
> value, only one can (should!) be correct. The __be32/__le32/...
> annotations are once again an extension of the typing system.
> Fine for sparse.
> 
> For RCU, the impression I have is that things are completly
> different: it's more a question of transient state than
> something exclusive. The choice to use another address space
> imposes the need of a lot of artificial annotation. And to 
> make sparse able to do its job, a lot of artificial helpers
> are needed to cast variable in and out of the __rcu address
> space.

I disagree.  The notion of whether a pointer is protected by RCU or not
is definitely not transient.  There are a lot of places in the kernel
with missing RCU annotations, and that's where you'll see a lot of
sparse warnings.  They're even correct in some cases!  For example,
this part *of the page cache* is not RCU safe (uhm, if I'm reading
rcu_dereference.txt correctly):

        void **slot;
        rcu_read_lock();
        radix_tree_for_each_slot(slot, &mapping->page_tree, &iter, start) {
                page = radix_tree_deref_slot(slot);
                page_cache_get_speculative(head)
                /* Has the page moved? */
                if (unlikely(page != *slot)) {

Now, it's pretty subtle why it's wrong, and we're probably getting away
with it with current compiler & CPU technology, but if people were more
diligent about the sparse RCU warnings, there would be no doubt that it
was correct.

(one of the major problems was that the radix tree was not diligent about
annotating the 'slot' pointer as being an __rcu pointer).