[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 20 Dec 2016 14:13:45 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Liljestrand Hans <ishkamiel@...il.com>
Cc: "Reshetova, Elena" <elena.reshetova@...el.com>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
Greg KH <gregkh@...uxfoundation.org>,
Kees Cook <keescook@...omium.org>,
"will.deacon@....com" <will.deacon@....com>,
Boqun Feng <boqun.feng@...il.com>,
David Windsor <dwindsor@...il.com>, "aik@...abs.ru" <aik@...abs.ru>,
"david@...son.dropbear.id.au" <david@...son.dropbear.id.au>
Subject: Re: Conversion from atomic_t to refcount_t: summary of issues
On Tue, Dec 20, 2016 at 12:55:02PM +0200, Liljestrand Hans wrote:
> For reference, I've listed here the places that were causing "increment
> on 0" WARNs on my previous boot (temporarily allowed inc on 0 to make
> boot possible). These seem to be mostly related to resource reuse, but
> we haven't yet to looked in detail on how to deal with them.
>
> fs/ext4/mballoc.c:3399 ext4_mb_use_preallocated
> Seems to have separate tracking of destruction
This one seems particularly daft, since afaict all pa_count usage is
under pa_lock. No need for it to be atomic. Also, that code is weird, it
has separate pa_free and pa_deleted state.
This should definitely not be converted to refcount_t until its
sanitized.
> net/ipv4/fib_semantics.c:994 fib_create_info
This one I'm not sure how its not broken.
It does something like:
ofi = fib_find_info(fi);
if (ofi) {
// use ofi, free fi
}
spin_lock_hb();
atomic_inc(&fi->fib_clntref);
// insert fi
spin_unlock_hb();
If that races against itself, both instances can fail to find an
existing matching fi, and both will insert fi, resulting in a duplicate
fi.
Also, note that at the point of atomic_inc(), fi isn't in fact published
and therefore this need not be an atomic operation.
I could of course miss something subtle, since I only read part of this
code. In any case, even if that were somehow incorrect, I think you can
init fib_clntref with 1 and make it work with that.
> net/ipv4/devinet.c:233 inetdev_init
This seems to use atomic_inc before the object is published, and could
therefore simply use atomic_set().
> net/ipv4/tcp_ipv4.c:1793 inet_sk_rx_dst_set
That needs more context to be evaluated. Also seems very dodgy code in
any case. We need the caller of this function that calls it on 0.
> net/ipv4/route.c:2153: __ip_route_output_key_hash
need more context, there's not in fact an atomic_ in that function, and
its a giant function.
> net/ipv6/ip6_fib.c:949 fib6_add
Can't make sense of this :/
Didn't look at the rest, but going by the above blindly converting to
refcount_t without prior cleanups isn't a good idea.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
