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 14:13:45 +0100
From: Peter Zijlstra <>
To: Liljestrand Hans <>
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, 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

> 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

		// insert fi

If that races against itself, both instances can fail to find an
existing matching fi, and both will insert fi, resulting in a duplicate

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.