[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 20 Dec 2016 17:20:08 +0200
From: Liljestrand Hans <ishkamiel@...il.com>
To: Peter Zijlstra <peterz@...radead.org>
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, 2016-12-20 at 14:13 +0100, Peter Zijlstra wrote:
> 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.
Thanks, I guess that's a relief, was just trying to figure this out, and
came pretty much to the same conclusion about the weirdness.
>
> > 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().
Oh. Thank you, this seems to be the case for some of the latter cases
too.
>
> > 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.
Yes couldn't find that either. It is possible I've made some mistake
when getting these from dmesg logs.
> > 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.
Yes, I agree. Do you propose we just leave the weirder cases as
atmoic_t, or should we try to incorporate needed cleanup in this initial
patchset?
As for the remaining locations, the following seem to be all incs on
unpublished objects, so the refcount_set strategy should work:
net/ipv6/route.c:1048 ip6_pol_route
net/ipv6/addrconf.c:930 ipv6_add_addr
net/ipv6/addrconf.c:357 ipv6_add_dev
mm/backing-dev.c:399 wb_congested_get_create
net/core/filter.c:940 sk_filter_charge
The sk_filter_charge is a bit iffier, since the refcount is passed in
from the caller. Still, the two calling locations have both just before
allocated/set the refcount, so I guess we could use refcount_set here
too?
fs/inode.c:813 find_inode_fast
This seems to be doing a search for freed up objects that are then
reused, maybe. Not sure if we can guarantee the refcount is 0, nor if it
would be appropriate to use refcount_set even if we could?
Thanks,
-hans
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.
