Message ID | 1389959706-30976-1-git-send-email-dborkman@redhat.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Fri, Jan 17, 2014 at 3:55 AM, Daniel Borkmann <dborkman@redhat.com> wrote: > - if (event == NETDEV_UNREGISTER) > + if (event == NETDEV_UNREGISTER) { > + vn = net_generic(dev_net(dev), vxlan_net_id); > vxlan_handle_lowerdev_unregister(vn, dev); > + } There is no need to keep vxlan_handle_lowerdev_unregister(), it is too short. So, just use my patch. -- 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 Fri, 17 Jan 2014 12:55:06 +0100 Daniel Borkmann <dborkman@redhat.com> wrote: > Jesse Brandeburg reported that commit acaf4e70997f caused a panic > when adding a network namespace while vxlan module was present in > the system: I ran a quick test and the namespace issue no longer occurs once this patch is applied. As for this one vs Cong's, you guys can fight that out. Thanks for fixing this. Tested-by: Jesse Brandeburg <jesse.brandeburg@intel.com> -- 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 01/17/2014 06:30 PM, Cong Wang wrote: > On Fri, Jan 17, 2014 at 3:55 AM, Daniel Borkmann <dborkman@redhat.com> wrote: >> - if (event == NETDEV_UNREGISTER) >> + if (event == NETDEV_UNREGISTER) { >> + vn = net_generic(dev_net(dev), vxlan_net_id); >> vxlan_handle_lowerdev_unregister(vn, dev); >> + } > > There is no need to keep vxlan_handle_lowerdev_unregister(), > it is too short. So, just use my patch. If you want to do cleanups, whatever, I really don't care. You had your chance to complain about that when you reviewed the initial version ... it has nothing to do with the fix. -- 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: Daniel Borkmann <dborkman@redhat.com> Date: Fri, 17 Jan 2014 12:55:06 +0100 > Jesse Brandeburg reported that commit acaf4e70997f caused a panic > when adding a network namespace while vxlan module was present in > the system: ... > Apparently loopback device is being registered first and thus we > receive an event notification when vxlan_net is not ready. Hence, > when we call net_generic() and request vxlan_net_id, we seem to > access garbage at that point in time. In setup_net() where we set > up a newly allocated network namespace, we traverse the list of > pernet ops ... > > list_for_each_entry(ops, &pernet_list, list) { > error = ops_init(ops, net); > if (error < 0) > goto out_undo; > } > > ... and loopback_net_init() is invoked first here, so in the middle > of setup_net() we get this notification in vxlan. As currently we > only care about devices that unregister, move access through > net_generic() there. Fix is based on Cong Wang's proposal, but > only changes what is needed here. It sucks a bit as we only work > around the actual cure: right now it seems the only way to check if > a netns actually finished traversing all init ops would be to check > if it's part of net_namespace_list. But that I find quite expensive > each time we go through a notifier callback. Anyway, did a couple > of tests and it seems good for now. > > Fixes: acaf4e70997f ("net: vxlan: when lower dev unregisters remove vxlan dev as well") > Reported-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > Cc: "Eric W. Biederman" <ebiederm@xmission.com> > Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> Applied, 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 Fri, Jan 17, 2014 at 10:32 AM, Daniel Borkmann <dborkman@redhat.com> wrote: > > > If you want to do cleanups, whatever, I really don't care. > You had your chance to complain about that when you reviewed > the initial version ... it has nothing to do with the fix. This is not for stable, as long as it doesn't harm the readability we are free to do any cleanup's. If unsure, check Eric's patch for tunnel dst cache. BTW, I am the original author of the patch, you just updated it *trivially* and set yourself as the author. :) I don't mind, but remember that this may be not appropriate for others. At very least I didn't and don't do this myself. -- 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 Fri, 2014-01-17 at 19:50 -0800, Cong Wang wrote: > On Fri, Jan 17, 2014 at 10:32 AM, Daniel Borkmann <dborkman@redhat.com> wrote: > > > > > > If you want to do cleanups, whatever, I really don't care. > > You had your chance to complain about that when you reviewed > > the initial version ... it has nothing to do with the fix. > > This is not for stable, as long as it doesn't harm the readability > we are free to do any cleanup's. > > If unsure, check Eric's patch for tunnel dst cache. > > BTW, I am the original author of the patch, you just updated > it *trivially* and set yourself as the author. :) I don't mind, but > remember that this may be not appropriate for others. At > very least I didn't and don't do this myself. Hmm... Daniel mentioned in the changelog you wrote the initial patch, and you are credited as the author of the patch, since he kept your "Signed-off-by: ..." as the first one. Quite frankly, keeping vxlan_handle_lowerdev_unregister() was the right choice. Stop thinking that a function needs to be used more than once to have the right to exist. Splitting code in small parts ease readability and code reuse/refactor, this should be obvious to you. -- 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 Sat, Jan 18, 2014 at 9:18 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Fri, 2014-01-17 at 19:50 -0800, Cong Wang wrote: >> On Fri, Jan 17, 2014 at 10:32 AM, Daniel Borkmann <dborkman@redhat.com> wrote: >> > >> > >> > If you want to do cleanups, whatever, I really don't care. >> > You had your chance to complain about that when you reviewed >> > the initial version ... it has nothing to do with the fix. >> >> This is not for stable, as long as it doesn't harm the readability >> we are free to do any cleanup's. >> >> If unsure, check Eric's patch for tunnel dst cache. >> >> BTW, I am the original author of the patch, you just updated >> it *trivially* and set yourself as the author. :) I don't mind, but >> remember that this may be not appropriate for others. At >> very least I didn't and don't do this myself. > > > Hmm... Daniel mentioned in the changelog you wrote the initial patch, > and you are credited as the author of the patch, since he kept your > "Signed-off-by: ..." as the first one. Author == 'From: ...', you knew it, right? But WITHOUT even asking for my permission. I am sure this is not how we usually work. At least, why not ask me before doing anything? Why not give me a chance to response? > > Quite frankly, keeping vxlan_handle_lowerdev_unregister() was the right > choice. > > Stop thinking that a function needs to be used more than once to have > the right to exist. Splitting code in small parts ease readability and > code reuse/refactor, this should be obvious to you. > When did I say because that it is only used once? Please, stop guessing my mind. -- 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 01/18/2014 06:57 PM, Cong Wang wrote: > On Sat, Jan 18, 2014 at 9:18 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: >> On Fri, 2014-01-17 at 19:50 -0800, Cong Wang wrote: >>> On Fri, Jan 17, 2014 at 10:32 AM, Daniel Borkmann <dborkman@redhat.com> wrote: >>>> >>>> If you want to do cleanups, whatever, I really don't care. >>>> You had your chance to complain about that when you reviewed >>>> the initial version ... it has nothing to do with the fix. >>> >>> This is not for stable, as long as it doesn't harm the readability >>> we are free to do any cleanup's. >>> >>> If unsure, check Eric's patch for tunnel dst cache. >>> >>> BTW, I am the original author of the patch, you just updated >>> it *trivially* and set yourself as the author. :) I don't mind, but >>> remember that this may be not appropriate for others. At >>> very least I didn't and don't do this myself. >> >> Hmm... Daniel mentioned in the changelog you wrote the initial patch, >> and you are credited as the author of the patch, since he kept your >> "Signed-off-by: ..." as the first one. > > Author == 'From: ...', you knew it, right? > > But WITHOUT even asking for my permission. I am sure this is > not how we usually work. At least, why not ask me before doing > anything? Why not give me a chance to response? > >> Quite frankly, keeping vxlan_handle_lowerdev_unregister() was the right >> choice. >> >> Stop thinking that a function needs to be used more than once to have >> the right to exist. Splitting code in small parts ease readability and >> code reuse/refactor, this should be obvious to you. > > When did I say because that it is only used once? Please, stop guessing > my mind. Cong, I'm really tired of discussing this BS with you, and this is my last mail on this topic. You said "There is no need to keep vxlan_handle_lowerdev_unregister(), it is too short." I, however, think keeping vxlan_handle_lowerdev_unregister() is the right choice as it makes the code more readable, plus you clearly agreed with the code earlier as you've given your Reviewed-by tag. You even got your Fixes tag wrong and I do care that an actual fix for a bug has a bit more in-depth commit message telling what's going on. I think the message in the commit is equally important as the code itself, you should know. Maybe, I was just in the wrong timezone, but while waiting for a v2 and not having endless discussions about vxlan_handle_lowerdev_unregister(), I do care that this gets fixed asap! Clearly, it seems it was an honest mistake to do so. -- 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 Sat, Jan 18, 2014 at 11:47 AM, Daniel Borkmann <dborkman@redhat.com> wrote: > I do care that this gets fixed asap! Clearly, it seems it was an honest > mistake to do so. Why??? Even for stable, you don't have to be hurry. That's all. -- 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
Cc'ing netdev back... On Sat, Jan 18, 2014 at 11:07 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Daniel did the right thing and David agreed. > > And I agreed. If you mean the code, I don't even want to argue from the beginning. If you mean the author of the patch, it is obviously wrong. > > So if you want to fight, feel free, but its going to be really hard. > I see. Next time, I will pick up your patch, change a very minor issue, and *steal* it as mine (a.k.a ignoring From:). So that in the changelog your patch will become mine. You don't mind, since you already agreed above... -- 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 Sat, Jan 18, 2014 at 3:32 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Sat, Jan 18, 2014 at 11:47 AM, Daniel Borkmann <dborkman@redhat.com> wrote: >> I do care that this gets fixed asap! Clearly, it seems it was an honest >> mistake to do so. > > Why??? Even for stable, you don't have to be hurry. That's all. Our community builds on trust and corporations. You just break it by being rush for a getting a patch for your own behalf and potentially being irrespective to me and others. Even patches for stable don't worth this... Corporation is much more important than just rushing for being the author of the patch. I do understand you want to be the author, next time, just tell me before you do, I will let you be whatever you want (if I can). That's all. REPEAT: I don't mind who fixes it, I DO mind you did it without asking me first. -- 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 01/19/2014 12:48 AM, Cong Wang wrote: > I do understand you want to be the author, next time, just tell me > before you do, I will let you be whatever you want (if I can). > That's all. > > REPEAT: I don't mind who fixes it, I DO mind you did it without > asking me first. Cong, I truly do __not__ care who is what or who isn't. I do care that the code is fine. Sure, next time I'll ask, or better, just give feedback; sorry for how this went, it was not my intention. -- 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 Sat, Jan 18, 2014 at 4:36 PM, Daniel Borkmann <dborkman@redhat.com> wrote: > > Cong, I truly do __not__ care who is what or who isn't. I do care > that the code is fine. Sure, next time I'll ask, or better, just > give feedback; sorry for how this went, it was not my intention. Apologies accepted. Let's build a better community! 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 Sat, 2014-01-18 at 15:38 -0800, Cong Wang wrote: > Cc'ing netdev back... > > On Sat, Jan 18, 2014 at 11:07 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Daniel did the right thing and David agreed. > > > > And I agreed. > > If you mean the code, I don't even want to argue from the beginning. > > If you mean the author of the patch, it is obviously wrong. > > > > > So if you want to fight, feel free, but its going to be really hard. > > > > I see. > > Next time, I will pick up your patch, change a very minor issue, > and *steal* it as mine (a.k.a ignoring From:). So that in the changelog > your patch will become mine. > > You don't mind, since you already agreed above... You failed to the test I did with you. By adding netdev back to a mail I sent privately, you violated one very elementary rule of communication. This is a huge mistake. I will never send you again a private mail. -- 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
Daniel Borkmann <dborkman@redhat.com> writes: > Jesse Brandeburg reported that commit acaf4e70997f caused a panic > when adding a network namespace while vxlan module was present in > the system: > > [<ffffffff814d0865>] vxlan_lowerdev_event+0xf5/0x100 > [<ffffffff816e9e5d>] notifier_call_chain+0x4d/0x70 > [<ffffffff810912be>] __raw_notifier_call_chain+0xe/0x10 > [<ffffffff810912d6>] raw_notifier_call_chain+0x16/0x20 > [<ffffffff815d9610>] call_netdevice_notifiers_info+0x40/0x70 > [<ffffffff815d9656>] call_netdevice_notifiers+0x16/0x20 > [<ffffffff815e1bce>] register_netdevice+0x1be/0x3a0 > [<ffffffff815e1dce>] register_netdev+0x1e/0x30 > [<ffffffff814cb94a>] loopback_net_init+0x4a/0xb0 > [<ffffffffa016ed6e>] ? lockd_init_net+0x6e/0xb0 [lockd] > [<ffffffff815d6bac>] ops_init+0x4c/0x150 > [<ffffffff815d6d23>] setup_net+0x73/0x110 > [<ffffffff815d725b>] copy_net_ns+0x7b/0x100 > [<ffffffff81090e11>] create_new_namespaces+0x101/0x1b0 > [<ffffffff81090f45>] copy_namespaces+0x85/0xb0 > [<ffffffff810693d5>] copy_process.part.26+0x935/0x1500 > [<ffffffff811d5186>] ? mntput+0x26/0x40 > [<ffffffff8106a15c>] do_fork+0xbc/0x2e0 > [<ffffffff811b7f2e>] ? ____fput+0xe/0x10 > [<ffffffff81089c5c>] ? task_work_run+0xac/0xe0 > [<ffffffff8106a406>] SyS_clone+0x16/0x20 > [<ffffffff816ee689>] stub_clone+0x69/0x90 > [<ffffffff816ee329>] ? system_call_fastpath+0x16/0x1b > > Apparently loopback device is being registered first and thus we > receive an event notification when vxlan_net is not ready. Hence, > when we call net_generic() and request vxlan_net_id, we seem to > access garbage at that point in time. In setup_net() where we set > up a newly allocated network namespace, we traverse the list of > pernet ops ... > > list_for_each_entry(ops, &pernet_list, list) { > error = ops_init(ops, net); > if (error < 0) > goto out_undo; > } > > ... and loopback_net_init() is invoked first here, so in the middle > of setup_net() we get this notification in vxlan. As currently we > only care about devices that unregister, move access through > net_generic() there. Fix is based on Cong Wang's proposal, but > only changes what is needed here. It sucks a bit as we only work > around the actual cure: right now it seems the only way to check if > a netns actually finished traversing all init ops would be to check > if it's part of net_namespace_list. But that I find quite expensive > each time we go through a notifier callback. Anyway, did a couple > of tests and it seems good for now. I am not going to argue against this patch as an immediate bug fix but something smells here, that bears deeper investigation. It looks like the symptom is being patched rather than the actual problem. In particular net_generic(dev_net(dev), vxlan_net_id) is valid at the point that it is being called. As the pointers are allocated in copy_net_ns in net_alloc prior to setup_net being called. On the flip side it is the responsibility of code that uses both register_netdev_notifier and register_pernet_xxx to be ready to handle events from any namespace as soon as they happen. vxlan should be using register_pernet_subsys instead of register_pernet_device to ensure the vxlan_net structure is initialized before and cleaned up after all network devices in a given network namespace. The vlan devices with a similar problem already do this. So in summary. Something smells and I don't believe this patch fixes the underlying issue. Please take a deeper look into what vxlan is doing. Eric > Fixes: acaf4e70997f ("net: vxlan: when lower dev unregisters remove vxlan dev as well") > Reported-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > Cc: "Eric W. Biederman" <ebiederm@xmission.com> > Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> > --- > drivers/net/vxlan.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c > index a2dee80..d6ec71f 100644 > --- a/drivers/net/vxlan.c > +++ b/drivers/net/vxlan.c > @@ -2681,10 +2681,12 @@ static int vxlan_lowerdev_event(struct notifier_block *unused, > unsigned long event, void *ptr) > { > struct net_device *dev = netdev_notifier_info_to_dev(ptr); > - struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id); > + struct vxlan_net *vn; > > - if (event == NETDEV_UNREGISTER) > + if (event == NETDEV_UNREGISTER) { > + vn = net_generic(dev_net(dev), vxlan_net_id); > vxlan_handle_lowerdev_unregister(vn, dev); > + } > > return NOTIFY_DONE; > } -- 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 01/20/2014 10:51 PM, Eric W. Biederman wrote: ... > I am not going to argue against this patch as an immediate bug fix but > something smells here, that bears deeper investigation. It looks like > the symptom is being patched rather than the actual problem. > > In particular net_generic(dev_net(dev), vxlan_net_id) is valid at the > point that it is being called. As the pointers are allocated in > copy_net_ns in net_alloc prior to setup_net being called. > > On the flip side it is the responsibility of code that uses both > register_netdev_notifier and register_pernet_xxx to be ready to handle > events from any namespace as soon as they happen. vxlan should be using > register_pernet_subsys instead of register_pernet_device to ensure the > vxlan_net structure is initialized before and cleaned up after all > network devices in a given network namespace. The vlan devices with a > similar problem already do this. > > So in summary. Something smells and I don't believe this patch fixes > the underlying issue. Please take a deeper look into what vxlan is doing. Thanks for the input Eric! If no-one is faster than me, I'll try to look into it soon. -- 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
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index a2dee80..d6ec71f 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -2681,10 +2681,12 @@ static int vxlan_lowerdev_event(struct notifier_block *unused, unsigned long event, void *ptr) { struct net_device *dev = netdev_notifier_info_to_dev(ptr); - struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id); + struct vxlan_net *vn; - if (event == NETDEV_UNREGISTER) + if (event == NETDEV_UNREGISTER) { + vn = net_generic(dev_net(dev), vxlan_net_id); vxlan_handle_lowerdev_unregister(vn, dev); + } return NOTIFY_DONE; }