| Message ID | 20200324123041.18825-1-ap420073@gmail.com |
|---|---|
| Headers | show |
| Series | net: core: avoid unexpected situation in namespace change routine | expand |
On Tue, Mar 24, 2020 at 12:30:41PM +0000, Taehee Yoo wrote: > This patchset is to avoid an unexpected situation when an interface's > namespace is being changed. > > When interface's namespace is being changed, dev_change_net_namespace() > is called. This removes and re-allocates many resources that include > sysfs files. The "/net/class/net/<interface name>" is one of them. > If the sysfs creation routine(device_rename()) found duplicate sysfs > file name, it warns about it and fails. But unfortunately, at that point, > dev_change_net_namespace() doesn't return fail because rollback cost > is too high. > So, the interface can't have a sysfs file. > > The approach of this patchset is to find the duplicate sysfs file as > fast as possible. If it found that, dev_change_net_namespace() returns > fail immediately with zero rollback cost. > > 1. The first patch is to add class_find_and_get_file_ns() helper function. > That function will be used for checking the existence of duplicate > sysfs file. > 2. The second patch is to add netdev_class_has_file_ns(). > That function is to check whether duplicate sysfs file in > the "/sys/class/net*" using class_find_and_get_file_ns(). > 3. The last patch is to avoid an unexpected situation. > a) If duplicate sysfs is existing, it fails as fast as possible in > the dev_change_net_namespace() > b) Acquire rtnl_lock() in both bond_create_sysfs() and bond_destroy_sysfs() > to avoid race condition. > c) Do not remove "/sys/class/net/bonding_masters" sysfs file by > bond_destroy_sysfs() if the file wasn't created by bond_create_sysfs(). > > Test commands#1: > ip netns add nst > ip link add bonding_masters type dummy > modprobe bonding > ip link set bonding_masters netns nst > > Test commands#2: > ip link add bonding_masters type dummy > ls /sys/class/net > modprobe bonding > modprobe -rv bonding > ls /sys/class/net > > After removing the bonding module, we can see the "bonding_masters" > interface's sysfs will be removed. > This is an unexpected situation. > > Taehee Yoo (3): > class: add class_find_and_get_file_ns() helper function > net: core: add netdev_class_has_file_ns() helper function > net: core: avoid warning in dev_change_net_namespace() > > drivers/base/class.c | 12 ++++++++++++ > drivers/net/bonding/bond_sysfs.c | 13 ++++++++++++- > include/linux/device/class.h | 4 +++- > include/linux/netdevice.h | 2 +- > include/net/bonding.h | 1 + > net/core/dev.c | 4 ++++ > net/core/net-sysfs.c | 13 +++++++++++++ > 7 files changed, 46 insertions(+), 3 deletions(-) I don't seem to see patch 1/3 anywhere...
On Tue, 24 Mar 2020 at 22:28, Greg KH <gregkh@linuxfoundation.org> wrote: > Hi Greg! > On Tue, Mar 24, 2020 at 12:30:41PM +0000, Taehee Yoo wrote: > > This patchset is to avoid an unexpected situation when an interface's > > namespace is being changed. > > > > When interface's namespace is being changed, dev_change_net_namespace() > > is called. This removes and re-allocates many resources that include > > sysfs files. The "/net/class/net/<interface name>" is one of them. > > If the sysfs creation routine(device_rename()) found duplicate sysfs > > file name, it warns about it and fails. But unfortunately, at that point, > > dev_change_net_namespace() doesn't return fail because rollback cost > > is too high. > > So, the interface can't have a sysfs file. > > > > The approach of this patchset is to find the duplicate sysfs file as > > fast as possible. If it found that, dev_change_net_namespace() returns > > fail immediately with zero rollback cost. > > > > 1. The first patch is to add class_find_and_get_file_ns() helper function. > > That function will be used for checking the existence of duplicate > > sysfs file. > > 2. The second patch is to add netdev_class_has_file_ns(). > > That function is to check whether duplicate sysfs file in > > the "/sys/class/net*" using class_find_and_get_file_ns(). > > 3. The last patch is to avoid an unexpected situation. > > a) If duplicate sysfs is existing, it fails as fast as possible in > > the dev_change_net_namespace() > > b) Acquire rtnl_lock() in both bond_create_sysfs() and bond_destroy_sysfs() > > to avoid race condition. > > c) Do not remove "/sys/class/net/bonding_masters" sysfs file by > > bond_destroy_sysfs() if the file wasn't created by bond_create_sysfs(). > > > > Test commands#1: > > ip netns add nst > > ip link add bonding_masters type dummy > > modprobe bonding > > ip link set bonding_masters netns nst > > > > Test commands#2: > > ip link add bonding_masters type dummy > > ls /sys/class/net > > modprobe bonding > > modprobe -rv bonding > > ls /sys/class/net > > > > After removing the bonding module, we can see the "bonding_masters" > > interface's sysfs will be removed. > > This is an unexpected situation. > > > > Taehee Yoo (3): > > class: add class_find_and_get_file_ns() helper function > > net: core: add netdev_class_has_file_ns() helper function > > net: core: avoid warning in dev_change_net_namespace() > > > > drivers/base/class.c | 12 ++++++++++++ > > drivers/net/bonding/bond_sysfs.c | 13 ++++++++++++- > > include/linux/device/class.h | 4 +++- > > include/linux/netdevice.h | 2 +- > > include/net/bonding.h | 1 + > > net/core/dev.c | 4 ++++ > > net/core/net-sysfs.c | 13 +++++++++++++ > > 7 files changed, 46 insertions(+), 3 deletions(-) > > I don't seem to see patch 1/3 anywhere... > I don't know why the first patch was lost. Below is the lkml link. https://lkml.org/lkml/2020/3/24/576 I will resend the first patch. Thank you! Taehee Yoo
This patchset is to avoid an unexpected situation when an interface's namespace is being changed. When interface's namespace is being changed, dev_change_net_namespace() is called. This removes and re-allocates many resources that include sysfs files. The "/net/class/net/<interface name>" is one of them. If the sysfs creation routine(device_rename()) found duplicate sysfs file name, it warns about it and fails. But unfortunately, at that point, dev_change_net_namespace() doesn't return fail because rollback cost is too high. So, the interface can't have a sysfs file. The approach of this patchset is to find the duplicate sysfs file as fast as possible. If it found that, dev_change_net_namespace() returns fail immediately with zero rollback cost. 1. The first patch is to add class_find_and_get_file_ns() helper function. That function will be used for checking the existence of duplicate sysfs file. 2. The second patch is to add netdev_class_has_file_ns(). That function is to check whether duplicate sysfs file in the "/sys/class/net*" using class_find_and_get_file_ns(). 3. The last patch is to avoid an unexpected situation. a) If duplicate sysfs is existing, it fails as fast as possible in the dev_change_net_namespace() b) Acquire rtnl_lock() in both bond_create_sysfs() and bond_destroy_sysfs() to avoid race condition. c) Do not remove "/sys/class/net/bonding_masters" sysfs file by bond_destroy_sysfs() if the file wasn't created by bond_create_sysfs(). Test commands#1: ip netns add nst ip link add bonding_masters type dummy modprobe bonding ip link set bonding_masters netns nst Test commands#2: ip link add bonding_masters type dummy ls /sys/class/net modprobe bonding modprobe -rv bonding ls /sys/class/net After removing the bonding module, we can see the "bonding_masters" interface's sysfs will be removed. This is an unexpected situation. Taehee Yoo (3): class: add class_find_and_get_file_ns() helper function net: core: add netdev_class_has_file_ns() helper function net: core: avoid warning in dev_change_net_namespace() drivers/base/class.c | 12 ++++++++++++ drivers/net/bonding/bond_sysfs.c | 13 ++++++++++++- include/linux/device/class.h | 4 +++- include/linux/netdevice.h | 2 +- include/net/bonding.h | 1 + net/core/dev.c | 4 ++++ net/core/net-sysfs.c | 13 +++++++++++++ 7 files changed, 46 insertions(+), 3 deletions(-)