Date:	Wed, 11 Jul 2012 18:57:33 -0700
From:	Jay Vosburgh <fubar@...ibm.com>
To:	ebiederm@...ssion.com (Eric W. Biederman)
cc:	netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] bonding: debugfs and network namespaces are incompatible

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

>Jay Vosburgh <fubar@...ibm.com> writes:
>
>> Eric W. Biederman <ebiederm@...ssion.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@...ibm.com

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists