* [PATCH net] ip[6]: don't register inet[6]dev when dev is down
@ 2017-07-05 15:57 Nicolas Dichtel
2017-07-05 22:43 ` Cong Wang
2017-07-08 10:02 ` David Miller
0 siblings, 2 replies; 10+ messages in thread
From: Nicolas Dichtel @ 2017-07-05 15:57 UTC (permalink / raw)
To: davem; +Cc: netdev, Hongjun Li, Nicolas Dichtel
From: Hongjun Li <hongjun.li@6wind.com>
When the netdev event NETDEV_CHANGEMTU is triggered, the inet[6]dev may be
created even if the corresponding device is down. This may lead to a leak
in the procfs when the device is unregistered, and finally trigger a
backtrace:
[ 114.724500] bond1 (unregistering): Released all slaves
[ 114.756700] remove_proc_entry: removing non-empty directory 'net/dev_snmp6', leaking at least 'dummy1'
[ 114.757335] ------------[ cut here ]------------
[ 114.757642] WARNING: CPU: 7 PID: 82 at fs/proc/generic.c:580 remove_proc_entry+0x100/0x113
[ 114.758165] Modules linked in: bonding dummy nfsv3 nfs_acl auth_rpcgss oid_registry nfsv4 nfs lockd grace sunrpc fscache i2c_piix4 pcspkr evdev tpm_tis tpm_tis_core tpm parport_pc parport serio_raw button loop ext4 crc16 jbd2 mbcache ide_cd_mod ide_gd_mod cdrom ata_generic ata_piix libata scsi_mod 8139too psmouse piix 8139cp i2c_core ide_core mii floppy
[ 114.760191] CPU: 7 PID: 82 Comm: kworker/u16:1 Not tainted 4.12.0-rc7+ #62
[ 114.760725] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[ 114.761377] Workqueue: netns cleanup_net
[ 114.761630] task: ffff88022fbec280 task.stack: ffffc900010cc000
[ 114.762008] RIP: 0010:remove_proc_entry+0x100/0x113
[ 114.762320] RSP: 0018:ffffc900010cfde0 EFLAGS: 00010282
[ 114.762653] RAX: 000000000000005a RBX: ffff88022ee73bd8 RCX: 0000000000000000
[ 114.763105] RDX: ffff88023fdd5ba8 RSI: ffff88023fdcde88 RDI: ffff88023fdcde88
[ 114.763555] RBP: ffffffff8177bb11 R08: 0000000000000000 R09: 0000000000000002
[ 114.764102] R10: ffffc900010cfd10 R11: ffffffff81a272ac R12: ffff880234010e78
[ 114.764673] R13: 00000000ffffffff R14: ffffc900010cfe48 R15: ffffffff818a6a98
[ 114.765123] FS: 0000000000000000(0000) GS:ffff88023fdc0000(0000) knlGS:0000000000000000
[ 114.765655] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 114.766030] CR2: 000055cd729a00c0 CR3: 000000022f1b7000 CR4: 00000000000006e0
[ 114.766496] Call Trace:
[ 114.766668] ? ipv6_proc_exit_net+0x2a/0x3e
[ 114.766945] ? ops_exit_list+0x3c/0x4b
[ 114.767194] ? cleanup_net+0x183/0x21f
[ 114.767447] ? process_one_work+0x171/0x2ab
[ 114.767724] ? worker_thread+0x194/0x242
[ 114.768028] ? cancel_delayed_work_sync+0xa/0xa
[ 114.768580] ? kthread+0x101/0x109
[ 114.768813] ? init_completion+0x1d/0x1d
[ 114.769073] ? sysfs_add_file_mode_ns+0xa4/0x145
[ 114.769371] ? __kernfs_create_file+0x25/0x9b
[ 114.769651] ? kernfs_new_node+0x2b/0x4b
[ 114.769908] ? ret_from_fork+0x25/0x30
[ 114.770150] Code: 30 4c 8d 80 85 00 00 00 48 8d 8b 85 00 00 00 48 c7 c6 80 1e 62 81 48 c7 c7 3e f6 73 81 31 c0 48 81 c2 85 00 00 00 e8 cb dc f5 ff <0f> ff 48 89 df e8 37 fd ff ff 48 83 c4 10 5b 5d 41 5c c3 41 55
[ 114.771343] ---[ end trace b41ba34b465c148a ]---
Here is a reproducer:
ip netns add foo
ip -n foo link add dummy1 type dummy
ip -n foo link add dummy2 type dummy
iodprobe bonding
ip -n foo link add bond1 type bond
ip -n foo link set dev bond1 down
ip -n foo addr add 10.10.10.1/24 dev bond1
ip -n foo link set dev bond1 up
ip -n foo link set dummy1 master bond1
ip -n foo link set dummy2 master bond1
ip -n foo link set bond1 mtu 1540
# Move slaves to init_net
ip -n foo link set dummy1 netns 1
ip -n foo link set dummy2 netns 1
# at this stage, wrong proc entries can be seen here:
ip netns exec foo ls /proc/sys/net/ipv4/conf/
ip netns exec foo ls /proc/net/dev_snmp6/
# the netns removal triggers the backtrace
ip netns del foo
When a device changes from one netns to another, it's first unregistered,
then the netns reference is updated and the dev is registered in the new
netns. Thus, when a slave moves to another netns, it is first
unregistered. This triggers a NETDEV_UNREGISTER event which is caught by
the bonding driver. The driver calls bond_release(), which calls
dev_set_mtu() and thus triggers NETDEV_CHANGEMTU (the device is still in
the old netns).
Signed-off-by: Hongjun Li <hongjun.li@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
net/ipv4/devinet.c | 3 ++-
net/ipv6/addrconf.c | 3 +++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index df14815a3b8c..fe94f48854d3 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1424,7 +1424,8 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
IN_DEV_CONF_SET(in_dev, NOXFRM, 1);
IN_DEV_CONF_SET(in_dev, NOPOLICY, 1);
}
- } else if (event == NETDEV_CHANGEMTU) {
+ } else if (event == NETDEV_CHANGEMTU &&
+ dev->flags & IFF_UP) {
/* Re-enabling IP */
if (inetdev_valid_mtu(dev->mtu))
in_dev = inetdev_init(dev);
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 1d2dbace42ff..034e74a309a0 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3395,6 +3395,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
break;
}
+ if (!(dev->flags & IFF_UP))
+ break;
+
/* allocate new idev */
idev = ipv6_add_dev(dev);
if (IS_ERR(idev))
--
2.13.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net] ip[6]: don't register inet[6]dev when dev is down
2017-07-05 15:57 [PATCH net] ip[6]: don't register inet[6]dev when dev is down Nicolas Dichtel
@ 2017-07-05 22:43 ` Cong Wang
2017-07-06 12:08 ` Nicolas Dichtel
2017-07-08 10:02 ` David Miller
1 sibling, 1 reply; 10+ messages in thread
From: Cong Wang @ 2017-07-05 22:43 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: David Miller, Linux Kernel Network Developers, Hongjun Li
[-- Attachment #1: Type: text/plain, Size: 733 bytes --]
On Wed, Jul 5, 2017 at 8:57 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> When a device changes from one netns to another, it's first unregistered,
> then the netns reference is updated and the dev is registered in the new
> netns. Thus, when a slave moves to another netns, it is first
> unregistered. This triggers a NETDEV_UNREGISTER event which is caught by
> the bonding driver. The driver calls bond_release(), which calls
> dev_set_mtu() and thus triggers NETDEV_CHANGEMTU (the device is still in
> the old netns).
I think in this special case it is meaningless to send
NETDEV_CHANGEMTU, because the device is dying within
its old netns, who still cares about its mtu change?
Something like the attached patch...
[-- Attachment #2: bond-change-mtu.diff --]
[-- Type: text/plain, Size: 3662 bytes --]
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 8ab6bdb..2971af1 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1820,7 +1820,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
*/
static int __bond_release_one(struct net_device *bond_dev,
struct net_device *slave_dev,
- bool all)
+ bool all, bool unregister)
{
struct bonding *bond = netdev_priv(bond_dev);
struct slave *slave, *oldcurrent;
@@ -1965,7 +1965,10 @@ static int __bond_release_one(struct net_device *bond_dev,
dev_set_mac_address(slave_dev, (struct sockaddr *)&ss);
}
- dev_set_mtu(slave_dev, slave->original_mtu);
+ if (unregister)
+ __dev_set_mtu(slave_dev, slave->original_mtu);
+ else
+ dev_set_mtu(slave_dev, slave->original_mtu);
slave_dev->priv_flags &= ~IFF_BONDING;
@@ -1977,7 +1980,7 @@ static int __bond_release_one(struct net_device *bond_dev,
/* A wrapper used because of ndo_del_link */
int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
{
- return __bond_release_one(bond_dev, slave_dev, false);
+ return __bond_release_one(bond_dev, slave_dev, false, false);
}
/* First release a slave and then destroy the bond if no more slaves are left.
@@ -1989,7 +1992,7 @@ static int bond_release_and_destroy(struct net_device *bond_dev,
struct bonding *bond = netdev_priv(bond_dev);
int ret;
- ret = bond_release(bond_dev, slave_dev);
+ ret = __bond_release_one(bond_dev, slave_dev, false, true);
if (ret == 0 && !bond_has_slaves(bond)) {
bond_dev->priv_flags |= IFF_DISABLE_NETPOLL;
netdev_info(bond_dev, "Destroying bond %s\n",
@@ -3060,7 +3063,7 @@ static int bond_slave_netdev_event(unsigned long event,
if (bond_dev->type != ARPHRD_ETHER)
bond_release_and_destroy(bond_dev, slave_dev);
else
- bond_release(bond_dev, slave_dev);
+ __bond_release_one(bond_dev, slave_dev, false, true);
break;
case NETDEV_UP:
case NETDEV_CHANGE:
@@ -4257,7 +4260,7 @@ static void bond_uninit(struct net_device *bond_dev)
/* Release the bonded slaves */
bond_for_each_slave(bond, slave, iter)
- __bond_release_one(bond_dev, slave->dev, true);
+ __bond_release_one(bond_dev, slave->dev, true, true);
netdev_info(bond_dev, "Released all slaves\n");
arr = rtnl_dereference(bond->slave_arr);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4ed952c..e4030db 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3287,6 +3287,7 @@ void __dev_notify_flags(struct net_device *, unsigned int old_flags,
int dev_change_name(struct net_device *, const char *);
int dev_set_alias(struct net_device *, const char *, size_t);
int dev_change_net_namespace(struct net_device *, struct net *, const char *);
+int __dev_set_mtu(struct net_device *, int);
int dev_set_mtu(struct net_device *, int);
void dev_set_group(struct net_device *, int);
int dev_set_mac_address(struct net_device *, struct sockaddr *);
diff --git a/net/core/dev.c b/net/core/dev.c
index 416137c..e567de8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6688,7 +6688,7 @@ int dev_change_flags(struct net_device *dev, unsigned int flags)
}
EXPORT_SYMBOL(dev_change_flags);
-static int __dev_set_mtu(struct net_device *dev, int new_mtu)
+int __dev_set_mtu(struct net_device *dev, int new_mtu)
{
const struct net_device_ops *ops = dev->netdev_ops;
@@ -6698,6 +6698,7 @@ static int __dev_set_mtu(struct net_device *dev, int new_mtu)
dev->mtu = new_mtu;
return 0;
}
+EXPORT_SYMBOL(__dev_set_mtu);
/**
* dev_set_mtu - Change maximum transfer unit
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net] ip[6]: don't register inet[6]dev when dev is down
2017-07-05 22:43 ` Cong Wang
@ 2017-07-06 12:08 ` Nicolas Dichtel
2017-07-06 18:16 ` Cong Wang
0 siblings, 1 reply; 10+ messages in thread
From: Nicolas Dichtel @ 2017-07-06 12:08 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, Linux Kernel Network Developers, Hongjun Li
Le 06/07/2017 à 00:43, Cong Wang a écrit :
> On Wed, Jul 5, 2017 at 8:57 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>> When a device changes from one netns to another, it's first unregistered,
>> then the netns reference is updated and the dev is registered in the new
>> netns. Thus, when a slave moves to another netns, it is first
>> unregistered. This triggers a NETDEV_UNREGISTER event which is caught by
>> the bonding driver. The driver calls bond_release(), which calls
>> dev_set_mtu() and thus triggers NETDEV_CHANGEMTU (the device is still in
>> the old netns).
>
> I think in this special case it is meaningless to send
> NETDEV_CHANGEMTU, because the device is dying within
> its old netns, who still cares about its mtu change?
>
> Something like the attached patch...
Yes, your patch seems good and I hesitated with something like this.
But I don't see a valid case where the inet[6]dev must be created on a down
interface. I think the patch is valid, even with your patch.
Regards,
Nicolas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ip[6]: don't register inet[6]dev when dev is down
2017-07-06 12:08 ` Nicolas Dichtel
@ 2017-07-06 18:16 ` Cong Wang
2017-07-07 12:39 ` Nicolas Dichtel
0 siblings, 1 reply; 10+ messages in thread
From: Cong Wang @ 2017-07-06 18:16 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: David Miller, Linux Kernel Network Developers, Hongjun Li
On Thu, Jul 6, 2017 at 5:08 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> Le 06/07/2017 à 00:43, Cong Wang a écrit :
>> On Wed, Jul 5, 2017 at 8:57 AM, Nicolas Dichtel
>> <nicolas.dichtel@6wind.com> wrote:
>>> When a device changes from one netns to another, it's first unregistered,
>>> then the netns reference is updated and the dev is registered in the new
>>> netns. Thus, when a slave moves to another netns, it is first
>>> unregistered. This triggers a NETDEV_UNREGISTER event which is caught by
>>> the bonding driver. The driver calls bond_release(), which calls
>>> dev_set_mtu() and thus triggers NETDEV_CHANGEMTU (the device is still in
>>> the old netns).
>>
>> I think in this special case it is meaningless to send
>> NETDEV_CHANGEMTU, because the device is dying within
>> its old netns, who still cares about its mtu change?
>>
>> Something like the attached patch...
> Yes, your patch seems good and I hesitated with something like this.
> But I don't see a valid case where the inet[6]dev must be created on a down
> interface. I think the patch is valid, even with your patch.
Your patch is more risky because it affects normal CHANGEMTU path,
I am not sure if it is correct to not to add idev when it is down either.
This is a very unusual path, we don't have to take the risk.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ip[6]: don't register inet[6]dev when dev is down
2017-07-06 18:16 ` Cong Wang
@ 2017-07-07 12:39 ` Nicolas Dichtel
2017-07-08 18:56 ` Cong Wang
0 siblings, 1 reply; 10+ messages in thread
From: Nicolas Dichtel @ 2017-07-07 12:39 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, Linux Kernel Network Developers, Hongjun Li
Le 06/07/2017 à 20:16, Cong Wang a écrit :
> On Thu, Jul 6, 2017 at 5:08 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>> Le 06/07/2017 à 00:43, Cong Wang a écrit :
>>> On Wed, Jul 5, 2017 at 8:57 AM, Nicolas Dichtel
>>> <nicolas.dichtel@6wind.com> wrote:
>>>> When a device changes from one netns to another, it's first unregistered,
>>>> then the netns reference is updated and the dev is registered in the new
>>>> netns. Thus, when a slave moves to another netns, it is first
>>>> unregistered. This triggers a NETDEV_UNREGISTER event which is caught by
>>>> the bonding driver. The driver calls bond_release(), which calls
>>>> dev_set_mtu() and thus triggers NETDEV_CHANGEMTU (the device is still in
>>>> the old netns).
>>>
>>> I think in this special case it is meaningless to send
>>> NETDEV_CHANGEMTU, because the device is dying within
>>> its old netns, who still cares about its mtu change?
>>>
>>> Something like the attached patch...
>> Yes, your patch seems good and I hesitated with something like this.
>> But I don't see a valid case where the inet[6]dev must be created on a down
>> interface. I think the patch is valid, even with your patch.
>
> Your patch is more risky because it affects normal CHANGEMTU path,
> I am not sure if it is correct to not to add idev when it is down either.
Why would it be needed to add this idev on a down interface?
If idev wasn't there I don't see why changing the mtu would justify to create
this idev.
>
> This is a very unusual path, we don't have to take the risk.
I still think that this approach is better for two reasons:
- we don't know if another path like this exists (need an audit) and it would
be easy to add one again by side effect in the future;
- the patch is easy to backport in older kernel.
Regards,
Nicolas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ip[6]: don't register inet[6]dev when dev is down
2017-07-05 15:57 [PATCH net] ip[6]: don't register inet[6]dev when dev is down Nicolas Dichtel
2017-07-05 22:43 ` Cong Wang
@ 2017-07-08 10:02 ` David Miller
2017-07-08 18:44 ` Cong Wang
1 sibling, 1 reply; 10+ messages in thread
From: David Miller @ 2017-07-08 10:02 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev, hongjun.li
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Wed, 5 Jul 2017 17:57:25 +0200
> From: Hongjun Li <hongjun.li@6wind.com>
>
> When the netdev event NETDEV_CHANGEMTU is triggered, the inet[6]dev may be
> created even if the corresponding device is down. This may lead to a leak
> in the procfs when the device is unregistered, and finally trigger a
> backtrace:
...
> When a device changes from one netns to another, it's first unregistered,
> then the netns reference is updated and the dev is registered in the new
> netns. Thus, when a slave moves to another netns, it is first
> unregistered. This triggers a NETDEV_UNREGISTER event which is caught by
> the bonding driver. The driver calls bond_release(), which calls
> dev_set_mtu() and thus triggers NETDEV_CHANGEMTU (the device is still in
> the old netns).
>
> Signed-off-by: Hongjun Li <hongjun.li@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
I'm still not convinced about this.
We have lots of code which iterates ipv6 idevs, and then has a
check for IFF_UP.
So having an idev attached to a down interface is not a bug nor
illegal.
In fact, addrconf_cleanup() walks all of the init_net idevs and
calls addrconf_ifdown() with how=1 regardless of IFF_UP or not.
This entire area is quite a mess.
Can you show exactly why the procfs state isn't cleaned up for
these devices moving between namespaces? Maybe that is the real
bug and a better place to fix this.
Thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ip[6]: don't register inet[6]dev when dev is down
2017-07-08 10:02 ` David Miller
@ 2017-07-08 18:44 ` Cong Wang
2017-07-10 8:19 ` Nicolas Dichtel
0 siblings, 1 reply; 10+ messages in thread
From: Cong Wang @ 2017-07-08 18:44 UTC (permalink / raw)
To: David Miller; +Cc: Nicolas Dichtel, Linux Kernel Network Developers, Hongjun Li
On Sat, Jul 8, 2017 at 3:02 AM, David Miller <davem@davemloft.net> wrote:
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Wed, 5 Jul 2017 17:57:25 +0200
>
>> From: Hongjun Li <hongjun.li@6wind.com>
>>
>> When the netdev event NETDEV_CHANGEMTU is triggered, the inet[6]dev may be
>> created even if the corresponding device is down. This may lead to a leak
>> in the procfs when the device is unregistered, and finally trigger a
>> backtrace:
> ...
>> When a device changes from one netns to another, it's first unregistered,
>> then the netns reference is updated and the dev is registered in the new
>> netns. Thus, when a slave moves to another netns, it is first
>> unregistered. This triggers a NETDEV_UNREGISTER event which is caught by
>> the bonding driver. The driver calls bond_release(), which calls
>> dev_set_mtu() and thus triggers NETDEV_CHANGEMTU (the device is still in
>> the old netns).
>>
>> Signed-off-by: Hongjun Li <hongjun.li@6wind.com>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
> I'm still not convinced about this.
>
> We have lots of code which iterates ipv6 idevs, and then has a
> check for IFF_UP.
>
> So having an idev attached to a down interface is not a bug nor
> illegal.
>
> In fact, addrconf_cleanup() walks all of the init_net idevs and
> calls addrconf_ifdown() with how=1 regardless of IFF_UP or not.
>
> This entire area is quite a mess.
+1. I fixed a nasty bug with how=1 for loopback before...
>
> Can you show exactly why the procfs state isn't cleaned up for
> these devices moving between namespaces? Maybe that is the real
> bug and a better place to fix this.
>
It is because the ipv6_add_dev() adds these proc files back after
NETDEV_UNREGISTER event.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ip[6]: don't register inet[6]dev when dev is down
2017-07-07 12:39 ` Nicolas Dichtel
@ 2017-07-08 18:56 ` Cong Wang
2017-07-10 8:20 ` Nicolas Dichtel
0 siblings, 1 reply; 10+ messages in thread
From: Cong Wang @ 2017-07-08 18:56 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: David Miller, Linux Kernel Network Developers, Hongjun Li
On Fri, Jul 7, 2017 at 5:39 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> Le 06/07/2017 à 20:16, Cong Wang a écrit :
>> On Thu, Jul 6, 2017 at 5:08 AM, Nicolas Dichtel
>> <nicolas.dichtel@6wind.com> wrote:
>>> Le 06/07/2017 à 00:43, Cong Wang a écrit :
>>>> On Wed, Jul 5, 2017 at 8:57 AM, Nicolas Dichtel
>>>> <nicolas.dichtel@6wind.com> wrote:
>>>>> When a device changes from one netns to another, it's first unregistered,
>>>>> then the netns reference is updated and the dev is registered in the new
>>>>> netns. Thus, when a slave moves to another netns, it is first
>>>>> unregistered. This triggers a NETDEV_UNREGISTER event which is caught by
>>>>> the bonding driver. The driver calls bond_release(), which calls
>>>>> dev_set_mtu() and thus triggers NETDEV_CHANGEMTU (the device is still in
>>>>> the old netns).
>>>>
>>>> I think in this special case it is meaningless to send
>>>> NETDEV_CHANGEMTU, because the device is dying within
>>>> its old netns, who still cares about its mtu change?
>>>>
>>>> Something like the attached patch...
>>> Yes, your patch seems good and I hesitated with something like this.
>>> But I don't see a valid case where the inet[6]dev must be created on a down
>>> interface. I think the patch is valid, even with your patch.
>>
>> Your patch is more risky because it affects normal CHANGEMTU path,
>> I am not sure if it is correct to not to add idev when it is down either.
> Why would it be needed to add this idev on a down interface?
> If idev wasn't there I don't see why changing the mtu would justify to create
> this idev.
>
There must be a reason to check idev->if_flags & IF_READY instead
of IFF_UP.
>>
>> This is a very unusual path, we don't have to take the risk.
> I still think that this approach is better for two reasons:
> - we don't know if another path like this exists (need an audit) and it would
> be easy to add one again by side effect in the future;
Perhaps we need to add a warning for these events triggered after
UNREGISTER, except UNREGISTER_FINAL, in case of trouble.
But again, the CHANGEMTU case is so special because of the
idev.
> - the patch is easy to backport in older kernel.
>
Easy to backport doesn't mean easy to verify. ;) As David said, this
code is mess, especially for the keep_addr_on_down logic.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ip[6]: don't register inet[6]dev when dev is down
2017-07-08 18:44 ` Cong Wang
@ 2017-07-10 8:19 ` Nicolas Dichtel
0 siblings, 0 replies; 10+ messages in thread
From: Nicolas Dichtel @ 2017-07-10 8:19 UTC (permalink / raw)
To: Cong Wang, David Miller; +Cc: Linux Kernel Network Developers, Hongjun Li
Le 08/07/2017 à 20:44, Cong Wang a écrit :
> On Sat, Jul 8, 2017 at 3:02 AM, David Miller <davem@davemloft.net> wrote:
[snip]
>> Can you show exactly why the procfs state isn't cleaned up for
>> these devices moving between namespaces? Maybe that is the real
>> bug and a better place to fix this.
>>
>
> It is because the ipv6_add_dev() adds these proc files back after
> NETDEV_UNREGISTER event.
>
Yep, that was the initial problem.
Regards,
Nicolas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ip[6]: don't register inet[6]dev when dev is down
2017-07-08 18:56 ` Cong Wang
@ 2017-07-10 8:20 ` Nicolas Dichtel
0 siblings, 0 replies; 10+ messages in thread
From: Nicolas Dichtel @ 2017-07-10 8:20 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, Linux Kernel Network Developers, Hongjun Li
Le 08/07/2017 à 20:56, Cong Wang a écrit :
> On Fri, Jul 7, 2017 at 5:39 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
[snip]
>> - the patch is easy to backport in older kernel.
>>
>
> Easy to backport doesn't mean easy to verify. ;) As David said, this
> code is mess, especially for the keep_addr_on_down logic.
>
Yes sure. Let's live with your patch.
Thank you,
Nicolas
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-07-10 8:20 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-05 15:57 [PATCH net] ip[6]: don't register inet[6]dev when dev is down Nicolas Dichtel
2017-07-05 22:43 ` Cong Wang
2017-07-06 12:08 ` Nicolas Dichtel
2017-07-06 18:16 ` Cong Wang
2017-07-07 12:39 ` Nicolas Dichtel
2017-07-08 18:56 ` Cong Wang
2017-07-10 8:20 ` Nicolas Dichtel
2017-07-08 10:02 ` David Miller
2017-07-08 18:44 ` Cong Wang
2017-07-10 8:19 ` Nicolas Dichtel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.