Re: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
On Tue, 1 Jan 2008, Greg KH wrote: On Mon, Dec 31, 2007 at 11:26:43AM -0800, Greg KH wrote: On Mon, Dec 31, 2007 at 12:49:52PM -0500, Alan Stern wrote: On Sun, 30 Dec 2007, Greg KH wrote: It looks like Greg misused the debugfs API -- which is ironic, because he wrote debugfs in the first place! :-) Ok, no, I didn't write that patch, I'm getting very confused here. In 2.6.24-rc6 there is no usage of debugfs in the ohci driver. In the -mm tree there is a patch, from Tony Jones, that moves some debug code out of sysfs and into debugfs where it belongs. It does it for both the ehci and ohci USB host controller drivers, and this is the code that is incorrect if CONFIG_DEBUGFS is not enabled. My mistake; I got the impression you had written that new code rather than Tony. BTW, I don't recall ever seeing Tony's patch announced on linux-usb or linux-usb-devel. Did I simply miss it? So, for the 2.6.24 release, nothing needs to be changed, all is good, and there is no regression. Right? Or am I still confused about this whole thing? Correct. The problem exists only in -mm and your development tree. I will go fix up Tony's patches in the -mm tree that do not handle the error return values from debugfs properly, but that is not such a rush, as Tony is on vacation for a few weeks, and those patches are going to be going in only after 2.6.24 is out. The fix I posted earlier in this thread should simply be merged into Tony's patches. Alan Stern -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
On Wednesday 02 January 2008, Alan Stern wrote: BTW, I don't recall ever seeing Tony's patch announced on linux-usb or linux-usb-devel. Did I simply miss it? I think he didn't post it. I got some questions from him at one point, which I answered, but as I recall he decided for some reason to stop that work. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
On Mon, Dec 31, 2007 at 11:26:43AM -0800, Greg KH wrote: On Mon, Dec 31, 2007 at 12:49:52PM -0500, Alan Stern wrote: On Sun, 30 Dec 2007, Greg KH wrote: It looks like Greg misused the debugfs API -- which is ironic, because he wrote debugfs in the first place! :-) Oh crap, sorry, I did mess that up :( Ok, no, I didn't write that patch, I'm getting very confused here. In 2.6.24-rc6 there is no usage of debugfs in the ohci driver. In the -mm tree there is a patch, from Tony Jones, that moves some debug code out of sysfs and into debugfs where it belongs. It does it for both the ehci and ohci USB host controller drivers, and this is the code that is incorrect if CONFIG_DEBUGFS is not enabled. So, for the 2.6.24 release, nothing needs to be changed, all is good, and there is no regression. Right? Or am I still confused about this whole thing? I will go fix up Tony's patches in the -mm tree that do not handle the error return values from debugfs properly, but that is not such a rush, as Tony is on vacation for a few weeks, and those patches are going to be going in only after 2.6.24 is out. Hm, I wonder if this means I can go back to drinking more holiday wine... :) thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Hi, On Sun, Dec 30, 2007 at 03:34:45PM -0500, Alan Stern wrote: It looks like Greg misused the debugfs API -- which is ironic, because he wrote debugfs in the first place! :-) Let me know if this patch fixes the problem. If it does, I'll submit it to Greg with all the proper accoutrements. Finally a 2.6.24-rc6-mm1 with working USB WLAN. :) IOW I can confirm that this was the cause of the USB problem I was having. Thanks! Andreas Mohr -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Hi, On Tue, Jan 01, 2008 at 10:00:06PM -0800, Greg KH wrote: Ok, no, I didn't write that patch, I'm getting very confused here. In 2.6.24-rc6 there is no usage of debugfs in the ohci driver. In the -mm tree there is a patch, from Tony Jones, that moves some debug code out of sysfs and into debugfs where it belongs. It does it for both the ehci and ohci USB host controller drivers, and this is the code that is incorrect if CONFIG_DEBUGFS is not enabled. So, for the 2.6.24 release, nothing needs to be changed, all is good, and there is no regression. Right? Or am I still confused about this whole thing? Probably, since I also wasn't sure about this getting added to the post-2.6.23 regressions. I'd expect this problem to be -mm only myself, but then I didn't actively verify it in code (and I haven't tried -rc6 proper on that machine yet either, build takes ages ;). OK, since I cannot really offer anything other than positive feelings about this being non-rc6 breakage, should I try -rc6 proper, too? Hm, I wonder if this means I can go back to drinking more holiday wine... :) .even more confusion? :) Thanks for your help, Andreas Mohr -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
On Wed, Jan 02, 2008 at 07:13:08AM +0100, Andreas Mohr wrote: Hi, On Tue, Jan 01, 2008 at 10:00:06PM -0800, Greg KH wrote: Ok, no, I didn't write that patch, I'm getting very confused here. In 2.6.24-rc6 there is no usage of debugfs in the ohci driver. In the -mm tree there is a patch, from Tony Jones, that moves some debug code out of sysfs and into debugfs where it belongs. It does it for both the ehci and ohci USB host controller drivers, and this is the code that is incorrect if CONFIG_DEBUGFS is not enabled. So, for the 2.6.24 release, nothing needs to be changed, all is good, and there is no regression. Right? Or am I still confused about this whole thing? Probably, since I also wasn't sure about this getting added to the post-2.6.23 regressions. I'd expect this problem to be -mm only myself, but then I didn't actively verify it in code (and I haven't tried -rc6 proper on that machine yet either, build takes ages ;). OK, since I cannot really offer anything other than positive feelings about this being non-rc6 breakage, should I try -rc6 proper, too? Please do, that would be very helpful. As the code being discussed isn't even in Linus's tree, it's hard to see how this could be a proposed fix for the regression :) thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Andrew Morton wrote: On Fri, 07 Dec 2007 04:51:37 + David Woodhouse [EMAIL PROTECTED] wrote: On Mon, 2007-11-26 at 15:17 -0700, Eric W. Biederman wrote: Well I clearly goofed when I added the initial network namespace support for /proc/net. Currently things work but there are odd details visible to user space, even when we have a single network namespace. Since we do not cache proc_dir_entry dentries at the moment we can just modify -lookup to return a different directory inode depending on the network namespace of the process looking at /proc/net, replacing the current technique of using a magic and fragile follow_link method. To accomplish that this patch: - introduces a shadow_proc method to allow different dentries to be returned from proc_lookup. - Removes the old /proc/net follow_link magic - Fixes a weakness in our not caching of proc generic dentries. As shadow_proc uses a task struct to decided which dentry to return we can go back later and fix the proc generic caching without modifying any code that uses the shadow_proc method. Signed-off-by: Eric W. Biederman [EMAIL PROTECTED] --- fs/proc/generic.c | 12 ++- fs/proc/proc_net.c | 86 +++ include/linux/proc_fs.h |3 ++ 3 files changed, 19 insertions(+), 82 deletions(-) (commit 2b1e300a9dfc3196ccddf6f1d74b91b7af55e416) This seems to have broken the use of /proc/bus/usb as a mountpoint. It always appears empty now, whatever's supposed to be mounted there. Yes. Denis and Eric are tossing around competing patches but afaik nobody is happy with any of them. Guys, could we get this sorted soonish please? Andrew, I become too relaxed after receiving Tested-by: Giacomo Catenazzi [EMAIL PROTECTED] Eric, I believe that reverting an original behavior is better than your new one as - you introduce search into the depth by calling have_submounts(dentry) during revalidation for all(!) /proc dentries - your shadowing behavior will be broken if you'll mount something in the depth of shadowed tree (this can be done as a DoS attempt) As a last minute call, may be it will be better to pin network namespace like a pid namespace during mount to avoid this crap at all? Regards, Den -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
On Fri, 07 Dec 2007 04:51:37 + David Woodhouse [EMAIL PROTECTED] wrote: On Mon, 2007-11-26 at 15:17 -0700, Eric W. Biederman wrote: Well I clearly goofed when I added the initial network namespace support for /proc/net. Currently things work but there are odd details visible to user space, even when we have a single network namespace. Since we do not cache proc_dir_entry dentries at the moment we can just modify -lookup to return a different directory inode depending on the network namespace of the process looking at /proc/net, replacing the current technique of using a magic and fragile follow_link method. To accomplish that this patch: - introduces a shadow_proc method to allow different dentries to be returned from proc_lookup. - Removes the old /proc/net follow_link magic - Fixes a weakness in our not caching of proc generic dentries. As shadow_proc uses a task struct to decided which dentry to return we can go back later and fix the proc generic caching without modifying any code that uses the shadow_proc method. Signed-off-by: Eric W. Biederman [EMAIL PROTECTED] --- fs/proc/generic.c | 12 ++- fs/proc/proc_net.c | 86 +++ include/linux/proc_fs.h |3 ++ 3 files changed, 19 insertions(+), 82 deletions(-) (commit 2b1e300a9dfc3196ccddf6f1d74b91b7af55e416) This seems to have broken the use of /proc/bus/usb as a mountpoint. It always appears empty now, whatever's supposed to be mounted there. Yes. Denis and Eric are tossing around competing patches but afaik nobody is happy with any of them. Guys, could we get this sorted soonish please? -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
On Mon, 2007-11-26 at 15:17 -0700, Eric W. Biederman wrote: Well I clearly goofed when I added the initial network namespace support for /proc/net. Currently things work but there are odd details visible to user space, even when we have a single network namespace. Since we do not cache proc_dir_entry dentries at the moment we can just modify -lookup to return a different directory inode depending on the network namespace of the process looking at /proc/net, replacing the current technique of using a magic and fragile follow_link method. To accomplish that this patch: - introduces a shadow_proc method to allow different dentries to be returned from proc_lookup. - Removes the old /proc/net follow_link magic - Fixes a weakness in our not caching of proc generic dentries. As shadow_proc uses a task struct to decided which dentry to return we can go back later and fix the proc generic caching without modifying any code that uses the shadow_proc method. Signed-off-by: Eric W. Biederman [EMAIL PROTECTED] --- fs/proc/generic.c | 12 ++- fs/proc/proc_net.c | 86 +++ include/linux/proc_fs.h |3 ++ 3 files changed, 19 insertions(+), 82 deletions(-) (commit 2b1e300a9dfc3196ccddf6f1d74b91b7af55e416) This seems to have broken the use of /proc/bus/usb as a mountpoint. It always appears empty now, whatever's supposed to be mounted there. -- dwmw2 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
[snip]
Well I clearly goofed when I added the initial network namespace support
for /proc/net. Currently things work but there are odd details visible
to user space, even when we have a single network namespace.
Since we do not cache proc_dir_entry dentries at the moment we can
just modify -lookup to return a different directory inode depending
on the network namespace of the process looking at /proc/net, replacing
the current technique of using a magic and fragile follow_link method.
To accomplish that this patch:
- introduces a shadow_proc method to allow different dentries to
be returned from proc_lookup.
- Removes the old /proc/net follow_link magic
- Fixes a weakness in our not caching of proc generic dentries.
As shadow_proc uses a task struct to decided which dentry to return we
can go back later and fix the proc generic caching without modifying any code
that
uses the shadow_proc method.
Signed-off-by: Eric W. Biederman [EMAIL PROTECTED]
Thanks, Eric.
Much better ('find /proc' works and so does 'ls ..'), but one
issue is still unsolved :(
I mentioned the program, that opens the directory and dumps the
content of the /proc/self/fd. Here it is (stupid but simple):
prog.c
#include stdio.h
#include unistd.h
#include stdlib.h
#include asm/fcntl.h
#include unistd.h
int main(int argc, char **argv)
{
int fd;
fd = open(argv[1], O_RDONLY|O_DIRECTORY);
if (fd == -1) {
perror(Can't open);
return 1;
}
system(ls -l /proc/self/fd);
return 0;
}
So. Here's the result of running this program:
# cd /proc/net/
# pwd
/proc/net
# ~/a.out .
total 0
lrwx-- 1 root root 64 Nov 27 13:27 0 - /dev/pts/0
lrwx-- 1 root root 64 Nov 27 13:27 1 - /dev/pts/0
lrwx-- 1 root root 64 Nov 27 13:27 2 - /dev/pts/0
lr-x-- 1 root root 64 Nov 27 13:27 3 - /proc/net (deleted)
lr-x-- 1 root root 64 Nov 27 13:27 4 - /proc/4475/fd
# cd /proc
# pwd
/proc
# ~/a.out net
total 0
lrwx-- 1 root root 64 Nov 27 13:27 0 - /dev/pts/0
lrwx-- 1 root root 64 Nov 27 13:27 1 - /dev/pts/0
lrwx-- 1 root root 64 Nov 27 13:27 2 - /dev/pts/0
lr-x-- 1 root root 64 Nov 27 13:27 3 - /proc/net
lr-x-- 1 root root 64 Nov 27 13:27 4 - /proc/4477/fd
# cd /proc/net/stat
# pwd
/proc/net/stat
# ~/a.out ..
total 0
lrwx-- 1 root root 64 Nov 27 13:29 0 - /dev/pts/0
lrwx-- 1 root root 64 Nov 27 13:29 1 - /dev/pts/0
lrwx-- 1 root root 64 Nov 27 13:29 2 - /dev/pts/0
lr-x-- 1 root root 64 Nov 27 13:29 3 - /proc/net (deleted)
lr-x-- 1 root root 64 Nov 27 13:29 4 - /proc/4482/fd
# ~/a.out .
total 0
lrwx-- 1 root root 64 Nov 27 13:32 0 - /dev/pts/0
lrwx-- 1 root root 64 Nov 27 13:32 1 - /dev/pts/0
lrwx-- 1 root root 64 Nov 27 13:32 2 - /dev/pts/0
lr-x-- 1 root root 64 Nov 27 13:32 3 - /proc/net/stat
lr-x-- 1 root root 64 Nov 27 13:32 4 - /proc/4488/fd
Bad thing is that . when cdir is /proc/net and .. when cdir is
anything under /proc/net (i.e. the /proc/net itself) is marked as (deleted).
[snip]
Thanks,
Pavel
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Pavel Emelyanov [EMAIL PROTECTED] writes:
[snip]
Well I clearly goofed when I added the initial network namespace support
for /proc/net. Currently things work but there are odd details visible
to user space, even when we have a single network namespace.
Since we do not cache proc_dir_entry dentries at the moment we can
just modify -lookup to return a different directory inode depending
on the network namespace of the process looking at /proc/net, replacing
the current technique of using a magic and fragile follow_link method.
To accomplish that this patch:
- introduces a shadow_proc method to allow different dentries to
be returned from proc_lookup.
- Removes the old /proc/net follow_link magic
- Fixes a weakness in our not caching of proc generic dentries.
As shadow_proc uses a task struct to decided which dentry to return we
can go back later and fix the proc generic caching without modifying any code
that
uses the shadow_proc method.
Signed-off-by: Eric W. Biederman [EMAIL PROTECTED]
Thanks, Eric.
Much better ('find /proc' works and so does 'ls ..'), but one
issue is still unsolved :(
I mentioned the program, that opens the directory and dumps the
content of the /proc/self/fd. Here it is (stupid but simple):
The cause is totally different this time, and is not limited
to /proc/net. But this is the only side effect of the changes now.
I used the one line version of your test program to confirm this:
$ cd /proc/driver/
$ ls -l /proc/self/fd/100 100 .
lr-x-- 1 eric eric 64 Nov 27 05:06 /proc/self/fd/100 - /proc/driver/
$ ls -l /proc/self/fd/100 100 .
lr-x-- 1 eric eric 64 Nov 27 05:07 /proc/self/fd/100 - /proc/driver
(deleted)
What is happening is that the aggressive non-caching logic is dropping
the dentries and thus they show up as deleted. I.e. When something
triggers another lookup of that dentry revalidate drops the old
dentry.
This actually fixes a race in /proc today where if you open a file,
remove the module for it, reload the module for that file, and then
attempt to access it, lookup will return the old dentry with the
old fops (even if there is not a current version of that file).
kill_proc_inodes current keeps us from using the old version of
the file_operations for directories (because it removes them) but
it does not prevent this DOS attack where keeping a proc file open
always ensures everyone will see the old version.
Given that the behavior seems more correct then what we have currently
I can live with files showing up as deleted from time to time.
Ultimately the fix is to correct the caching logic in
fs/proc/generic.c to only drop dentries when necessary and then
the deleted markers will only show up when the file or directory
really goes away.
I don't expect user space will care about the semi-legitimate
(deleted) marks. So I don't think this one down side will be a big
deal for 2.6.24.
Eric
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2.6.24-rc3] Fix /proc/net breakage
Pavel Emelyanov [EMAIL PROTECTED] writes:
Rafael J. Wysocki wrote:
On Monday, 19 of November 2007, Pavel Machek wrote:
Hi!
I think that this worked before:
[EMAIL PROTECTED]:/proc# find . -name timer_info
find: WARNING: Hard link count is wrong for ./net: this may be a bug
in your filesystem driver. Automatically turning on find's -noleaf
option. Earlier results may have failed to include directories that
should have been searched.
[EMAIL PROTECTED]:/proc#
I'm seeing that too.
I have a better things with 2.6.24-rc3 ;)
# cd /proc/net
# ls ..
ls: reading directory ..: Not a directory
and this
# cd /proc
# find
...
./net
find: . changed during execution of find
# find net
find: net changed during execution of find
# find net/
this works ok however
Moreover. Program that opens /proc/net and dumps the /proc/self/fd
files produces the following:
# cd /
# a.out /proc/net
...
lr-x-- 1 root root 64 Nov 20 18:02 3 - /proc/net/net (deleted)
...
# cd /proc/net
# a.out .
...
lr-x-- 1 root root 64 Nov 20 18:03 3 - /proc/net/net (deleted)
...
# a.out ..
...
lr-x-- 1 root root 64 Nov 20 18:03 3 - /proc/net
...
Well I clearly goofed when I added the initial network namespace support
for /proc/net. Currently things work but there are odd details visible
to user space, even when we have a single network namespace.
Since we do not cache proc_dir_entry dentries at the moment we can
just modify -lookup to return a different directory inode depending
on the network namespace of the process looking at /proc/net, replacing
the current technique of using a magic and fragile follow_link method.
To accomplish that this patch:
- introduces a shadow_proc method to allow different dentries to
be returned from proc_lookup.
- Removes the old /proc/net follow_link magic
- Fixes a weakness in our not caching of proc generic dentries.
As shadow_proc uses a task struct to decided which dentry to return we
can go back later and fix the proc generic caching without modifying any code
that
uses the shadow_proc method.
Signed-off-by: Eric W. Biederman [EMAIL PROTECTED]
---
fs/proc/generic.c | 12 ++-
fs/proc/proc_net.c | 86 +++
include/linux/proc_fs.h |3 ++
3 files changed, 19 insertions(+), 82 deletions(-)
diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index a9806bc..c2b7523 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -374,9 +374,16 @@ static int proc_delete_dentry(struct dentry * dentry)
return 1;
}
+static int proc_revalidate_dentry(struct dentry *dentry, struct nameidata *nd)
+{
+ d_drop(dentry);
+ return 0;
+}
+
static struct dentry_operations proc_dentry_operations =
{
.d_delete = proc_delete_dentry,
+ .d_revalidate = proc_revalidate_dentry,
};
/*
@@ -397,8 +404,11 @@ struct dentry *proc_lookup(struct inode * dir, struct
dentry *dentry, struct nam
if (de-namelen != dentry-d_name.len)
continue;
if (!memcmp(dentry-d_name.name, de-name,
de-namelen)) {
- unsigned int ino = de-low_ino;
+ unsigned int ino;
+ if (de-shadow_proc)
+ de = de-shadow_proc(current, de);
+ ino = de-low_ino;
de_get(de);
spin_unlock(proc_subdir_lock);
error = -EINVAL;
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
index 131f9c6..0afe21e 100644
--- a/fs/proc/proc_net.c
+++ b/fs/proc/proc_net.c
@@ -50,89 +50,14 @@ struct net *get_proc_net(const struct inode *inode)
}
EXPORT_SYMBOL_GPL(get_proc_net);
-static struct proc_dir_entry *proc_net_shadow;
+static struct proc_dir_entry *shadow_pde;
-static struct dentry *proc_net_shadow_dentry(struct dentry *parent,
+static struct proc_dir_entry *proc_net_shadow(struct task_struct *task,
struct proc_dir_entry *de)
{
- struct dentry *shadow = NULL;
- struct inode *inode;
- if (!de)
- goto out;
- de_get(de);
- inode = proc_get_inode(parent-d_inode-i_sb, de-low_ino, de);
- if (!inode)
- goto out_de_put;
- shadow = d_alloc_name(parent, de-name);
- if (!shadow)
- goto out_iput;
- shadow-d_op = parent-d_op; /* proc_dentry_operations */
- d_instantiate(shadow, inode);
-out:
- return shadow;
-out_iput:
- iput(inode);
-out_de_put:
- de_put(de);
- goto out;
-}
-
-static void *proc_net_follow_link(struct dentry *parent, struct nameidata *nd)
-{
- struct net *net = current-nsproxy-net_ns;
- struct dentry *shadow;
- shadow = proc_net_shadow_dentry(parent, net-proc_net);
- if (!shadow)
-
