diff mbox

[net-next] net: vxlan: do not use vxlan_net before checking event type

Message ID 1389959706-30976-1-git-send-email-dborkman@redhat.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Daniel Borkmann Jan. 17, 2014, 11:55 a.m. UTC
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(-)

Comments

Cong Wang Jan. 17, 2014, 5:30 p.m. UTC | #1
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
Jesse Brandeburg Jan. 17, 2014, 6:20 p.m. UTC | #2
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
Daniel Borkmann Jan. 17, 2014, 6:32 p.m. UTC | #3
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
David Miller Jan. 18, 2014, 2:50 a.m. UTC | #4
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
Cong Wang Jan. 18, 2014, 3:50 a.m. UTC | #5
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
Eric Dumazet Jan. 18, 2014, 5:18 p.m. UTC | #6
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
Cong Wang Jan. 18, 2014, 5:57 p.m. UTC | #7
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
Daniel Borkmann Jan. 18, 2014, 7:47 p.m. UTC | #8
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
Cong Wang Jan. 18, 2014, 11:32 p.m. UTC | #9
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
Cong Wang Jan. 18, 2014, 11:38 p.m. UTC | #10
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
Cong Wang Jan. 18, 2014, 11:48 p.m. UTC | #11
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
Daniel Borkmann Jan. 19, 2014, 12:36 a.m. UTC | #12
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
Cong Wang Jan. 19, 2014, 12:50 a.m. UTC | #13
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
Eric Dumazet Jan. 19, 2014, 2:01 a.m. UTC | #14
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
Eric W. Biederman Jan. 20, 2014, 9:51 p.m. UTC | #15
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
Daniel Borkmann Jan. 20, 2014, 10:01 p.m. UTC | #16
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 mbox

Patch

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