Skip to contentSkip to conversation

ipmr_free_table() should be called under taken rtnl_lock

PATCH In review Vasily Averin 2 participants 7 messages Jul 7, 2015 – Jul 8, 2015

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Vasily Averin

author
#0 lore raw

PATCH 3/3 ipmr_free_table() should be called under taken rtnl_lock

ipmr_free_table() calls unregister_netdevice_many() inside and changes net_todo_list protected by rtnl_lock

Signed-off-by Vasily Averin <[email protected]>
---
 net/ipv6/ip6mr.c | 2 ++
 1 file changed, 2 insertions(+)
net/ipv6/ip6mr.c +2 -0
index 74ceb73..9108636 100644
@@ -250,7 +250,9 @@ static int __net_init ip6mr_rules_init(struct net *net)
 	return 0;
 
 err2:
+	rtnl_lock();
 	ip6mr_free_table(mrt);
+	rtnl_unlock();
 err1:
 	fib_rules_unregister(ops);
 	return err;
Signature · 1 line
1.9.1
#1 lore raw
Quoted 3 lines · Vasily Averin

ipmr_free_table() calls unregister_netdevice_many() inside and changes net_todo_list protected by rtnl_lock

Did you see any real bug?

ipmr_free_table() is called in failure path, in this case there is no device registered yet, so unregister should be just a nop?

author
#2 lore raw
Quoted 5 lines · Cong Wang
Quoted 2 lines · Vasily Averin

ipmr_free_table() calls unregister_netdevice_many() inside and changes net_todo_list protected by rtnl_lock

Did you see any real bug?

No, it was result of manual code review.

Quoted 2 lines

ipmr_free_table() is called in failure path, in this case there is no device registered yet, so unregister should be just a nop?

However may be it's better to mark this place for future anyway?

#3 lore raw
Quoted 5 lines · Vasily Averin
Quoted 4 lines · Cong Wang
Quoted 2 lines · Vasily Averin

ipmr_free_table() calls unregister_netdevice_many() inside and changes net_todo_list protected by rtnl_lock

Quoted 8 lines
Quoted 1 line

Did you see any real bug?

No, it was result of manual code review.

Quoted 2 lines

ipmr_free_table() is called in failure path, in this case there is no device registered yet, so unregister should be just a nop?

However may be it's better to mark this place for future anyway?

Then add a comment there. ;)

author
#4 lore raw
Quoted 8 lines · Cong Wang
Quoted 7 lines · Vasily Averin
Quoted 5 lines · Cong Wang
Quoted 2 lines · Vasily Averin

ipmr_free_table() calls unregister_netdevice_many() inside and changes net_todo_list protected by rtnl_lock

Did you see any real bug?

Quoted 8 lines
Quoted 6 lines

No, it was result of manual code review.

Quoted 2 lines

ipmr_free_table() is called in failure path, in this case there is no device registered yet, so unregister should be just a nop?

However may be it's better to mark this place for future anyway?

Then add a comment there. ;)

As you can see I'm not familiar with this code,
so I would like to ask you to do it. :)

author
#5 lore raw
Quoted 14 lines · Vasily Averin
Quoted 13 lines · Cong Wang
Quoted 12 lines · Vasily Averin
Quoted 5 lines · Cong Wang
Quoted 2 lines · Vasily Averin

ipmr_free_table() calls unregister_netdevice_many() inside and changes net_todo_list protected by rtnl_lock

Did you see any real bug?

No, it was result of manual code review.

Quoted 2 lines

ipmr_free_table() is called in failure path, in this case there is no device registered yet, so unregister should be just a nop?

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

However may be it's better to mark this place for future anyway?

Then add a comment there. ;)

As you can see I'm not familiar with this code,
so I would like to ask you to do it. :)

Seems I've found a better idea:
to add ASSERT_RTNL() into unregister_netdevice_many()

author
#6 lore raw
Quoted 17 lines · Vasily Averin
Quoted 16 lines · Vasily Averin
Quoted 15 lines · Cong Wang
Quoted 13 lines · Vasily Averin
Quoted 5 lines · Cong Wang
Quoted 2 lines · Vasily Averin

ipmr_free_table() calls unregister_netdevice_many() inside and changes net_todo_list protected by rtnl_lock

Did you see any real bug?

No, it was result of manual code review.

Quoted 2 lines

ipmr_free_table() is called in failure path, in this case there is no device registered yet, so unregister should be just a nop?

However may be it's better to mark this place for future anyway?

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Then add a comment there. ;)

As you can see I'm not familiar with this code,
so I would like to ask you to do it. :)

Seems I've found a better idea:
to add ASSERT_RTNL() into unregister_netdevice_many()

It was done already,
there is ASSERT_RTNL() in rollback_registered_many() called from unregister_netdevice_many().

Please drop this patch, seems it isn't required.