Re: [PATCH] ipv4: fix the rcu race between free_fib_info and ip_route_output_slow

On Wed, 2012-05-23 at 15:24 +0800, Yanmin Zhang wrote:

On Wed, 2012-05-23 at 09:13 +0200, Eric Dumazet wrote:

quoted

On Wed, 2012-05-23 at 08:55 +0200, Eric Dumazet wrote:

quoted

Please hold on, I'll send a v2

I believe your patch should be fine, if you move back the
fib_info_cnt--;

So only do the dev_put() in free_fib_info_rcu().

We would do so in a new patch.

quoted

No need to clear nh_dev to NULL since we are freeing fi at the end of
function.

David suggests to reset it to NULL to detect other potential
race conditions.

Yes but no.

Users are in a RCU read lock and we should not change nh_dev to NULL
while they are running.

Thats what I tried to do (David message gave me this wrong hint) but it
came to a dead issue.

Only after a grace period we can :
	dev_put() all involved net_devices
	kfree(fi)

Besides above suggestions, how do you think about:

fib_create_info=>fib_find_info, but fib_find_info is not protected by
fib_info_lock. See the codes:

fib_create_info()
{
	...
link_it:
        ofi = fib_find_info(fi);
        if (ofi) {
                fi->fib_dead = 1;
                free_fib_info(fi);
                ofi->fib_treeref++;
                return ofi;
        }
        fi->fib_treeref++;
        atomic_inc(&fi->fib_clntref);
        spin_lock_bh(&fib_info_lock);

	...
}

I plan to change it to hold fib_info_lock before calling fib_find_info. Is
it ok for you?

Its not needed we hold RTNL mutex.

spinlock is needed mainly because of ip_fib_check_default(), but this
could be changed to use RCU as well. 

(readers use RCU, writers already hold RTNL -> no more fib_info_lock )