[PATCH] bonding: remove sysfs before removing devices

[PATCH] bonding: remove sysfs before removing devices

[Veaceslav Falico]

We have a race condition if we try to rmmod bonding and simultaneously add
a bond master through sysfs. In bonding_exit() we first remove the devices
(through rtnl_link_unregister() ) and only after that we remove the sysfs.
If we manage to add a device through sysfs after that the devices were
removed - we'll end up with that device/sysfs structure and with the module
unloaded.

Fix this by first removing the sysfs and only after that calling
rtnl_link_unregister().

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 11a8cb3..dadeffc 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4902,8 +4902,8 @@ static void __exit bonding_exit(void)
 
 	bond_destroy_debugfs();
 
-	rtnl_link_unregister(&bond_link_ops);
 	unregister_pernet_subsys(&bond_net_ops);
+	rtnl_link_unregister(&bond_link_ops);
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	/*
-- 
1.7.1

Re: [PATCH] bonding: remove sysfs before removing devices

[David Miller]

From: Veaceslav Falico <vfalico@redhat.com>
Date: Wed,  3 Apr 2013 17:46:33 +0200

> We have a race condition if we try to rmmod bonding and simultaneously add
> a bond master through sysfs. In bonding_exit() we first remove the devices
> (through rtnl_link_unregister() ) and only after that we remove the sysfs.
> If we manage to add a device through sysfs after that the devices were
> removed - we'll end up with that device/sysfs structure and with the module
> unloaded.
> 
> Fix this by first removing the sysfs and only after that calling
> rtnl_link_unregister().
> 
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

Applied and queued up for -stable, thanks!

Re: [PATCH] bonding: remove sysfs before removing devices

[Nikolay Aleksandrov]

Hi,
Sorry for the late reply but I was travelling this week. In my opinion this
fix is wrong because in bond_uninit() (called by rtnl_link_unregister)
you have:
list_del(&bond->bond_list);
which is linked in the bond_net dev_list which is freed by
unregister_pernet_subsys.
You'll get a corrupted list warning at best.

Here's a sample from running insmod max_bonds=3/rmmod in a loop with the
patch applied:
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302186] ------------[ cut here
]------------
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302191] WARNING: at
lib/list_debug.c:62 __list_del_entry+0x82/0xd0()
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302192] Hardware name: VirtualBox
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302194] list_del corruption.
next->prev should be ffff880036bc6860, but was ffff88002ee23000
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302194] Modules linked in:
bonding(O-) ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6
nf_conntrack_ipv4 nf_defrag_ipv4 ip6table_filter xt_conntrack
nf_conntrack ip6_tables snd_hda_codec_idt snd_hda_intel snd_hda_codec
snd_hwdep snd_seq snd_seq_device ppdev snd_pcm pcspkr snd_page_alloc
i2c_piix4 joydev snd_timer snd soundcore i2c_core microcode parport_pc
parport e1000 [last unloaded: bonding]
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302214] Call Trace:
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302219]  [<ffffffff8105e99f>]
warn_slowpath_common+0x7f/0xc0
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302221]  [<ffffffff8105ea96>]
warn_slowpath_fmt+0x46/0x50
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302225]  [<ffffffff8164efaf>]
? printk+0x61/0x63
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302227]  [<ffffffff8130c302>]
__list_del_entry+0x82/0xd0
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302229]  [<ffffffff8130c361>]
list_del+0x11/0x40
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302233]  [<ffffffffa0187f70>]
bond_uninit+0x70/0xd0 [bonding]
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302236]  [<ffffffff815486d8>]
rollback_registered_many+0x158/0x220
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302238]  [<ffffffff815487f9>]
unregister_netdevice_many+0x19/0x60
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302240]  [<ffffffff815575ae>]
__rtnl_link_unregister+0x6e/0xb0
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302243]  [<ffffffff81557b7e>]
rtnl_link_unregister+0x1e/0x30
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302246]  [<ffffffffa019359e>]
bonding_exit+0x2d/0xa8f [bonding]
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302249]  [<ffffffff810c1dd0>]
sys_delete_module+0x170/0x2d0
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302252]  [<ffffffff81014981>]
? do_notify_resume+0x71/0xb0
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302255]  [<ffffffff81661899>]
system_call_fastpath+0x16/0x1b
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302256] ---[ end trace
31cca9f26623fa11 ]---
Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302417] bonding: bond1:
released all slaves

You can hit this also with a NULL pointer dereference.
I have a correct fix for this bug which I intend to post next week when
I get back and after some more testing.

Please let me know if I've missed something about this patch.

Best regards,
 Nik

Re: [PATCH] bonding: remove sysfs before removing devices

[Veaceslav Falico]

On Sat, Apr 06, 2013 at 12:15:11AM +0200, Nikolay Aleksandrov wrote:
>Hi,
>Sorry for the late reply but I was travelling this week. In my opinion this
>fix is wrong because in bond_uninit() (called by rtnl_link_unregister)
>you have:
>list_del(&bond->bond_list);
>which is linked in the bond_net dev_list which is freed by
>unregister_pernet_subsys.

Yep, you're right, I've hit it recently with the patch applied, and now
working on it. However, I still think that the idea of the patch is correct
- i.e. to first disable sysfs (especially bonding_masters) and only
  afterwards to start removing everything else. Or, obviously, to finally
add normal locking to sysfs functions.

Anyway, this corruption is really rare, so I'll wait for your fix next
week.

>You'll get a corrupted list warning at best.
>
>Here's a sample from running insmod max_bonds=3/rmmod in a loop with the
>patch applied:
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302186] ------------[ cut here
>]------------
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302191] WARNING: at
>lib/list_debug.c:62 __list_del_entry+0x82/0xd0()
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302192] Hardware name: VirtualBox
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302194] list_del corruption.
>next->prev should be ffff880036bc6860, but was ffff88002ee23000
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302194] Modules linked in:
>bonding(O-) ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6
>nf_conntrack_ipv4 nf_defrag_ipv4 ip6table_filter xt_conntrack
>nf_conntrack ip6_tables snd_hda_codec_idt snd_hda_intel snd_hda_codec
>snd_hwdep snd_seq snd_seq_device ppdev snd_pcm pcspkr snd_page_alloc
>i2c_piix4 joydev snd_timer snd soundcore i2c_core microcode parport_pc
>parport e1000 [last unloaded: bonding]
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302214] Call Trace:
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302219]  [<ffffffff8105e99f>]
>warn_slowpath_common+0x7f/0xc0
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302221]  [<ffffffff8105ea96>]
>warn_slowpath_fmt+0x46/0x50
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302225]  [<ffffffff8164efaf>]
>? printk+0x61/0x63
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302227]  [<ffffffff8130c302>]
>__list_del_entry+0x82/0xd0
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302229]  [<ffffffff8130c361>]
>list_del+0x11/0x40
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302233]  [<ffffffffa0187f70>]
>bond_uninit+0x70/0xd0 [bonding]
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302236]  [<ffffffff815486d8>]
>rollback_registered_many+0x158/0x220
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302238]  [<ffffffff815487f9>]
>unregister_netdevice_many+0x19/0x60
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302240]  [<ffffffff815575ae>]
>__rtnl_link_unregister+0x6e/0xb0
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302243]  [<ffffffff81557b7e>]
>rtnl_link_unregister+0x1e/0x30
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302246]  [<ffffffffa019359e>]
>bonding_exit+0x2d/0xa8f [bonding]
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302249]  [<ffffffff810c1dd0>]
>sys_delete_module+0x170/0x2d0
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302252]  [<ffffffff81014981>]
>? do_notify_resume+0x71/0xb0
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302255]  [<ffffffff81661899>]
>system_call_fastpath+0x16/0x1b
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302256] ---[ end trace
>31cca9f26623fa11 ]---
>Apr  5 23:54:54 dhcp-1-171 kernel: [   21.302417] bonding: bond1:
>released all slaves
>
>You can hit this also with a NULL pointer dereference.
>I have a correct fix for this bug which I intend to post next week when
>I get back and after some more testing.
>
>Please let me know if I've missed something about this patch.
>
>Best regards,
> Nik

Re: [PATCH] bonding: remove sysfs before removing devices

[Nikolay Aleksandrov]

On 04/06/2013 01:21 AM, Veaceslav Falico wrote:
> On Sat, Apr 06, 2013 at 12:15:11AM +0200, Nikolay Aleksandrov wrote:
>> Hi,
>> Sorry for the late reply but I was travelling this week. In my
>> opinion this
>> fix is wrong because in bond_uninit() (called by rtnl_link_unregister)
>> you have:
>> list_del(&bond->bond_list);
>> which is linked in the bond_net dev_list which is freed by
>> unregister_pernet_subsys.
>
> Yep, you're right, I've hit it recently with the patch applied, and now
> working on it. However, I still think that the idea of the patch is
> correct
> - i.e. to first disable sysfs (especially bonding_masters) and only
>  afterwards to start removing everything else. Or, obviously, to finally
> add normal locking to sysfs functions.
>
> Anyway, this corruption is really rare, so I'll wait for your fix next
> week.
>
Well, there's no need for that because you'll have to iterate over all
"net"s to do it properly. Since we already have code that does it
(unregister_pernet_subsys), my fix is to kill off any "left" bond
devices in bond_net_exit() _after_ destroying the bonding_masters
sysfs entry for that net. This way we preserve the code structure and avoid
duplicating a loop over all nets.
This is the fix we discussed a week ago.
I'd be happy to hear any comments on it before posting :-)

Of course stopping the whole bonding sysfs handling is also an alternative.

Re: [PATCH] bonding: remove sysfs before removing devices

[Veaceslav Falico]

On Sat, Apr 06, 2013 at 01:29:48AM +0200, Nikolay Aleksandrov wrote:
>On 04/06/2013 01:21 AM, Veaceslav Falico wrote:
>> On Sat, Apr 06, 2013 at 12:15:11AM +0200, Nikolay Aleksandrov wrote:
>>> Hi,
>>> Sorry for the late reply but I was travelling this week. In my
>>> opinion this
>>> fix is wrong because in bond_uninit() (called by rtnl_link_unregister)
>>> you have:
>>> list_del(&bond->bond_list);
>>> which is linked in the bond_net dev_list which is freed by
>>> unregister_pernet_subsys.
>>
>> Yep, you're right, I've hit it recently with the patch applied, and now
>> working on it. However, I still think that the idea of the patch is
>> correct
>> - i.e. to first disable sysfs (especially bonding_masters) and only
>>  afterwards to start removing everything else. Or, obviously, to finally
>> add normal locking to sysfs functions.
>>
>> Anyway, this corruption is really rare, so I'll wait for your fix next
>> week.
>>
>Well, there's no need for that because you'll have to iterate over all
>"net"s to do it properly. Since we already have code that does it
>(unregister_pernet_subsys), my fix is to kill off any "left" bond
>devices in bond_net_exit() _after_ destroying the bonding_masters
>sysfs entry for that net. This way we preserve the code structure and avoid
>duplicating a loop over all nets.

Yep, that was one approach that I wanted to do, however I didn't like the
idea to duplicate the device destruction - i.e. the rtnl_link_unregister()
already does that, and to re-delete them after sysfs gets out of the way
feeled wrong. However, I've also missed the net->dev_list part, so seems
like both approaches have drawbacks... Or did I miss something?

>This is the fix we discussed a week ago.

Not with me :). I didn't know of this bug by that time...

>I'd be happy to hear any comments on it before posting :-)
>
>Of course stopping the whole bonding sysfs handling is also an alternative.

I think it'd be the best way to do that.

1) We can't remove the net_ns before removing the devices cause they depend
on it (and I think it's quite a hack anyway now that I'm aware of dev_list).
2) We can't re-remove devices after sysfs deactivation, it's also a hack,
cause rtnl does the same thing.
3) We can't hold rtnl_mutex to do both rtnl and net_ns removal cause we can
deadlock in sysfs code (when it gets removed).
4) We can't remove the b_masters before all the logic cause it'll duplicate
the code for unregister_pernet_operations(), and will also look like a
hack (with looping through net_ns).

Out of all these, #2 (your option) looks the best - the least intrusive and
the easiest to read. However, I still think that it's the lesser evil, and
there must exist a way to do it correctly (like with #3, and avoid the
deadlock by restart_syscall() technique in bond_store_bonds() - however I
don't really know if it'll work.).

Anyway, I'd really like your feedback on these thoughts, and if nothing
better comes up - your patch :).

Thank you!

Re: [PATCH] bonding: remove sysfs before removing devices

[Nikolay Aleksandrov]

On 04/06/2013 03:49 AM, Veaceslav Falico wrote:
> On Sat, Apr 06, 2013 at 01:29:48AM +0200, Nikolay Aleksandrov wrote:
>> On 04/06/2013 01:21 AM, Veaceslav Falico wrote:
>>> On Sat, Apr 06, 2013 at 12:15:11AM +0200, Nikolay Aleksandrov wrote:
>>>> Hi,
>>>> Sorry for the late reply but I was travelling this week. In my
>>>> opinion this
>>>> fix is wrong because in bond_uninit() (called by rtnl_link_unregister)
>>>> you have:
>>>> list_del(&bond->bond_list);
>>>> which is linked in the bond_net dev_list which is freed by
>>>> unregister_pernet_subsys.
>>>
>>> Yep, you're right, I've hit it recently with the patch applied, and now
>>> working on it. However, I still think that the idea of the patch is
>>> correct
>>> - i.e. to first disable sysfs (especially bonding_masters) and only
>>>  afterwards to start removing everything else. Or, obviously, to
>>> finally
>>> add normal locking to sysfs functions.
>>>
>>> Anyway, this corruption is really rare, so I'll wait for your fix next
>>> week.
>>>
>> Well, there's no need for that because you'll have to iterate over all
>> "net"s to do it properly. Since we already have code that does it
>> (unregister_pernet_subsys), my fix is to kill off any "left" bond
>> devices in bond_net_exit() _after_ destroying the bonding_masters
>> sysfs entry for that net. This way we preserve the code structure and
>> avoid
>> duplicating a loop over all nets.
>
> Yep, that was one approach that I wanted to do, however I didn't like the
> idea to duplicate the device destruction - i.e. the
> rtnl_link_unregister()
> already does that, and to re-delete them after sysfs gets out of the way
> feeled wrong. However, I've also missed the net->dev_list part, so seems
> like both approaches have drawbacks... Or did I miss something?
>
Bridge code does it this way. See br_deinit() (br_netlink_fini() followed
by unregister pernet_subsys which again deletes anything left).
>> This is the fix we discussed a week ago.
>
> Not with me :). I didn't know of this bug by that time...
>
>> I'd be happy to hear any comments on it before posting :-)
>>
>> Of course stopping the whole bonding sysfs handling is also an
>> alternative.
>
> I think it'd be the best way to do that.
>
> 1) We can't remove the net_ns before removing the devices cause they
> depend
> on it (and I think it's quite a hack anyway now that I'm aware of
> dev_list).
> 2) We can't re-remove devices after sysfs deactivation, it's also a hack,
> cause rtnl does the same thing.
As I mentioned earlier see the bridge code for example.
> 3) We can't hold rtnl_mutex to do both rtnl and net_ns removal cause
> we can
> deadlock in sysfs code (when it gets removed).
> 4) We can't remove the b_masters before all the logic cause it'll
> duplicate
> the code for unregister_pernet_operations(), and will also look like a
> hack (with looping through net_ns).
>
Agreed.
> Out of all these, #2 (your option) looks the best - the least
> intrusive and
> the easiest to read. However, I still think that it's the lesser evil,
> and
> there must exist a way to do it correctly (like with #3, and avoid the
> deadlock by restart_syscall() technique in bond_store_bonds() - however I
> don't really know if it'll work.).
>
> Anyway, I'd really like your feedback on these thoughts, and if nothing
> better comes up - your patch :).
>
One more way would be to make a mechanism that prevents sysfs
bond handling while doing loading/unloading, it can also later be
used to fix other bugs that are present, but it might be tricky with
the loading case.
> Thank you!
Thank you for the input.