/net/mpls/conf/ethX//input duplicate entry

/net/mpls/conf/ethX//input duplicate entry

[Scott Feldman]

I'm getting this dump_stack when reloading rocker driver.  Did some
sysctl MPLS nodes not get cleaned up on NETDEV_UNREGISTER?

Steps to repro: load rocker (on system) with rocker device, rmmod
rocker, and then modprobe rocker.  I doubt this is specific to rocker:
and re-registration of a netdev should hit it. I am using UDEV rules
to rename kernel's ethX to a different name.  Maybe that's what
tripped it up?


[   68.587713] sysctl duplicate entry: /net/mpls/conf/eth4//input
[   68.590015] CPU: 0 PID: 2944 Comm: modprobe Not tainted 4.1.0-rc7+ #35
[   68.591514] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.8.1-0-g4adadbd-20150316_085822-nilsson.home.kraxel.org
04/01/2014
[   68.594401]  ffff88000f521f00 ffff88000f73f848 ffffffff81443991
0000000000000006
[   68.596885]  0000000000000000 ffff88000f73f8a8 ffffffff811a4d5a
ffff88000f73f8b8
[   68.599328]  ffffffff81a9e400 0000000000000000 0000000400000020
ffff88000f73f8c8
[   68.620961] Call Trace:
[   68.621770]  [<ffffffff81443991>] dump_stack+0x4c/0x65
[   68.623147]  [<ffffffff811a4d5a>] __register_sysctl_table+0x414/0x426
[   68.624659]  [<ffffffff81432c17>] register_net_sysctl+0x10/0x12
[   68.626052]  [<ffffffff81435c85>] mpls_dev_notify+0xd6/0x26f
[   68.627403]  [<ffffffff814297fa>] ? packet_notifier+0x18c/0x19a
[   68.628821]  [<ffffffff81062337>] notifier_call_chain+0x3f/0x6c
[   68.630225]  [<ffffffff8106236d>] __raw_notifier_call_chain+0x9/0xb
[   68.631675]  [<ffffffff8106237e>] raw_notifier_call_chain+0xf/0x11
[   68.633146]  [<ffffffff8135eb6c>] call_netdevice_notifiers_info+0x4f/0x58
[   68.634695]  [<ffffffff8135ebb6>] call_netdevice_notifiers+0x11/0x13
[   68.636175]  [<ffffffff8136718b>] register_netdevice+0x2f6/0x365
[   68.637604]  [<ffffffff81367214>] register_netdev+0x1a/0x27
[   68.638962]  [<ffffffffa039d3f8>] rocker_probe+0x94d/0xc20 [rocker]
[   68.640424]  [<ffffffff81300001>] ? component_bind_all+0x5b/0x190
[   68.641861]  [<ffffffff812561fa>] local_pci_probe+0x38/0x7e
[   68.643204]  [<ffffffff8125630e>] pci_device_probe+0xce/0xf4
[   68.644579]  [<ffffffff81304312>] driver_probe_device+0xcc/0x23d
[   68.646220]  [<ffffffff81304483>] ? driver_probe_device+0x23d/0x23d
[   68.647679]  [<ffffffff813044d1>] __driver_attach+0x4e/0x6f
[   68.649051]  [<ffffffff813029d0>] bus_for_each_dev+0x5a/0x8c
[   68.650402]  [<ffffffff81303dc2>] driver_attach+0x19/0x1b
[   68.651773]  [<ffffffff81303afd>] bus_add_driver+0xf1/0x1d6
[   68.653305]  [<ffffffff81304c23>] driver_register+0x87/0xbe
[   68.654652]  [<ffffffff81255a74>] __pci_register_driver+0x5d/0x62
[   68.656087]  [<ffffffffa0378000>] ? 0xffffffffa0378000
[   68.657367]  [<ffffffffa0378039>] rocker_module_init+0x39/0x1000 [rocker]
[   68.658900]  [<ffffffffa0378000>] ? 0xffffffffa0378000
[   68.660178]  [<ffffffffa0378000>] ? 0xffffffffa0378000
[   68.661461]  [<ffffffff8100030d>] do_one_initcall+0xe9/0x186
[   68.662813]  [<ffffffff8113d228>] ? kmem_cache_alloc_trace+0xee/0x100
[   68.664316]  [<ffffffff814418bf>] do_init_module+0x5b/0x1ca
[   68.665666]  [<ffffffff810b2364>] load_module+0x1cf2/0x1ee1
[   68.667002]  [<ffffffff810aefed>] ? __module_get+0x29/0x29
[   68.668336]  [<ffffffff8144a4e7>] ? retint_kernel+0x10/0x10
[   68.669694]  [<ffffffff810b2666>] SyS_init_module+0x113/0x124
[   68.671062]  [<ffffffff81449957>] system_call_fastpath+0x12/0x6f

Re: /net/mpls/conf/ethX//input duplicate entry

[roopa]

On 6/10/15, 1:43 PM, Scott Feldman wrote:
> I'm getting this dump_stack when reloading rocker driver.  Did some
> sysctl MPLS nodes not get cleaned up on NETDEV_UNREGISTER?
>
> Steps to repro: load rocker (on system) with rocker device, rmmod
> rocker, and then modprobe rocker.  I doubt this is specific to rocker:
> and re-registration of a netdev should hit it. I am using UDEV rules
> to rename kernel's ethX to a different name.  Maybe that's what
> tripped it up?
>
On a quick look, wondering if this is because mpls driver does not seem 
to do a unregister and re-register sysctl
on device name change.

diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7b3f732..ec21a5d 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -564,6 +564,14 @@ static int mpls_dev_notify(struct notifier_block 
*this, unsigned long event,
         case NETDEV_UNREGISTER:
                 mpls_ifdown(dev);
                 break;
+       case NETDEV_CHANGENAME:
+               mpls_ifdown(dev);
+               if ((dev->type == ARPHRD_ETHER) ||
+                   (dev->type == ARPHRD_LOOPBACK)) {
+                       mdev = mpls_add_dev(dev);
+                       if (IS_ERR(mdev))
+                               return notifier_from_errno(PTR_ERR(mdev));
+               }
         }
         return NOTIFY_OK;
  }

Re: /net/mpls/conf/ethX//input duplicate entry

[Scott Feldman]

On Wed, Jun 10, 2015 at 2:58 PM, roopa <roopa@cumulusnetworks.com> wrote:
> On 6/10/15, 1:43 PM, Scott Feldman wrote:
>>
>> I'm getting this dump_stack when reloading rocker driver.  Did some
>> sysctl MPLS nodes not get cleaned up on NETDEV_UNREGISTER?
>>
>> Steps to repro: load rocker (on system) with rocker device, rmmod
>> rocker, and then modprobe rocker.  I doubt this is specific to rocker:
>> and re-registration of a netdev should hit it. I am using UDEV rules
>> to rename kernel's ethX to a different name.  Maybe that's what
>> tripped it up?
>>
> On a quick look, wondering if this is because mpls driver does not seem to
> do a unregister and re-register sysctl
> on device name change.
>
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index 7b3f732..ec21a5d 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -564,6 +564,14 @@ static int mpls_dev_notify(struct notifier_block *this,
> unsigned long event,
>         case NETDEV_UNREGISTER:
>                 mpls_ifdown(dev);
>                 break;
> +       case NETDEV_CHANGENAME:
> +               mpls_ifdown(dev);
> +               if ((dev->type == ARPHRD_ETHER) ||
> +                   (dev->type == ARPHRD_LOOPBACK)) {
> +                       mdev = mpls_add_dev(dev);
> +                       if (IS_ERR(mdev))
> +                               return notifier_from_errno(PTR_ERR(mdev));
> +               }
>         }
>         return NOTIFY_OK;
>  }

Roopa, I tested this patch and problem goes away.  (It's missing a
break statement, BTW).  I didn't look into the correctness of the
patch, but at first glance it seems liek the right thing to do.  Maybe
breaking out the renaming portions into sub-functions could keep the
work done in NETDEV_CHANGENAME to a minimum.

Are you sending official fix?

-scott

Re: /net/mpls/conf/ethX//input duplicate entry

[Robert Shearman]

On 11/06/15 00:23, Scott Feldman wrote:
> On Wed, Jun 10, 2015 at 2:58 PM, roopa <roopa@cumulusnetworks.com> wrote:
>> On 6/10/15, 1:43 PM, Scott Feldman wrote:
>>>
>>> I'm getting this dump_stack when reloading rocker driver.  Did some
>>> sysctl MPLS nodes not get cleaned up on NETDEV_UNREGISTER?
>>>
>>> Steps to repro: load rocker (on system) with rocker device, rmmod
>>> rocker, and then modprobe rocker.  I doubt this is specific to rocker:
>>> and re-registration of a netdev should hit it. I am using UDEV rules
>>> to rename kernel's ethX to a different name.  Maybe that's what
>>> tripped it up?
>>>
>> On a quick look, wondering if this is because mpls driver does not seem to
>> do a unregister and re-register sysctl
>> on device name change.

Mea culpa. Thanks for looking at this.

>>
>> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
>> index 7b3f732..ec21a5d 100644
>> --- a/net/mpls/af_mpls.c
>> +++ b/net/mpls/af_mpls.c
>> @@ -564,6 +564,14 @@ static int mpls_dev_notify(struct notifier_block *this,
>> unsigned long event,
>>          case NETDEV_UNREGISTER:
>>                  mpls_ifdown(dev);
>>                  break;
>> +       case NETDEV_CHANGENAME:
>> +               mpls_ifdown(dev);
>> +               if ((dev->type == ARPHRD_ETHER) ||
>> +                   (dev->type == ARPHRD_LOOPBACK)) {
>> +                       mdev = mpls_add_dev(dev);
>> +                       if (IS_ERR(mdev))
>> +                               return notifier_from_errno(PTR_ERR(mdev));
>> +               }
>>          }
>>          return NOTIFY_OK;
>>   }
>
> Roopa, I tested this patch and problem goes away.  (It's missing a
> break statement, BTW).  I didn't look into the correctness of the
> patch, but at first glance it seems liek the right thing to do.  Maybe
> breaking out the renaming portions into sub-functions could keep the
> work done in NETDEV_CHANGENAME to a minimum.

I agree that breaking out the sysctl registration/unregistration is a 
good idea to not have to do more work than is necessary, and to avoid 
unintended consequences (like routes using the interface being made 
unusable).

>
> Are you sending official fix?

Roopa, let me know if you'd like me to carry this forward.

Thanks,
Rob

Re: /net/mpls/conf/ethX//input duplicate entry

[roopa]

On 6/11/15, 5:55 AM, Robert Shearman wrote:
> On 11/06/15 00:23, Scott Feldman wrote:
>> On Wed, Jun 10, 2015 at 2:58 PM, roopa <roopa@cumulusnetworks.com> 
>> wrote:
>>> On 6/10/15, 1:43 PM, Scott Feldman wrote:
>>>>
>>>> I'm getting this dump_stack when reloading rocker driver. Did some
>>>> sysctl MPLS nodes not get cleaned up on NETDEV_UNREGISTER?
>>>>
>>>> Steps to repro: load rocker (on system) with rocker device, rmmod
>>>> rocker, and then modprobe rocker.  I doubt this is specific to rocker:
>>>> and re-registration of a netdev should hit it. I am using UDEV rules
>>>> to rename kernel's ethX to a different name.  Maybe that's what
>>>> tripped it up?
>>>>
>>> On a quick look, wondering if this is because mpls driver does not 
>>> seem to
>>> do a unregister and re-register sysctl
>>> on device name change.
>
> Mea culpa. Thanks for looking at this.
>
>>>
>>> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
>>> index 7b3f732..ec21a5d 100644
>>> --- a/net/mpls/af_mpls.c
>>> +++ b/net/mpls/af_mpls.c
>>> @@ -564,6 +564,14 @@ static int mpls_dev_notify(struct 
>>> notifier_block *this,
>>> unsigned long event,
>>>          case NETDEV_UNREGISTER:
>>>                  mpls_ifdown(dev);
>>>                  break;
>>> +       case NETDEV_CHANGENAME:
>>> +               mpls_ifdown(dev);
>>> +               if ((dev->type == ARPHRD_ETHER) ||
>>> +                   (dev->type == ARPHRD_LOOPBACK)) {
>>> +                       mdev = mpls_add_dev(dev);
>>> +                       if (IS_ERR(mdev))
>>> +                               return 
>>> notifier_from_errno(PTR_ERR(mdev));
>>> +               }
>>>          }
>>>          return NOTIFY_OK;
>>>   }
>>
>> Roopa, I tested this patch and problem goes away.  (It's missing a
>> break statement, BTW).  I didn't look into the correctness of the
>> patch, but at first glance it seems liek the right thing to do. Maybe
>> breaking out the renaming portions into sub-functions could keep the
>> work done in NETDEV_CHANGENAME to a minimum.
>
> I agree that breaking out the sysctl registration/unregistration is a 
> good idea to not have to do more work than is necessary, and to avoid 
> unintended consequences (like routes using the interface being made 
> unusable).
>
>>
>> Are you sending official fix?
>
> Roopa, let me know if you'd like me to carry this forward.
sorry for the delay in getting back. Its going to be a busy day for me. 
And i don't know the side-effects of my changes yet.
you probably have a better handle on this. So if you can, yes please 
carry this forward.

thanks!.

[PATCH net] mpls: handle device renames for per-device sysctls

[Robert Shearman]

If a device is renamed and the original name is subsequently reused
for a new device, the following warning is generated:

sysctl duplicate entry: /net/mpls/conf/veth0//input
CPU: 3 PID: 1379 Comm: ip Not tainted 4.1.0-rc4+ #20
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
 0000000000000000 0000000000000000 ffffffff81566aaf 0000000000000000
 ffffffff81236279 ffff88002f7d7f00 0000000000000000 ffff88000db336d8
 ffff88000db33698 0000000000000005 ffff88002e046000 ffff8800168c9280
Call Trace:
 [<ffffffff81566aaf>] ? dump_stack+0x40/0x50
 [<ffffffff81236279>] ? __register_sysctl_table+0x289/0x5a0
 [<ffffffffa051a24f>] ? mpls_dev_notify+0x1ff/0x300 [mpls_router]
 [<ffffffff8108db7f>] ? notifier_call_chain+0x4f/0x70
 [<ffffffff81470e72>] ? register_netdevice+0x2b2/0x480
 [<ffffffffa0524748>] ? veth_newlink+0x178/0x2d3 [veth]
 [<ffffffff8147f84c>] ? rtnl_newlink+0x73c/0x8e0
 [<ffffffff8147f27a>] ? rtnl_newlink+0x16a/0x8e0
 [<ffffffff81459ff2>] ? __kmalloc_reserve.isra.30+0x32/0x90
 [<ffffffff8147ccfd>] ? rtnetlink_rcv_msg+0x8d/0x250
 [<ffffffff8145b027>] ? __alloc_skb+0x47/0x1f0
 [<ffffffff8149badb>] ? __netlink_lookup+0xab/0xe0
 [<ffffffff8147cc70>] ? rtnetlink_rcv+0x30/0x30
 [<ffffffff8149e7a0>] ? netlink_rcv_skb+0xb0/0xd0
 [<ffffffff8147cc64>] ? rtnetlink_rcv+0x24/0x30
 [<ffffffff8149df17>] ? netlink_unicast+0x107/0x1a0
 [<ffffffff8149e4be>] ? netlink_sendmsg+0x50e/0x630
 [<ffffffff8145209c>] ? sock_sendmsg+0x3c/0x50
 [<ffffffff81452beb>] ? ___sys_sendmsg+0x27b/0x290
 [<ffffffff811bd258>] ? mem_cgroup_try_charge+0x88/0x110
 [<ffffffff811bd5b6>] ? mem_cgroup_commit_charge+0x56/0xa0
 [<ffffffff811d7700>] ? do_filp_open+0x30/0xa0
 [<ffffffff8145336e>] ? __sys_sendmsg+0x3e/0x80
 [<ffffffff8156c3f2>] ? system_call_fastpath+0x16/0x75

Fix this by unregistering the previous sysctl table (registered for
the path containing the original device name) and re-registering the
table for the path containing the new device name.

Fixes: 37bde79979c3 ("mpls: Per-device enabling of packet input")
Reported-by: Scott Feldman <sfeldma@gmail.com>
Signed-off-by: Robert Shearman <rshearma@brocade.com>
---
 net/mpls/af_mpls.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index bff427f31924..1f93a5978f2a 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -564,6 +564,17 @@ static int mpls_dev_notify(struct notifier_block *this, unsigned long event,
 	case NETDEV_UNREGISTER:
 		mpls_ifdown(dev);
 		break;
+	case NETDEV_CHANGENAME:
+		mdev = mpls_dev_get(dev);
+		if (mdev) {
+			int err;
+
+			mpls_dev_sysctl_unregister(mdev);
+			err = mpls_dev_sysctl_register(dev, mdev);
+			if (err)
+				return notifier_from_errno(err);
+		}
+		break;
 	}
 	return NOTIFY_OK;
 }
-- 
2.1.4

Re: [PATCH net] mpls: handle device renames for per-device sysctls

[David Miller]

From: Robert Shearman <rshearma@brocade.com>
Date: Thu, 11 Jun 2015 19:58:26 +0100

> If a device is renamed and the original name is subsequently reused
> for a new device, the following warning is generated:
 ...
> Fix this by unregistering the previous sysctl table (registered for
> the path containing the original device name) and re-registering the
> table for the path containing the new device name.
> 
> Fixes: 37bde79979c3 ("mpls: Per-device enabling of packet input")
> Reported-by: Scott Feldman <sfeldma@gmail.com>
> Signed-off-by: Robert Shearman <rshearma@brocade.com>

Applied, thanks Robert.

Re: /net/mpls/conf/ethX//input duplicate entry

[Eric W. Biederman]

Robert Shearman <rshearma@brocade.com> writes:

> On 11/06/15 00:23, Scott Feldman wrote:
>> On Wed, Jun 10, 2015 at 2:58 PM, roopa <roopa@cumulusnetworks.com> wrote:
>>> On 6/10/15, 1:43 PM, Scott Feldman wrote:
>>>>
>>>> I'm getting this dump_stack when reloading rocker driver.  Did some
>>>> sysctl MPLS nodes not get cleaned up on NETDEV_UNREGISTER?
>>>>
>>>> Steps to repro: load rocker (on system) with rocker device, rmmod
>>>> rocker, and then modprobe rocker.  I doubt this is specific to rocker:
>>>> and re-registration of a netdev should hit it. I am using UDEV rules
>>>> to rename kernel's ethX to a different name.  Maybe that's what
>>>> tripped it up?
>>>>
>>> On a quick look, wondering if this is because mpls driver does not seem to
>>> do a unregister and re-register sysctl
>>> on device name change.
>
> Mea culpa. Thanks for looking at this.
>
>>>
>>> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
>>> index 7b3f732..ec21a5d 100644
>>> --- a/net/mpls/af_mpls.c
>>> +++ b/net/mpls/af_mpls.c
>>> @@ -564,6 +564,14 @@ static int mpls_dev_notify(struct notifier_block *this,
>>> unsigned long event,
>>>          case NETDEV_UNREGISTER:
>>>                  mpls_ifdown(dev);
>>>                  break;
>>> +       case NETDEV_CHANGENAME:
>>> +               mpls_ifdown(dev);
>>> +               if ((dev->type == ARPHRD_ETHER) ||
>>> +                   (dev->type == ARPHRD_LOOPBACK)) {
>>> +                       mdev = mpls_add_dev(dev);
>>> +                       if (IS_ERR(mdev))
>>> +                               return notifier_from_errno(PTR_ERR(mdev));
>>> +               }
>>>          }
>>>          return NOTIFY_OK;
>>>   }
>>
>> Roopa, I tested this patch and problem goes away.  (It's missing a
>> break statement, BTW).  I didn't look into the correctness of the
>> patch, but at first glance it seems liek the right thing to do.  Maybe
>> breaking out the renaming portions into sub-functions could keep the
>> work done in NETDEV_CHANGENAME to a minimum.
>
> I agree that breaking out the sysctl registration/unregistration is a good idea
> to not have to do more work than is necessary, and to avoid unintended
> consequences (like routes using the interface being made unusable).

Yes.  It needs to be something like:

+       case NETDEV_CHANGENAME:
+		mdev = mpls_dev_get(dev);
+		if (mdev) {
+			mpls_dev_sysctl_unregister(mdev);
+			mpls_dev_sysctl_register(dev, mdev);
+		}
+		break;

Eric