netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
@ 2014-01-17 11:55 Daniel Borkmann
  2014-01-17 17:30 ` Cong Wang
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Daniel Borkmann @ 2014-01-17 11:55 UTC (permalink / raw)
  To: davem; +Cc: netdev, Eric W. Biederman, Jesse Brandeburg, Cong Wang

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.

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;
 }
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-17 11:55 [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type Daniel Borkmann
@ 2014-01-17 17:30 ` Cong Wang
  2014-01-17 18:32   ` Daniel Borkmann
  2014-01-17 18:20 ` Jesse Brandeburg
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2014-01-17 17:30 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: David Miller, Linux Kernel Network Developers, Eric W. Biederman,
	Jesse Brandeburg

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-17 11:55 [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type Daniel Borkmann
  2014-01-17 17:30 ` Cong Wang
@ 2014-01-17 18:20 ` Jesse Brandeburg
  2014-01-18  2:50 ` David Miller
  2014-01-20 21:51 ` Eric W. Biederman
  3 siblings, 0 replies; 17+ messages in thread
From: Jesse Brandeburg @ 2014-01-17 18:20 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: davem, netdev, Eric W. Biederman, Cong Wang

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>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-17 17:30 ` Cong Wang
@ 2014-01-17 18:32   ` Daniel Borkmann
  2014-01-18  3:50     ` Cong Wang
  0 siblings, 1 reply; 17+ messages in thread
From: Daniel Borkmann @ 2014-01-17 18:32 UTC (permalink / raw)
  To: Cong Wang
  Cc: David Miller, Linux Kernel Network Developers, Eric W. Biederman,
	Jesse Brandeburg

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-17 11:55 [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type Daniel Borkmann
  2014-01-17 17:30 ` Cong Wang
  2014-01-17 18:20 ` Jesse Brandeburg
@ 2014-01-18  2:50 ` David Miller
  2014-01-20 21:51 ` Eric W. Biederman
  3 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2014-01-18  2:50 UTC (permalink / raw)
  To: dborkman; +Cc: netdev, ebiederm, jesse.brandeburg, xiyou.wangcong

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-17 18:32   ` Daniel Borkmann
@ 2014-01-18  3:50     ` Cong Wang
  2014-01-18 17:18       ` Eric Dumazet
  0 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2014-01-18  3:50 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: David Miller, Linux Kernel Network Developers, Eric W. Biederman,
	Jesse Brandeburg

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-18  3:50     ` Cong Wang
@ 2014-01-18 17:18       ` Eric Dumazet
  2014-01-18 17:57         ` Cong Wang
  0 siblings, 1 reply; 17+ messages in thread
From: Eric Dumazet @ 2014-01-18 17:18 UTC (permalink / raw)
  To: Cong Wang
  Cc: Daniel Borkmann, David Miller, Linux Kernel Network Developers,
	Eric W. Biederman, Jesse Brandeburg

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-18 17:18       ` Eric Dumazet
@ 2014-01-18 17:57         ` Cong Wang
  2014-01-18 19:47           ` Daniel Borkmann
       [not found]           ` <1390072047.31367.543.camel@edumazet-glaptop2.roam.corp.google.com>
  0 siblings, 2 replies; 17+ messages in thread
From: Cong Wang @ 2014-01-18 17:57 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Daniel Borkmann, David Miller, Linux Kernel Network Developers,
	Eric W. Biederman, Jesse Brandeburg

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-18 17:57         ` Cong Wang
@ 2014-01-18 19:47           ` Daniel Borkmann
  2014-01-18 23:32             ` Cong Wang
       [not found]           ` <1390072047.31367.543.camel@edumazet-glaptop2.roam.corp.google.com>
  1 sibling, 1 reply; 17+ messages in thread
From: Daniel Borkmann @ 2014-01-18 19:47 UTC (permalink / raw)
  To: Cong Wang
  Cc: Eric Dumazet, David Miller, Linux Kernel Network Developers,
	Eric W. Biederman, Jesse Brandeburg

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-18 19:47           ` Daniel Borkmann
@ 2014-01-18 23:32             ` Cong Wang
  2014-01-18 23:48               ` Cong Wang
  0 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2014-01-18 23:32 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Eric Dumazet, David Miller, Linux Kernel Network Developers,
	Eric W. Biederman, Jesse Brandeburg

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
       [not found]           ` <1390072047.31367.543.camel@edumazet-glaptop2.roam.corp.google.com>
@ 2014-01-18 23:38             ` Cong Wang
  2014-01-19  2:01               ` Eric Dumazet
  0 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2014-01-18 23:38 UTC (permalink / raw)
  To: Eric Dumazet, Linux Kernel Network Developers

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-18 23:32             ` Cong Wang
@ 2014-01-18 23:48               ` Cong Wang
  2014-01-19  0:36                 ` Daniel Borkmann
  0 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2014-01-18 23:48 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Eric Dumazet, David Miller, Linux Kernel Network Developers,
	Eric W. Biederman, Jesse Brandeburg

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-18 23:48               ` Cong Wang
@ 2014-01-19  0:36                 ` Daniel Borkmann
  2014-01-19  0:50                   ` Cong Wang
  0 siblings, 1 reply; 17+ messages in thread
From: Daniel Borkmann @ 2014-01-19  0:36 UTC (permalink / raw)
  To: Cong Wang
  Cc: Eric Dumazet, David Miller, Linux Kernel Network Developers,
	Eric W. Biederman, Jesse Brandeburg

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-19  0:36                 ` Daniel Borkmann
@ 2014-01-19  0:50                   ` Cong Wang
  0 siblings, 0 replies; 17+ messages in thread
From: Cong Wang @ 2014-01-19  0:50 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Eric Dumazet, David Miller, Linux Kernel Network Developers,
	Eric W. Biederman, Jesse Brandeburg

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-18 23:38             ` Cong Wang
@ 2014-01-19  2:01               ` Eric Dumazet
  0 siblings, 0 replies; 17+ messages in thread
From: Eric Dumazet @ 2014-01-19  2:01 UTC (permalink / raw)
  To: Cong Wang; +Cc: Linux Kernel Network Developers

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-17 11:55 [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type Daniel Borkmann
                   ` (2 preceding siblings ...)
  2014-01-18  2:50 ` David Miller
@ 2014-01-20 21:51 ` Eric W. Biederman
  2014-01-20 22:01   ` Daniel Borkmann
  3 siblings, 1 reply; 17+ messages in thread
From: Eric W. Biederman @ 2014-01-20 21:51 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: davem, netdev, Jesse Brandeburg, Cong Wang

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;
>  }

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type
  2014-01-20 21:51 ` Eric W. Biederman
@ 2014-01-20 22:01   ` Daniel Borkmann
  0 siblings, 0 replies; 17+ messages in thread
From: Daniel Borkmann @ 2014-01-20 22:01 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: davem, netdev, Jesse Brandeburg, Cong Wang

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2014-01-20 22:01 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-17 11:55 [PATCH net-next] net: vxlan: do not use vxlan_net before checking event type Daniel Borkmann
2014-01-17 17:30 ` Cong Wang
2014-01-17 18:32   ` Daniel Borkmann
2014-01-18  3:50     ` Cong Wang
2014-01-18 17:18       ` Eric Dumazet
2014-01-18 17:57         ` Cong Wang
2014-01-18 19:47           ` Daniel Borkmann
2014-01-18 23:32             ` Cong Wang
2014-01-18 23:48               ` Cong Wang
2014-01-19  0:36                 ` Daniel Borkmann
2014-01-19  0:50                   ` Cong Wang
     [not found]           ` <1390072047.31367.543.camel@edumazet-glaptop2.roam.corp.google.com>
2014-01-18 23:38             ` Cong Wang
2014-01-19  2:01               ` Eric Dumazet
2014-01-17 18:20 ` Jesse Brandeburg
2014-01-18  2:50 ` David Miller
2014-01-20 21:51 ` Eric W. Biederman
2014-01-20 22:01   ` Daniel Borkmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).