ipv4: fix the rcu race between free_fib_info and ip_route_output_slow

On Tue, 2012-05-22 at 15:15 -0400, David Miller wrote:
> From: "kun.jiang" <kunx.jiang@intel.com>
> Date: Tue, 22 May 2012 17:48:53 +0800
> 
> > From: Yanmin Zhang <yanmin_zhang@linux.intel.com>
> > 
> > We hit a kernel OOPS.
>  ...
> > In function free_fib_info, we don't reset nexthop_nh->nh_dev to NULL before releasing
> > nh_dev. kfree_rcu(fi, rcu) would release the whole area.
> > 
> > Signed-off-by: Yanmin Zhang <yanmin_zhang@linux.intel.com>
> > Signed-off-by: Kun Jiang <kunx.jiang@intel.com>
> 
> This isn't a fix.  You're keeping around a pointer to a completely
> released object, which is therefore illegal to dereference.
> 
> That's why we must set it to NULL, to catch such illegal accesses.
Thanks for the comments. The network stack in kernel is complicated.

Questions:
1) Why does free_fib_info call call_rcu instead of releasing fi directly?
I assume other cpu might be accessing it. nexthop_nh->nh_dev is in fi.
If other cpu are accessing it, here resetting to NULL would cause other
cpu panic.

void free_fib_info(struct fib_info *fi)
{
        if (fi->fib_dead == 0) {
                pr_warn("Freeing alive fib_info %p\n", fi);
                return;
        }
        change_nexthops(fi) {
                if (nexthop_nh->nh_dev)
                        dev_put(nexthop_nh->nh_dev);
                nexthop_nh->nh_dev = NULL;
        } endfor_nexthops(fi);
        fib_info_cnt--;
        release_net(fi->fib_net);
        call_rcu(&fi->rcu, free_fib_info_rcu);		<=====rcu
}
2) dev_put is to decrease the counter and doesn't release nh_dev.

If 2) is incorrect, how about below solution? It delays the resetting
in rcu.

From: Yanmin Zhang <yanmin_zhang@linux.intel.com>
Date: Wed, 23 May 2012 11:02:03 +0800

> 1) Why does free_fib_info call call_rcu instead of releasing fi directly?
> I assume other cpu might be accessing it. nexthop_nh->nh_dev is in fi.
> If other cpu are accessing it, here resetting to NULL would cause other
> cpu panic.

Because fib trie lookups are done with RCU locking, therefore we must
use RCU freeing to release the object.

What I was trying to impart to you is that removing the NULL
assignment is wrong and that an alternative fix is warranted (hint:
consider moving something into the RCU release).
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 2012-05-22 at 23:23 -0400, David Miller wrote:
> From: Yanmin Zhang <yanmin_zhang@linux.intel.com>
> Date: Wed, 23 May 2012 11:02:03 +0800
> 
> > 1) Why does free_fib_info call call_rcu instead of releasing fi directly?
> > I assume other cpu might be accessing it. nexthop_nh->nh_dev is in fi.
> > If other cpu are accessing it, here resetting to NULL would cause other
> > cpu panic.
> 
> Because fib trie lookups are done with RCU locking, therefore we must
> use RCU freeing to release the object.
> 
> What I was trying to impart to you is that removing the NULL
> assignment is wrong and that an alternative fix is warranted (hint:
> consider moving something into the RCU release).
Thanks for the explanation.

How about the new patch posted in the end of previous reply? It does move the
the resetting to RCU release.
https://lkml.org/lkml/2012/5/22/558?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Yanmin Zhang <yanmin_zhang@linux.intel.com>
Date: Wed, 23 May 2012 11:30:41 +0800

> How about the new patch posted in the end of previous reply? It does move the
> the resetting to RCU release.
> https://lkml.org/lkml/2012/5/22/558?

If you test it and submit it formally I'll probably apply it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 2012-05-22 at 23:23 -0400, David Miller wrote:
> From: Yanmin Zhang <yanmin_zhang@linux.intel.com>
> Date: Wed, 23 May 2012 11:02:03 +0800
> 
> > 1) Why does free_fib_info call call_rcu instead of releasing fi directly?
> > I assume other cpu might be accessing it. nexthop_nh->nh_dev is in fi.
> > If other cpu are accessing it, here resetting to NULL would cause other
> > cpu panic.
> 
> Because fib trie lookups are done with RCU locking, therefore we must
> use RCU freeing to release the object.
> 
> What I was trying to impart to you is that removing the NULL
> assignment is wrong and that an alternative fix is warranted (hint:
> consider moving something into the RCU release).
> --

Its more than that I'm afraid.

fi->fib_dev (aka fib_nh[0].nh_dev) need full RCU protection.

Also the "fib_info_cnt--;" must stay in free_fib_info() (so that it is
protected by RTNL)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 2012-05-22 at 23:49 -0400, David Miller wrote:
> From: Yanmin Zhang <yanmin_zhang@linux.intel.com>
> Date: Wed, 23 May 2012 11:30:41 +0800
> 
> > How about the new patch posted in the end of previous reply? It does move the
> > the resetting to RCU release.
> > https://lkml.org/lkml/2012/5/22/558?
> 
> If you test it and submit it formally I'll probably apply it.
We did test it. But it's hard to reproduce it. We hit it the issue
for a couple of times when running MTBF testing on Android smart phone.
Mostly, it happens after MTBF runs for more than 12 hours. To releasing
the product, MTBF testing should run for hundreds of hours. This bug
blocks it.

We would submit it formally and also run more testing.

Sorry for taking you too much time.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2012-05-23 at 06:37 +0200, Eric Dumazet wrote:
> On Tue, 2012-05-22 at 23:23 -0400, David Miller wrote:
> > From: Yanmin Zhang <yanmin_zhang@linux.intel.com>
> > Date: Wed, 23 May 2012 11:02:03 +0800
> > 
> > > 1) Why does free_fib_info call call_rcu instead of releasing fi directly?
> > > I assume other cpu might be accessing it. nexthop_nh->nh_dev is in fi.
> > > If other cpu are accessing it, here resetting to NULL would cause other
> > > cpu panic.
> > 
> > Because fib trie lookups are done with RCU locking, therefore we must
> > use RCU freeing to release the object.
> > 
> > What I was trying to impart to you is that removing the NULL
> > assignment is wrong and that an alternative fix is warranted (hint:
> > consider moving something into the RCU release).
> > --
> 
> Its more than that I'm afraid.
> 
> fi->fib_dev (aka fib_nh[0].nh_dev) need full RCU protection.
The new patch posted at https://lkml.org/lkml/2012/5/22/558 does move the
resetting to RCU protection.

> 
> Also the "fib_info_cnt--;" must stay in free_fib_info() (so that it is
> protected by RTNL)
We would move "fib_info_cnt--;" back to free_fib_info.

Thanks,
Yanmin


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2012-05-23 at 12:54 +0800, Yanmin Zhang wrote:

> > fi->fib_dev (aka fib_nh[0].nh_dev) need full RCU protection.
> The new patch posted at https://lkml.org/lkml/2012/5/22/558 does move the
> resetting to RCU protection.

Its not enough.

We must take care that all users are in a RCU protected region.

They might be already, but a full check is needed.

For example 

net/ipv4/fib_trie.c:2563:    fi->fib_dev ? fi->fib_dev->name : "*"

looks to be safe (because already in a rcu_read_lock())

But its not.

Right thing would be to do :

struct net_device *ndev = rcu_dereference(fi->fib_dev)

	...
	ndev ? ndev->name : "*"




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2012-05-23 at 12:41 +0800, Yanmin Zhang wrote:
> We did test it. But it's hard to reproduce it. We hit it the issue
> for a couple of times when running MTBF testing on Android smart phone.
> Mostly, it happens after MTBF runs for more than 12 hours. To releasing
> the product, MTBF testing should run for hundreds of hours. This bug
> blocks it.
> 

I have an idea how to trigger this in my test machine.

> We would submit it formally and also run more testing.
> 
> Sorry for taking you too much time.

Hey, you fix a bug, take your time and dont worry.

Thanks


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2012-05-23 at 07:02 +0200, Eric Dumazet wrote:
> On Wed, 2012-05-23 at 12:54 +0800, Yanmin Zhang wrote:
> 
> > > fi->fib_dev (aka fib_nh[0].nh_dev) need full RCU protection.
> > The new patch posted at https://lkml.org/lkml/2012/5/22/558 does move the
> > resetting to RCU protection.
> 
> Its not enough.
> 
> We must take care that all users are in a RCU protected region.
> 
> They might be already, but a full check is needed.
> 
> For example 
> 
> net/ipv4/fib_trie.c:2563:    fi->fib_dev ? fi->fib_dev->name : "*"
> 
> looks to be safe (because already in a rcu_read_lock())
> 
> But its not.
> 
> Right thing would be to do :
> 
> struct net_device *ndev = rcu_dereference(fi->fib_dev)
> 
> 	...
> 	ndev ? ndev->name : "*"
Thanks for the pointer.

Besides fi->fib_dev here, we need check fi->fib_hash, fi->fib_lhash,
and nexthop_nh->nh_hash.

Yanmin


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

> 
> Besides fi->fib_dev here, we need check fi->fib_hash, fi->fib_lhash,
> and nexthop_nh->nh_hash.

No, its not needed.

I am sending a patch, because I feel this area is too complex for non
netdev guys.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2012-05-23 at 08:27 +0200, Eric Dumazet wrote:
> On Wed, 2012-05-23 at 14:15 +0800, Yanmin Zhang wrote:
> 
> > 
> > Besides fi->fib_dev here, we need check fi->fib_hash, fi->fib_lhash,
> > and nexthop_nh->nh_hash.
> 
> No, its not needed.
I am checking it now.
fib_create_info=>fib_find_info, but fib_find_info is not protected by
fib_info_lock. See the codes:


link_it:
        ofi = fib_find_info(fi);
        if (ofi) {
                fi->fib_dead = 1;
                free_fib_info(fi);
                ofi->fib_treeref++;
                return ofi;
        }

> 
> I am sending a patch, because I feel this area is too complex for non
> netdev guys.
Indeed, it's complicated. I will test your patches.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2012-05-23 at 14:47 +0800, Yanmin Zhang wrote:
> On Wed, 2012-05-23 at 08:27 +0200, Eric Dumazet wrote:
> > On Wed, 2012-05-23 at 14:15 +0800, Yanmin Zhang wrote:
> > 
> > > 
> > > Besides fi->fib_dev here, we need check fi->fib_hash, fi->fib_lhash,
> > > and nexthop_nh->nh_hash.
> > 
> > No, its not needed.
> I am checking it now.
> fib_create_info=>fib_find_info, but fib_find_info is not protected by
> fib_info_lock. See the codes:
> 
> 
> link_it:
>         ofi = fib_find_info(fi);
>         if (ofi) {
>                 fi->fib_dead = 1;
>                 free_fib_info(fi);
>                 ofi->fib_treeref++;
>                 return ofi;
>         }
> 
> > 
> > I am sending a patch, because I feel this area is too complex for non
> > netdev guys.
> Indeed, it's complicated. I will test your patches.

Please hold on, I'll send a v2



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

> 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().

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



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2012-05-23 at 09:13 +0200, Eric Dumazet wrote:
> On Wed, 2012-05-23 at 08:55 +0200, Eric Dumazet wrote:
> 
> > 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.

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

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?

Thanks for the direct speaking.

Yanmin


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2012-05-23 at 15:24 +0800, Yanmin Zhang wrote:
> On Wed, 2012-05-23 at 09:13 +0200, Eric Dumazet wrote:
> > On Wed, 2012-05-23 at 08:55 +0200, Eric Dumazet wrote:
> > 
> > > 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.
> 
> > 
> > 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 )



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

Since its not a fast path and hash table can be resized, its probably
not worth the pain.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2012-05-23 at 09:39 +0200, Eric Dumazet wrote:
> On Wed, 2012-05-23 at 15:24 +0800, Yanmin Zhang wrote:
> > On Wed, 2012-05-23 at 09:13 +0200, Eric Dumazet wrote:
> > > On Wed, 2012-05-23 at 08:55 +0200, Eric Dumazet wrote:
> > > 
> > > > 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.
> > 
> > > 
> > > 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.
Thanks. We would work out a new patch and post it here after running MTBF
testing.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html