[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 20 Dec 2016 13:35:26 +0000
From: "Reshetova, Elena" <elena.reshetova@...el.com>
To: "kernel-hardening@...ts.openwall.com"
<kernel-hardening@...ts.openwall.com>, Liljestrand Hans <ishkamiel@...il.com>
CC: 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: 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.
I think then we would only convert for now the cases that can be reasonably converted (and it is still a huge patch set) and then keep track and reasons for all other cases recorded in a doc. After we done with the first part and refcount_t API is merged in upstream, we can proceed to more complex cases and if needed involve people who knows the code better than us.
Any objections to this approach?
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.
