Fw: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore

Fw: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore

[Stephen Hemminger]

Begin forwarded message:

Date: Wed, 5 Feb 2014 04:36:03 -0800
From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
To: "stephen@networkplumber.org" <stephen@networkplumber.org>
Subject: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore


https://bugzilla.kernel.org/show_bug.cgi?id=70071

            Bug ID: 70071
           Summary: Sending netconsole messages over a bridged network
                    interface doesn't work anymore
           Product: Networking
           Version: 2.5
    Kernel Version: 3.12
          Hardware: All
                OS: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: Other
          Assignee: shemminger@linux-foundation.org
          Reporter: bvanassche@acm.org
        Regression: Yes

Apparently several users already ran into this issue. See e.g.:
http://thread.gmane.org/gmane.linux.network/288373
http://lists.suse.de/archive/opensuse-kernel/2014-01/msg00061.html

-- 
You are receiving this mail because:
You are the assignee for the bug.

Re: Fw: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore

[Cong Wang]

On Wed, Feb 5, 2014 at 1:15 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> Apparently several users already ran into this issue. See e.g.:
> http://thread.gmane.org/gmane.linux.network/288373
> http://lists.suse.de/archive/opensuse-kernel/2014-01/msg00061.html
>

I will take a look at this.

Re: Fw: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore

[Toshiaki Makita]

(2014/02/06 6:15), Stephen Hemminger wrote:
> 
> 
> Begin forwarded message:
> 
> Date: Wed, 5 Feb 2014 04:36:03 -0800
> From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
> To: "stephen@networkplumber.org" <stephen@networkplumber.org>
> Subject: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore

Reverting this commit seems to fix the problem.
93d8bf9fb8f3 ("bridge: cleanup netpoll code")

In the following code path, p->br->dev->npinfo is NULL at
br_netpoll_enable() because __netpoll_setup() assigns ndev->npinfo after
calling ndo_netpoll_setup().

  __netpoll_setup() -> br_netpoll_setup() -> br_netpoll_enable()

Thanks,
Toshiaki Makita

Re: Fw: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore

[Cong Wang]

On Wed, Feb 5, 2014 at 7:58 PM, Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
> (2014/02/06 6:15), Stephen Hemminger wrote:
>>
>>
>> Begin forwarded message:
>>
>> Date: Wed, 5 Feb 2014 04:36:03 -0800
>> From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
>> To: "stephen@networkplumber.org" <stephen@networkplumber.org>
>> Subject: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore
>
> Reverting this commit seems to fix the problem.
> 93d8bf9fb8f3 ("bridge: cleanup netpoll code")
>
> In the following code path, p->br->dev->npinfo is NULL at
> br_netpoll_enable() because __netpoll_setup() assigns ndev->npinfo after
> calling ndo_netpoll_setup().
>
>   __netpoll_setup() -> br_netpoll_setup() -> br_netpoll_enable()

Yeah, looks like we can just remove that incorrect 'if'.
Please test the following patch.

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index e4401a5..bb2b706 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -254,9 +254,6 @@ int br_netpoll_enable(struct net_bridge_port *p, gfp_t gfp)
        struct netpoll *np;
        int err;

-       if (!p->br->dev->npinfo)
-               return 0;
-
        np = kzalloc(sizeof(*p->np), gfp);
        if (!np)
                return -ENOMEM;

Re: Fw: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore

[Toshiaki Makita]

(2014/02/06 14:05), Cong Wang wrote:
> On Wed, Feb 5, 2014 at 7:58 PM, Toshiaki Makita
> <makita.toshiaki@lab.ntt.co.jp> wrote:
>> (2014/02/06 6:15), Stephen Hemminger wrote:
>>>
>>>
>>> Begin forwarded message:
>>>
>>> Date: Wed, 5 Feb 2014 04:36:03 -0800
>>> From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
>>> To: "stephen@networkplumber.org" <stephen@networkplumber.org>
>>> Subject: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore
>>
>> Reverting this commit seems to fix the problem.
>> 93d8bf9fb8f3 ("bridge: cleanup netpoll code")
>>
>> In the following code path, p->br->dev->npinfo is NULL at
>> br_netpoll_enable() because __netpoll_setup() assigns ndev->npinfo after
>> calling ndo_netpoll_setup().
>>
>>   __netpoll_setup() -> br_netpoll_setup() -> br_netpoll_enable()
> 
> Yeah, looks like we can just remove that incorrect 'if'.
> Please test the following patch.
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index e4401a5..bb2b706 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -254,9 +254,6 @@ int br_netpoll_enable(struct net_bridge_port *p, gfp_t gfp)
>         struct netpoll *np;
>         int err;
> 
> -       if (!p->br->dev->npinfo)
> -               return 0;
> -
>         np = kzalloc(sizeof(*p->np), gfp);
>         if (!np)
>                 return -ENOMEM;

Tested this patch with latest net-tree and netconsole works with it.
But I thinks it is better to move that "if" to br_add_if() because if we
don't have npinfo, we don't have to alloc p->np in br_add_if(), right?

Thanks,
Toshiaki Makita

Re: Fw: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore

[Cong Wang]

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

On Wed, Feb 5, 2014 at 9:44 PM, Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
> Tested this patch with latest net-tree and netconsole works with it.
> But I thinks it is better to move that "if" to br_add_if() because if we
> don't have npinfo, we don't have to alloc p->np in br_add_if(), right?
>

Hmm, we shouldn't handle netpoll-specific code inside br_add_if(),
we probably need the attached patch instead. Please give it
a try, or I will test it tomorrow, it's too late here.

Thanks!

[-- Attachment #2: br.diff --]
[-- Type: text/plain, Size: 1548 bytes --]

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index e4401a5..d9a9b0f 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -226,6 +226,33 @@ static void br_netpoll_cleanup(struct net_device *dev)
 		br_netpoll_disable(p);
 }
 
+static int __br_netpoll_enable(struct net_bridge_port *p, gfp_t gfp)
+{
+	struct netpoll *np;
+	int err;
+
+	np = kzalloc(sizeof(*p->np), gfp);
+	if (!np)
+		return -ENOMEM;
+
+	err = __netpoll_setup(np, p->dev, gfp);
+	if (err) {
+		kfree(np);
+		return err;
+	}
+
+	p->np = np;
+	return err;
+}
+
+int br_netpoll_enable(struct net_bridge_port *p, gfp_t gfp)
+{
+	if (!p->br->dev->npinfo)
+		return 0;
+
+	return __br_netpoll_enable(p, gfp);
+}
+
 static int br_netpoll_setup(struct net_device *dev, struct netpoll_info *ni,
 			    gfp_t gfp)
 {
@@ -236,7 +263,7 @@ static int br_netpoll_setup(struct net_device *dev, struct netpoll_info *ni,
 	list_for_each_entry(p, &br->port_list, list) {
 		if (!p->dev)
 			continue;
-		err = br_netpoll_enable(p, gfp);
+		err = __br_netpoll_enable(p, gfp);
 		if (err)
 			goto fail;
 	}
@@ -249,28 +276,6 @@ fail:
 	goto out;
 }
 
-int br_netpoll_enable(struct net_bridge_port *p, gfp_t gfp)
-{
-	struct netpoll *np;
-	int err;
-
-	if (!p->br->dev->npinfo)
-		return 0;
-
-	np = kzalloc(sizeof(*p->np), gfp);
-	if (!np)
-		return -ENOMEM;
-
-	err = __netpoll_setup(np, p->dev, gfp);
-	if (err) {
-		kfree(np);
-		return err;
-	}
-
-	p->np = np;
-	return err;
-}
-
 void br_netpoll_disable(struct net_bridge_port *p)
 {
 	struct netpoll *np = p->np;

Re: Fw: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore

[Toshiaki Makita]

(2014/02/06 15:49), Cong Wang wrote:
> On Wed, Feb 5, 2014 at 9:44 PM, Toshiaki Makita
> <makita.toshiaki@lab.ntt.co.jp> wrote:
>> Tested this patch with latest net-tree and netconsole works with it.
>> But I thinks it is better to move that "if" to br_add_if() because if we
>> don't have npinfo, we don't have to alloc p->np in br_add_if(), right?
>>
> 
> Hmm, we shouldn't handle netpoll-specific code inside br_add_if(),
> we probably need the attached patch instead. Please give it
> a try, or I will test it tomorrow, it's too late here.
> 

I tested whether netconsole works and whether it can be built
with/without CONFIG_NET_POLL_CONTROLLER, and couldn't find any problem.
This looks good to me.

Thanks,
Toshiaki Makita

Re: Fw: [Bug 70071] New: Sending netconsole messages over a bridged network interface doesn't work anymore

[Cong Wang]

On Thu, Feb 6, 2014 at 12:52 AM, Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
> (2014/02/06 15:49), Cong Wang wrote:
>> On Wed, Feb 5, 2014 at 9:44 PM, Toshiaki Makita
>> <makita.toshiaki@lab.ntt.co.jp> wrote:
>>> Tested this patch with latest net-tree and netconsole works with it.
>>> But I thinks it is better to move that "if" to br_add_if() because if we
>>> don't have npinfo, we don't have to alloc p->np in br_add_if(), right?
>>>
>>
>> Hmm, we shouldn't handle netpoll-specific code inside br_add_if(),
>> we probably need the attached patch instead. Please give it
>> a try, or I will test it tomorrow, it's too late here.
>>
>
> I tested whether netconsole works and whether it can be built
> with/without CONFIG_NET_POLL_CONTROLLER, and couldn't find any problem.
> This looks good to me.
>

Excellent! I will send it formally.

Thanks for testing!