Re: Network namespace and bonding WARNING at fs/proc/generic.c remove_proc_entry

Re: Network namespace and bonding WARNING at fs/proc/generic.c remove_proc_entry

[Eric W. Biederman]

"Serge E. Hallyn" <serge@hallyn.com> writes:

> Quoting Dilip Daya (dilip.daya@hp.com):
>> Hi,
>> 
>> I'd discussed the following with Serge Hallyn.
>> 
>> => Environment based on 3.2.18 / x86_64 kernel.
>> => WARNING: at fs/proc/generic.c:808 remove_proc_entry+0xdb/0x21f()
>> => WARNING: at fs/proc/generic.c:849 remove_proc_entry+0x208/0x21f()
>
> Hi,
>
> thanks much for sending this.  I'm still getting this error on
> 3.5.0-2-generic (today's ubuntu quantal kernel)
>
>> network namespace and bonding
>> -----------------------------
>> 
>> * Migrate two phy nics from host to netns (netns0).
>>   - ip link set ethX netns netns0
>> 
>> * In host environment:
>>   - load bonding module, /sbin/modprobe -v bonding mode=1 miimon=100
>>   - /sys/class/net/bond0 exists.
>>   - /proc/net/bonding/bond0 exists.
>>   - /sys/class/net/bonding_masters has bond0.
>> 
>> * Migrate bond0 to netns (netns0):
>>   - ip link set bond0 netns netns0.
>> 
>> * Within netns (netns0):
>>   - /sys/class/net/bonding_masters is empty.
>>   - /sys/class/net/bond0 exist.
>>   - configure bond0 and ifenslave with two phy nics.
>>   - /proc/net/bonding/bond0 does not exist within netns0, but does
>>     exist in the host environment.
>>   - /sys/class/net/bonding_masters is empty.
>
> mine is not empty, fwiw.  However
>
>>   - ping to remote end of bond0 works.
>> 
>> * Within netns (netns0), flushing ethX and bondY:
>>   - down bond0 and its phy nic interfaces:
>>   - ip link set ... down
>>   - ip addr flush dev [bond0 | eth#]
>>   - deleting bond0, /sbin/ip link del dev bond0
>
> Yup I still get a remove_proc_entry WARNING at fs/proc/generic.c:808,
> which is the warning when (!de)

It looks like Dilip is running an old kernel.  There should have been
some version of /sys/class/net/bonding_masters in every network
namespace since sometime in 2009.

>From the warning it looks like the proc files are being added/removed
to the wrong network namespace.  So in one namespace we get an error
when we delete the moved device and in the other network namespace
we get an error when we remove the /proc/directory.

An old kernel without proper network namespace support is the only
reason I can imagine someone would be moving an existing bond device
between network namespaces.

If there are other reasons for wanting to move a bonding device between
network namespaces it is possible to catch the NETDEV_UNREGISTER and
NETDEV_REGISTER events to remove/add the per device proc files at the
appropriate time.

However since moving bonding devices appears to be an unneded operation
let's just do things simply and forbid moving bonding devices between
network namespaces.  Serge, Dilip can you two test the patch below
and see if it fixes the warnings.

Eric


diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 2ee8cf9..818ed64 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4345,6 +4345,9 @@ static void bond_setup(struct net_device *bond_dev)
        bond_dev->priv_flags |= IFF_BONDING;
        bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
 
+       /* Don't allow bond devices to change network namespaces. */
+       bond_dev->features |= NETIF_F_LOCAL;
+
        /* At first, we block adding VLANs. That's the only way to
         * prevent problems that occur when adding VLANs over an
         * empty bond. The block will be removed once non-challenged

Re: Network namespace and bonding WARNING at fs/proc/generic.c remove_proc_entry

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
> 
> > Quoting Dilip Daya (dilip.daya-VXdhtT5mjnY@public.gmane.org):
> >> Hi,
> >> 
> >> I'd discussed the following with Serge Hallyn.
> >> 
> >> => Environment based on 3.2.18 / x86_64 kernel.
> >> => WARNING: at fs/proc/generic.c:808 remove_proc_entry+0xdb/0x21f()
> >> => WARNING: at fs/proc/generic.c:849 remove_proc_entry+0x208/0x21f()
> >
> > Hi,
> >
> > thanks much for sending this.  I'm still getting this error on
> > 3.5.0-2-generic (today's ubuntu quantal kernel)
> >
> >> network namespace and bonding
> >> -----------------------------
> >> 
> >> * Migrate two phy nics from host to netns (netns0).
> >>   - ip link set ethX netns netns0
> >> 
> >> * In host environment:
> >>   - load bonding module, /sbin/modprobe -v bonding mode=1 miimon=100
> >>   - /sys/class/net/bond0 exists.
> >>   - /proc/net/bonding/bond0 exists.
> >>   - /sys/class/net/bonding_masters has bond0.
> >> 
> >> * Migrate bond0 to netns (netns0):
> >>   - ip link set bond0 netns netns0.
> >> 
> >> * Within netns (netns0):
> >>   - /sys/class/net/bonding_masters is empty.
> >>   - /sys/class/net/bond0 exist.
> >>   - configure bond0 and ifenslave with two phy nics.
> >>   - /proc/net/bonding/bond0 does not exist within netns0, but does
> >>     exist in the host environment.
> >>   - /sys/class/net/bonding_masters is empty.
> >
> > mine is not empty, fwiw.  However
> >
> >>   - ping to remote end of bond0 works.
> >> 
> >> * Within netns (netns0), flushing ethX and bondY:
> >>   - down bond0 and its phy nic interfaces:
> >>   - ip link set ... down
> >>   - ip addr flush dev [bond0 | eth#]
> >>   - deleting bond0, /sbin/ip link del dev bond0
> >
> > Yup I still get a remove_proc_entry WARNING at fs/proc/generic.c:808,
> > which is the warning when (!de)
> 
> It looks like Dilip is running an old kernel.  There should have been
> some version of /sys/class/net/bonding_masters in every network
> namespace since sometime in 2009.
> 
> >From the warning it looks like the proc files are being added/removed
> to the wrong network namespace.  So in one namespace we get an error
> when we delete the moved device and in the other network namespace
> we get an error when we remove the /proc/directory.
> 
> An old kernel without proper network namespace support is the only
> reason I can imagine someone would be moving an existing bond device
> between network namespaces.
> 
> If there are other reasons for wanting to move a bonding device between
> network namespaces it is possible to catch the NETDEV_UNREGISTER and
> NETDEV_REGISTER events to remove/add the per device proc files at the
> appropriate time.
> 
> However since moving bonding devices appears to be an unneded operation
> let's just do things simply and forbid moving bonding devices between
> network namespaces.  Serge, Dilip can you two test the patch below
> and see if it fixes the warnings.
> 
> Eric
> 
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 2ee8cf9..818ed64 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4345,6 +4345,9 @@ static void bond_setup(struct net_device *bond_dev)
>         bond_dev->priv_flags |= IFF_BONDING;
>         bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
>  
> +       /* Don't allow bond devices to change network namespaces. */
> +       bond_dev->features |= NETIF_F_LOCAL;

I believe this needs to be NETIF_F_NETNS_LOCAL.  Test build still going with
that change.

> +
>         /* At first, we block adding VLANs. That's the only way to
>          * prevent problems that occur when adding VLANs over an
>          * empty bond. The block will be removed once non-challenged

Re: Network namespace and bonding WARNING at fs/proc/generic.c remove_proc_entry

[Dilip Daya]

Hi Serge,

On Fri, 2012-07-06 at 17:05 +0000, Serge E. Hallyn wrote:
> Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> > "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
> > 
> > > Quoting Dilip Daya (dilip.daya-VXdhtT5mjnY@public.gmane.org):
> > >> Hi,
> > >> 
> > >> I'd discussed the following with Serge Hallyn.
> > >> 
> > >> => Environment based on 3.2.18 / x86_64 kernel.
> > >> => WARNING: at fs/proc/generic.c:808 remove_proc_entry+0xdb/0x21f()
> > >> => WARNING: at fs/proc/generic.c:849 remove_proc_entry+0x208/0x21f()
> > >
> > > Hi,
> > >
> > > thanks much for sending this.  I'm still getting this error on
> > > 3.5.0-2-generic (today's ubuntu quantal kernel)
> > >
> > >> network namespace and bonding
> > >> -----------------------------
> > >> 
> > >> * Migrate two phy nics from host to netns (netns0).
> > >>   - ip link set ethX netns netns0
> > >> 
> > >> * In host environment:
> > >>   - load bonding module, /sbin/modprobe -v bonding mode=1 miimon=100
> > >>   - /sys/class/net/bond0 exists.
> > >>   - /proc/net/bonding/bond0 exists.
> > >>   - /sys/class/net/bonding_masters has bond0.
> > >> 
> > >> * Migrate bond0 to netns (netns0):
> > >>   - ip link set bond0 netns netns0.
> > >> 
> > >> * Within netns (netns0):
> > >>   - /sys/class/net/bonding_masters is empty.
> > >>   - /sys/class/net/bond0 exist.
> > >>   - configure bond0 and ifenslave with two phy nics.
> > >>   - /proc/net/bonding/bond0 does not exist within netns0, but does
> > >>     exist in the host environment.
> > >>   - /sys/class/net/bonding_masters is empty.
> > >
> > > mine is not empty, fwiw.  However
> > >
> > >>   - ping to remote end of bond0 works.
> > >> 
> > >> * Within netns (netns0), flushing ethX and bondY:
> > >>   - down bond0 and its phy nic interfaces:
> > >>   - ip link set ... down
> > >>   - ip addr flush dev [bond0 | eth#]
> > >>   - deleting bond0, /sbin/ip link del dev bond0
> > >
> > > Yup I still get a remove_proc_entry WARNING at fs/proc/generic.c:808,
> > > which is the warning when (!de)
> > 
> > It looks like Dilip is running an old kernel.  There should have been
> > some version of /sys/class/net/bonding_masters in every network
> > namespace since sometime in 2009.
> > 
> > >From the warning it looks like the proc files are being added/removed
> > to the wrong network namespace.  So in one namespace we get an error
> > when we delete the moved device and in the other network namespace
> > we get an error when we remove the /proc/directory.
> > 
> > An old kernel without proper network namespace support is the only
> > reason I can imagine someone would be moving an existing bond device
> > between network namespaces.
> > 
> > If there are other reasons for wanting to move a bonding device between
> > network namespaces it is possible to catch the NETDEV_UNREGISTER and
> > NETDEV_REGISTER events to remove/add the per device proc files at the
> > appropriate time.
> > 
> > However since moving bonding devices appears to be an unneded operation
> > let's just do things simply and forbid moving bonding devices between
> > network namespaces.  Serge, Dilip can you two test the patch below
> > and see if it fixes the warnings.
> > 
> > Eric
> > 
> > 
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index 2ee8cf9..818ed64 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -4345,6 +4345,9 @@ static void bond_setup(struct net_device *bond_dev)
> >         bond_dev->priv_flags |= IFF_BONDING;
> >         bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
> >  
> > +       /* Don't allow bond devices to change network namespaces. */
> > +       bond_dev->features |= NETIF_F_LOCAL;
> 
> I believe this needs to be NETIF_F_NETNS_LOCAL.  Test build still going with
> that change.


Correct, I made that change and rebuilt bonding driver:

# modinfo bonding | head
filename:       /lib/modules/3.2.18-clim-3-amd64/kernel/drivers/net/bonding/bonding.ko
alias:          rtnl-link-bond
author:         Thomas Davis, tadavis-/3juihCSby0@public.gmane.org and many others
description:    Ethernet Channel Bonding Driver, v3.7.1-netns
version:        3.7.1-netns
...


My results with the above bonding driver:

(1) Migrating bond0 from host to netns:

  # ip link set bond0 netns netns0
  RTNETLINK answers: Invalid argument

  => cannot migrate bond0 from host to netns.
  => No warnings.


(2) Loading bonding module in host environment and unloading bonding
    module from within netns:

  # modprobe -v -r bonding
  #
rmmod /lib/modules/3.2.18-clim-3-amd64/kernel/drivers/net/bonding/bonding.ko

	# lsmod | grep bond
	<<< bonding module does not exist >>>

	# ll /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 Jul  6 11:00 lo
-> ../../devices/virtual/net/lo/
lrwxrwxrwx 1 root root 0 Jul  6 11:00 eth7
-> ../../devices/pci0000:00/0000:00:05.0/0000:14:00.1/net/eth7/
lrwxrwxrwx 1 root root 0 Jul  6 11:00 eth6
-> ../../devices/pci0000:00/0000:00:05.0/0000:14:00.0/net/eth6/

	=> No warnings.


-DilipD.

Re: Network namespace and bonding WARNING at fs/proc/generic.c remove_proc_entry

[Eric W. Biederman]

"Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:

>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 2ee8cf9..818ed64 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -4345,6 +4345,9 @@ static void bond_setup(struct net_device *bond_dev)
>>         bond_dev->priv_flags |= IFF_BONDING;
>>         bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
>>  
>> +       /* Don't allow bond devices to change network namespaces. */
>> +       bond_dev->features |= NETIF_F_LOCAL;
>
> I believe this needs to be NETIF_F_NETNS_LOCAL.  Test build still going with
> that change.

Yes that is what I mean.

Eric

Re: Network namespace and bonding WARNING at fs/proc/generic.c remove_proc_entry

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
> 
> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >> index 2ee8cf9..818ed64 100644
> >> --- a/drivers/net/bonding/bond_main.c
> >> +++ b/drivers/net/bonding/bond_main.c
> >> @@ -4345,6 +4345,9 @@ static void bond_setup(struct net_device *bond_dev)
> >>         bond_dev->priv_flags |= IFF_BONDING;
> >>         bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
> >>  
> >> +       /* Don't allow bond devices to change network namespaces. */
> >> +       bond_dev->features |= NETIF_F_LOCAL;
> >
> > I believe this needs to be NETIF_F_NETNS_LOCAL.  Test build still going with
> > that change.
> 
> Yes that is what I mean.

With that change, build is fine, boots fine, I can't pass a bond to another
netns (preventing the problem), and I can create a bond in a child netns
just fine.

Thanks!

Acked-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

-serge

[PATCH 1/2] bonding: Manage /proc/net/bonding/ entries from the netdev events

[Eric W. Biederman]

It was recently reported that moving a bonding device between network
namespaces causes warnings from /proc.  It turns out after the move we
were trying to add and to remove the /proc/net/bonding entries from the
wrong network namespace.

Move the bonding /proc registration code into the NETDEV_REGISTER and
NETDEV_UNREGISTER events where the proc registration and unregistration
will always happen at the right time.

Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 drivers/net/bonding/bond_main.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 2ee8cf9..50de0fe 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3226,6 +3226,12 @@ static int bond_master_netdev_event(unsigned long event,
 	switch (event) {
 	case NETDEV_CHANGENAME:
 		return bond_event_changename(event_bond);
+	case NETDEV_UNREGISTER:
+		bond_remove_proc_entry(event_bond);
+		break;
+	case NETDEV_REGISTER:
+		bond_create_proc_entry(event_bond);
+		break;
 	default:
 		break;
 	}
@@ -4410,8 +4416,6 @@ static void bond_uninit(struct net_device *bond_dev)
 
 	bond_work_cancel_all(bond);
 
-	bond_remove_proc_entry(bond);
-
 	bond_debug_unregister(bond);
 
 	__hw_addr_flush(&bond->mc_list);
@@ -4813,7 +4817,6 @@ static int bond_init(struct net_device *bond_dev)
 
 	bond_set_lockdep_class(bond_dev);
 
-	bond_create_proc_entry(bond);
 	list_add_tail(&bond->bond_list, &bn->dev_list);
 
 	bond_prepare_sysfs_group(bond);
-- 
1.7.5.4

[PATCH 2/2] bonding: debugfs and network namespaces are incompatible

[Eric W. Biederman]

The bonding debugfs support has been broken in the presence of network
namespaces since it has been added.  The debugfs support does not handle
multiple bonding devices with the same name in different network
namespaces.

I haven't had any bug reports, and I'm not interested in getting any.
Disable the debugfs support when network namespaces are enabled.

Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 drivers/net/bonding/bond_debugfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_debugfs.c b/drivers/net/bonding/bond_debugfs.c
index 3680aa2..2cf084e 100644
--- a/drivers/net/bonding/bond_debugfs.c
+++ b/drivers/net/bonding/bond_debugfs.c
@@ -6,7 +6,7 @@
 #include "bonding.h"
 #include "bond_alb.h"
 
-#ifdef CONFIG_DEBUG_FS
+#if defined(CONFIG_DEBUG_FS) && !defined(CONFIG_NET_NS)
 
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
-- 
1.7.5.4

Re: [PATCH 2/2] bonding: debugfs and network namespaces are incompatible

[David Miller]

From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman)
Date: Mon, 09 Jul 2012 13:52:43 -0700

> 
> The bonding debugfs support has been broken in the presence of network
> namespaces since it has been added.  The debugfs support does not handle
> multiple bonding devices with the same name in different network
> namespaces.
> 
> I haven't had any bug reports, and I'm not interested in getting any.
> Disable the debugfs support when network namespaces are enabled.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

Applied.

Re: [PATCH 2/2] bonding: debugfs and network namespaces are incompatible

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:

>From: ebiederm@xmission.com (Eric W. Biederman)
>Date: Mon, 09 Jul 2012 13:52:43 -0700
>
>> 
>> The bonding debugfs support has been broken in the presence of network
>> namespaces since it has been added.  The debugfs support does not handle
>> multiple bonding devices with the same name in different network
>> namespaces.
>> 
>> I haven't had any bug reports, and I'm not interested in getting any.
>> Disable the debugfs support when network namespaces are enabled.
>> 
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
>Applied.

	Since distro kernels appear to set CONFIG_NET_NS, doesn't this
effectively disable debugfs for bonding on most distros?

	Do the other network device drivers that support debugfs have a
similar problem?  E.g., if each of two namespaces have an skge device
with the same name, will there be a debugfs conflict there as well?

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 2/2] bonding: debugfs and network namespaces are incompatible

[Jay Vosburgh]

	[ adding netdev back to cc: ]

Eric W. Biederman <ebiederm@xmission.com> wrote:

>Jay Vosburgh <fubar@us.ibm.com> wrote:
>
>>David Miller <davem@davemloft.net> wrote:
>>
>>>From: ebiederm@xmission.com (Eric W. Biederman)
>>>Date: Mon, 09 Jul 2012 13:52:43 -0700
>>>
>>>> 
>>>> The bonding debugfs support has been broken in the presence of
>>network
>>>> namespaces since it has been added.  The debugfs support does not
>>handle
>>>> multiple bonding devices with the same name in different network
>>>> namespaces.
>>>> 
>>>> I haven't had any bug reports, and I'm not interested in getting
>>any.
>>>> Disable the debugfs support when network namespaces are enabled.
>>>> 
>>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>>
>>>Applied.
>>
>>	Since distro kernels appear to set CONFIG_NET_NS, doesn't this
>>effectively disable debugfs for bonding on most distros?
>
>Yes.
>
>>	Do the other network device drivers that support debugfs have a
>>similar problem?  E.g., if each of two namespaces have an skge device
>>with the same name, will there be a debugfs conflict there as well?
>
>I haven't run across any of those network devices, but if they create a
>debugfs entry that embeds the device name it will be a problem.

	A quick grep suggests that cxgb4, skge, sky2, stmmac, ipoib and
half a dozen of the wireless drivers all create files in debugfs.  I did
not check exhaustively, but at least some of them include the device
name.

>Last I looked any custom user space interface from network devices was
>rare and bonding using debugfs is the first instance of using debugfs
>from networking devices I have seen.
>
>I think the problem will be a little less severe for physical network
>devices as they all start in the initial network namespace and so start
>with distinct names.
>
>With bonding I can do "ip link add type bond" in any network namespace
>and get another bond0.  So name conflicts are very much expeted with all
>virtual networking devices.

	Fair enough, although it is trivial to rename any network device
such that a conflict would occur.

	It looks like some of the drivers use fixed names for some
things as well.

>But if you know of any other networking devices using debugsfs that
>code should probably get the same treatment as the bonding debugfs code.

	Is there no alternative than simply disabling debugfs whenever
network namespaces are enabled?  The information bonding displays via
debugfs is useful, and having it unavailable on all distro kernels seems
a bit harsh.

	Why is the logic already in the driver not sufficient?  If the
attempt to create the debugfs directory with the interface name fails,
then it merely prints a warning and continues without the debugfs for
that interface.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 2/2] bonding: debugfs and network namespaces are incompatible

[Eric W. Biederman]

Jay Vosburgh <fubar@us.ibm.com> writes:

> Eric W. Biederman <ebiederm@xmission.com> wrote:

>>I haven't run across any of those network devices, but if they create a
>>debugfs entry that embeds the device name it will be a problem.
>
> 	A quick grep suggests that cxgb4, skge, sky2, stmmac, ipoib and
> half a dozen of the wireless drivers all create files in debugfs.  I did
> not check exhaustively, but at least some of them include the device
> name.

Yep.  It looks like imperfect habits are common.

>>Last I looked any custom user space interface from network devices was
>>rare and bonding using debugfs is the first instance of using debugfs
>>from networking devices I have seen.
>>
>>I think the problem will be a little less severe for physical network
>>devices as they all start in the initial network namespace and so start
>>with distinct names.
>>
>>With bonding I can do "ip link add type bond" in any network namespace
>>and get another bond0.  So name conflicts are very much expeted with all
>>virtual networking devices.
>
> 	Fair enough, although it is trivial to rename any network device
> such that a conflict would occur.

Actually for userspace and administrative reasons frequently it isn't
trivial to rename devices.

>>But if you know of any other networking devices using debugsfs that
>>code should probably get the same treatment as the bonding debugfs code.
>
> 	Is there no alternative than simply disabling debugfs whenever
> network namespaces are enabled?  The information bonding displays via
> debugfs is useful, and having it unavailable on all distro kernels seems
> a bit harsh.


I took a good hard look at debugfs while writing this reply and debufs
scares me.  It is the kind of code that just about wants to me to throw
in the towel seeing no hope of a good solid kernel. 

I can definitely open a /sys/kernel/debug/bonding/bond0/rlb_hash_table
and delete the bond and then read the file.  On a bad day that will oops
the kernel, as there is nothing holding a reference to the network
device.  I think only the BOND_MODE_ALB check makes keeps the kernel
from oopsing in my quick tests.

The fact that debugfs is enabled in distro kernels is actually apalling
to me.  debugfs makes it easy to oops the kernel.

There are lots of alternatives to debugfs on where to put information
and the bonding driver already uses most of them.

> 	Why is the logic already in the driver not sufficient?  If the
> attempt to create the debugfs directory with the interface name fails,
> then it merely prints a warning and continues without the debugfs for
> that interface.

All I know for certain is the existing logic will eventually cause
someone doing something reasonable to send me a bug report.

I can see where you are coming from in that the bonding driver debugfs
code really was built to gracefully fail and ignore problems of instead
of just hapharzardly and sloppily ignore problems.  At the same time
I can oops the kernel if I try with your debugfs in the bonding driver.

But it causes the code to fail and issue a warning.  So if I don't
disable the code now, I expect I will get a bug report, and who
knows how many sill files in bonding will have in debugfs by then.
what silly things bonding may be doing in debugfs by then.

Eric

Re: [PATCH 2/2] bonding: debugfs and network namespaces are incompatible

[Jay Vosburgh]

Eric W. Biederman <ebiederm@xmission.com> wrote:

>Jay Vosburgh <fubar@us.ibm.com> writes:
>
>> Eric W. Biederman <ebiederm@xmission.com> wrote:
>
>>>I haven't run across any of those network devices, but if they create a
>>>debugfs entry that embeds the device name it will be a problem.
>>
>> 	A quick grep suggests that cxgb4, skge, sky2, stmmac, ipoib and
>> half a dozen of the wireless drivers all create files in debugfs.  I did
>> not check exhaustively, but at least some of them include the device
>> name.
>
>Yep.  It looks like imperfect habits are common.
>
>>>Last I looked any custom user space interface from network devices was
>>>rare and bonding using debugfs is the first instance of using debugfs
>>>from networking devices I have seen.
>>>
>>>I think the problem will be a little less severe for physical network
>>>devices as they all start in the initial network namespace and so start
>>>with distinct names.
>>>
>>>With bonding I can do "ip link add type bond" in any network namespace
>>>and get another bond0.  So name conflicts are very much expeted with all
>>>virtual networking devices.
>>
>> 	Fair enough, although it is trivial to rename any network device
>> such that a conflict would occur.
>
>Actually for userspace and administrative reasons frequently it isn't
>trivial to rename devices.

	Well, perhaps it's uncommon for users to do so, but "ip link set
dev eth0 name eth44" is pretty easy to do.

>>>But if you know of any other networking devices using debugsfs that
>>>code should probably get the same treatment as the bonding debugfs code.
>>
>> 	Is there no alternative than simply disabling debugfs whenever
>> network namespaces are enabled?  The information bonding displays via
>> debugfs is useful, and having it unavailable on all distro kernels seems
>> a bit harsh.
>
>
>I took a good hard look at debugfs while writing this reply and debufs
>scares me.  It is the kind of code that just about wants to me to throw
>in the towel seeing no hope of a good solid kernel. 
>
>I can definitely open a /sys/kernel/debug/bonding/bond0/rlb_hash_table
>and delete the bond and then read the file.  On a bad day that will oops
>the kernel, as there is nothing holding a reference to the network
>device.  I think only the BOND_MODE_ALB check makes keeps the kernel
>from oopsing in my quick tests.
>
>The fact that debugfs is enabled in distro kernels is actually apalling
>to me.  debugfs makes it easy to oops the kernel.

	I'm not so sure things are that bad.  I cannot unload the
bonding module while a program holds an open file descriptor on its
debugfs file (it appears to hold a reference to the module), so uses
that only remove the debugfs file on module unload shouldn't have a
problem.

	The /proc file that bonding removes when an interface is
dynamically removed does not have this problem, as subsequent reads on
that file descriptor will fail.  I suspect that's because
remove_proc_entry NULLs the proc_fops, whereas debugfs_remove does not
do the equivalent for its case.  It may not be that simple, though; I'm
just looking at the code and have not tested anything.

>There are lots of alternatives to debugfs on where to put information
>and the bonding driver already uses most of them.
>
>> 	Why is the logic already in the driver not sufficient?  If the
>> attempt to create the debugfs directory with the interface name fails,
>> then it merely prints a warning and continues without the debugfs for
>> that interface.
>
>All I know for certain is the existing logic will eventually cause
>someone doing something reasonable to send me a bug report.
>
>I can see where you are coming from in that the bonding driver debugfs
>code really was built to gracefully fail and ignore problems of instead
>of just hapharzardly and sloppily ignore problems.  At the same time
>I can oops the kernel if I try with your debugfs in the bonding driver.
>
>But it causes the code to fail and issue a warning.  So if I don't
>disable the code now, I expect I will get a bug report, and who
>knows how many sill files in bonding will have in debugfs by then.
>what silly things bonding may be doing in debugfs by then.

	Or perhaps we can fix the debugfs support to function correctly
even in the face of network namespaces.  For example, do namespaces have
a unique name or identifier than can go into the debugfs name?

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 1/2] bonding: Manage /proc/net/bonding/ entries from the netdev events

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Mon, 09 Jul 2012 13:51:45 -0700

> 
> It was recently reported that moving a bonding device between network
> namespaces causes warnings from /proc.  It turns out after the move we
> were trying to add and to remove the /proc/net/bonding entries from the
> wrong network namespace.
> 
> Move the bonding /proc registration code into the NETDEV_REGISTER and
> NETDEV_UNREGISTER events where the proc registration and unregistration
> will always happen at the right time.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Applied.

Re: Network namespace and bonding WARNING at fs/proc/generic.c remove_proc_entry

[Dilip Daya]

Hi Eric,

On Thu, 2012-07-05 at 17:41 -0700, Eric W. Biederman wrote:
> "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
> 
> > Quoting Dilip Daya (dilip.daya-VXdhtT5mjnY@public.gmane.org):
> >> Hi,
> >> 
> >> I'd discussed the following with Serge Hallyn.
> >> 
> >> => Environment based on 3.2.18 / x86_64 kernel.
> >> => WARNING: at fs/proc/generic.c:808 remove_proc_entry+0xdb/0x21f()
> >> => WARNING: at fs/proc/generic.c:849 remove_proc_entry+0x208/0x21f()
> >
> > Hi,
> >
> > thanks much for sending this.  I'm still getting this error on
> > 3.5.0-2-generic (today's ubuntu quantal kernel)
> >
> >> network namespace and bonding
> >> -----------------------------
> >> 
> >> * Migrate two phy nics from host to netns (netns0).
> >>   - ip link set ethX netns netns0
> >> 
> >> * In host environment:
> >>   - load bonding module, /sbin/modprobe -v bonding mode=1 miimon=100
> >>   - /sys/class/net/bond0 exists.
> >>   - /proc/net/bonding/bond0 exists.
> >>   - /sys/class/net/bonding_masters has bond0.
> >> 
> >> * Migrate bond0 to netns (netns0):
> >>   - ip link set bond0 netns netns0.
> >> 
> >> * Within netns (netns0):
> >>   - /sys/class/net/bonding_masters is empty.
> >>   - /sys/class/net/bond0 exist.
> >>   - configure bond0 and ifenslave with two phy nics.
> >>   - /proc/net/bonding/bond0 does not exist within netns0, but does
> >>     exist in the host environment.
> >>   - /sys/class/net/bonding_masters is empty.
> >
> > mine is not empty, fwiw.  However
> >
> >>   - ping to remote end of bond0 works.
> >> 
> >> * Within netns (netns0), flushing ethX and bondY:
> >>   - down bond0 and its phy nic interfaces:
> >>   - ip link set ... down
> >>   - ip addr flush dev [bond0 | eth#]
> >>   - deleting bond0, /sbin/ip link del dev bond0
> >
> > Yup I still get a remove_proc_entry WARNING at fs/proc/generic.c:808,
> > which is the warning when (!de)
> 
> It looks like Dilip is running an old kernel.  There should have been
> some version of /sys/class/net/bonding_masters in every network
> namespace since sometime in 2009.
> 
> >From the warning it looks like the proc files are being added/removed
> to the wrong network namespace.  So in one namespace we get an error
> when we delete the moved device and in the other network namespace
> we get an error when we remove the /proc/directory.
> 
> An old kernel without proper network namespace support is the only
> reason I can imagine someone would be moving an existing bond device
> between network namespaces.
> 
> If there are other reasons for wanting to move a bonding device between
> network namespaces it is possible to catch the NETDEV_UNREGISTER and
> NETDEV_REGISTER events to remove/add the per device proc files at the
> appropriate time.


We do need to move bonds between namespaces - because we require
physical interfaces in each namespace -- we don't want the overheads of
virtual interfaces, don't have the management infrastructure, and don't
want to manufacture fake mac addresses that would be required for
macvlan interfaces.   Since the bonds are implicitly created in the host
namespace, the only way we know to get bonds directly into the
namespaces is to move them.

Would "NETDEV_UNREGISTER and NETDEV_REGISTER events to remove/add the
per device proc files at the appropriate time." help in the case?


-DilipD.


> However since moving bonding devices appears to be an unneded operation
> let's just do things simply and forbid moving bonding devices between
> network namespaces.  Serge, Dilip can you two test the patch below
> and see if it fixes the warnings.
> 
> Eric
> 
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 2ee8cf9..818ed64 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4345,6 +4345,9 @@ static void bond_setup(struct net_device *bond_dev)
>         bond_dev->priv_flags |= IFF_BONDING;
>         bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
>  
> +       /* Don't allow bond devices to change network namespaces. */
> +       bond_dev->features |= NETIF_F_LOCAL;
> +
>         /* At first, we block adding VLANs. That's the only way to
>          * prevent problems that occur when adding VLANs over an
>          * empty bond. The block will be removed once non-challenged

Re: Network namespace and bonding WARNING at fs/proc/generic.c remove_proc_entry

[Eric W. Biederman]

Dilip Daya <dilip.daya@hp.com> writes:

> Hi Eric,

> We do need to move bonds between namespaces - because we require
> physical interfaces in each namespace -- we don't want the overheads of
> virtual interfaces, don't have the management infrastructure, and don't
> want to manufacture fake mac addresses that would be required for
> macvlan interfaces.   Since the bonds are implicitly created in the host
> namespace, the only way we know to get bonds directly into the
> namespaces is to move them.

There about 3 ways to create bonding devices.  One of those ways
is to create bonding devices when loading the module.  Another
way is to create a bond device with "echo '+bond35 > /sys/class/net/bonding_masters".
them when loading the module, and my favorite is the standard way
"ip link add type bond".  All but loading the bonding device work in the
network namespace you are in at the type.

> Would "NETDEV_UNREGISTER and NETDEV_REGISTER events to remove/add the
> per device proc files at the appropriate time." help in the case?

Yes.  But since you can create the bonding device in the network
namespace you need it in, I don't see the point, of adding a code
path no one will test for 3 years at a time.

It seems easier to me to just not allow migration of bonding devices
and set peoples expectations a little lower.  Especially given
the very complex user space interfaces.

On ther other hand if you want to write and test and generally own the
patch I will review it.

Eric