Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 20 Dec 2016 17:20:08 +0200
From: Liljestrand Hans <>
To: Peter Zijlstra <>
Cc: "Reshetova, Elena" <>, 
 <>, Greg KH
 <>,  Kees Cook <>,
 "" <>, Boqun Feng
 <>, David Windsor <>, ""
 <>, "" <>
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

> > 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

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

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?


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.