2.6.24-rc3: find complains about /proc/net
Subsystems (6)
Hi!
I think that this worked before:
root@amd:/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. root@amd:/proc#
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 10 lines · Pavel Machek
Hi!
I think that this worked before:
root@amd:/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. root@amd:/proc#
I'm seeing that too.
Rafael
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Pavel Machek
Hi!
I think that this worked before:
Quoted 8 lines
Quoted 6 lines
root@amd:/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. root@amd:/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 ...
This all is somehow related to the shadow proc files.
E.g. the first problem (with -ENOTDIR) is due to the shadow /proc/net dentry doesn't implement the .readdir method:
static const struct file_operations proc_net_dir_operations = {
.read = generic_read_dir,
};
And I haven't managed to find out why the rest problems occur...
Eric, do you have fixes for it?
Quoted 6 lines
Rafael
-
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
Quoted 13 lines · Pavel Emelyanov
Quoted 12 lines · Rafael J. Wysocki
Quoted 10 lines · Pavel Machek
Hi!
I think that this worked before:
root@amd:/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. root@amd:/proc#
Quoted 7 lines
Quoted 1 line
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
Ok. That part is truly a bug.
Looks like you have tracked down the cause.
Grumble you are getting the wrong .. :(
Quoted 26 lines
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/netand dumps the/proc/self/fdfiles 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) ...
Quoted 4 lines
# a.out .. ... lr-x------ 1 root root 64 Nov 20 18:03 3 -> /proc/net ...
Yes all of those are nasty. So much for my clever way of implementing these things. Grr. Simple hacks that almost work!
Quoted 4 lines
This all is somehow related to the shadow proc files.
E.g. the first problem (with-ENOTDIR) is due to the shadow/proc/netdentry doesn't implement the .readdir method:
Quoted 8 lines
static const struct
file_operationsproc_net_dir_operations= {
.read =generic_read_dir,
};And I haven't managed to find out why the rest problems occur...
Eric, do you have fixes for it?
Not exactly. It is tricky. I have known there are issues but so far the difficulty of a better solution has been higher then my annoyance level with this problem.
A special solution for !CONFIG_NET_NS may be practical for 2.6.24.
The only way I know of to really solve this problem cleanly and completely is to make /proc/net an explicit symlink to /proc/self/net and make /proc/<pid>/net a magic mountpoint (ala nfs automounts) that mounts a per network namespace filesystem. Al Viro wasn't to happy when I suggested it (mostly because he was convinced such a solution was likely to be full of races).
The half assed clean solution is to ensure nothing under /proc/net gets cached and ensure the dentry tree is built properly, for the current reader of /proc.
A third option is to fix .. in /proc/net. Although I'm a bit dubious if that will do more then fix a few symptoms with the current solution.
Eric
Quoted 5 lines · Eric W. Biederman
Quoted 2 lines
lr-x------ 1 root root 64 Nov 20 18:03 3 ->
/proc/net
...Yes all of those are nasty. So much for my clever way of implementing these things. Grr. Simple hacks that almost work!
btw., in case you feel inclined, i recently did some userspace coding and found to my surprise that /proc/self points to the parent task, not the thread itself (giving threads no real way to examine themselves). If you are hacking in this area, would it be a big trouble to add something like /proc/self-task/ or something like that? I had to use a raw gettid syscall to figure out the TID to get to /proc/*/tasks/TID/sched instrumentation info - which is quite a PITA.
Ingo
Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Eric W. Biederman
Quoted 2 lines
lr-x------ 1 root root 64 Nov 20 18:03 3 ->
/proc/net
...Yes all of those are nasty. So much for my clever way of implementing these things. Grr. Simple hacks that almost work!
Quoted 7 lines
btw., in case you feel inclined, i recently did some userspace coding and found to my surprise that
/proc/selfpoints to the parent task, not the thread itself (giving threads no real way to examine themselves). If you are hacking in this area, would it be a big trouble to add something like/proc/self-task/or something like that? I had to use a raw gettid syscall to figure out the TID to get to/proc/*/tasks/TID/schedinstrumentation info - which is quite a PITA.
Agreed. I have been debating with myself in the last couple of days if it is a bug that /proc/self uses the tgid and not the actual pid/tid value.
If I can be convinced that posix threads don't care I will happily just switch /proc/self, calling the current implementation a bug.
I think it is a bug the real question is what are the backwards compatibility implications. Do posix threads care?
It appears to me that either we need to fix /proc/self or we need to add /proc/task-self and fix /proc/mounts to point at that.
In the normal case we share all of the same things so I think it is a don't care. Except that /proc/self/status | grep Pid returns the tgid.
Hmm. I think I am just going to send Andrew a patch for 2.6.25 that just fixes /proc/self. I just fail to see how using the tgid is correct. The only cases we could care seem to do the wrong thing when we use the tgid.
Eric
these are all questions for Ulrich and Roland - Cc:-ed them.
Quoted 39 lines · Eric W. Biederman
Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Eric W. Biederman
Quoted 2 lines
lr-x------ 1 root root 64 Nov 20 18:03 3 ->
/proc/net
...Yes all of those are nasty. So much for my clever way of implementing these things. Grr. Simple hacks that almost work!
Quoted 7 lines
btw., in case you feel inclined, i recently did some userspace coding and found to my surprise that
/proc/selfpoints to the parent task, not the thread itself (giving threads no real way to examine themselves). If you are hacking in this area, would it be a big trouble to add something like/proc/self-task/or something like that? I had to use a raw gettid syscall to figure out the TID to get to/proc/*/tasks/TID/schedinstrumentation info - which is quite a PITA.Agreed. I have been debating with myself in the last couple of days if it is a bug that
/proc/selfuses the tgid and not the actual pid/tid value.If I can be convinced that posix threads don't care I will happily just switch
/proc/self, calling the current implementation a bug.I think it is a bug the real question is what are the backwards compatibility implications. Do posix threads care?
It appears to me that either we need to fix
/proc/selfor we need to add/proc/task-selfand fix/proc/mountsto point at that.In the normal case we share all of the same things so I think it is a don't care. Except that
/proc/self/status| grep Pid returns the tgid.Hmm. I think I am just going to send Andrew a patch for 2.6.25 that just fixes
/proc/self. I just fail to see how using the tgid is correct. The only cases we could care seem to do the wrong thing when we use the tgid.
When did /proc/self get changed to follow tgid instead of pid? glibc uses /proc/self to refer to various things that are usually shared anyway (fd, maps, cwd, exe), but I think the expectation has always been that this refers to the same calling thread, not the group leader. e.g., if one thread has changed uids so it no longer has access to the group leader's /proc/PID/fd, suddenly it using /proc/self/fd starts failing.
Thanks,
Roland
Quoted 7 lines · Roland McGrath
When did
/proc/selfget changed to follow tgid instead of pid? glibc uses/proc/selfto refer to various things that are usually shared anyway (fd, maps, cwd, exe), but I think the expectation has always been that this refers to the same calling thread, not the group leader. e.g., if one thread has changed uids so it no longer has access to the group leader's/proc/PID/fd, suddenly it using/proc/self/fdstarts failing.
i guess it was a v2.6.24 change, hence a regression that needs to be fixed?
Ingo
Quoted 2 lines · Ingo Molnar
i guess it was a v2.6.24 change, hence a regression that needs to be fixed?
It seems to be
http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commitdiff;h=01660410
So, linux 2.6.0-test6
Signature · 1 line
Guillaume
Oh, it seems it has indeed been that way for a very long time, so I was mistaken. It still seems a little odd to me. Ulrich can say definitively whether the kind of concern I mentioned really matters one way or the other for glibc.
Thanks,
Roland
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 4 lines · Roland McGrath
Oh, it seems it has indeed been that way for a very long time, so I was mistaken. It still seems a little odd to me. Ulrich can say definitively whether the kind of concern I mentioned really matters one way or the other for glibc.
glibc cannot survive (at least NPTL) if somebody uses funny CLONE_* flags to separate various pieces of information, e.g., file descriptors.
So, all the information in each thread's /proc/self should be identical.
When the information is not the same, the current semantics seems to be more useful. So I guess, no change is the way to go here.
- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖ -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org
iD8DBQFHQ25/2ijCOnn/RHQRAmhhAJsHRF7FqO8DWwZ97gHxIO/i4Z1AAQCffCGa Q2J8kjthKbbNQf1USWMAw3Y=
=xl/a
-----END PGP SIGNATURE-----
Quoted 5 lines · Ulrich Drepper
Quoted 4 lines
Oh, it seems it has indeed been that way for a very long time, so I was mistaken. It still seems a little odd to me. Ulrich can say definitively whether the kind of concern I mentioned really matters one way or the other for glibc.
Quoted 8 lines
glibc cannot survive (at least NPTL) if somebody uses funny CLONE_* flags to separate various pieces of information, e.g., file descriptors.
So, all the information in each thread's/proc/selfshould be identical.When the information is not the same, the current semantics seems to be more useful. So I guess, no change is the way to go here.
can you see any danger to providing a /proc/self_task/ link? (or can you think of a better name/API/approach)
Ingo
Quoted 2 lines
can you see any danger to providing a
/proc/self_task/link? (or can you think of a better name/API/approach)
That is a poor name to choose given /proc/self/task exists as something else (just try writing a sentence comparing them and then read it aloud). Probably /proc/self/task/self is what makes the most sense structurally. I don't know if it matters to whatever use you are concerned with to have two more steps in the lookup.
Thanks,
Roland
Quoted 8 lines · Roland McGrath
Quoted 2 lines
can you see any danger to providing a
/proc/self_task/link? (or can you think of a better name/API/approach)That is a poor name to choose given
/proc/self/taskexists as something else (just try writing a sentence comparing them and then read it aloud). Probably/proc/self/task/selfis what makes the most sense structurally. I don't know if it matters to whatever use you are concerned with to have two more steps in the lookup.
Hm, /proc/this_thread maybe?
Rafael
Quoted 8 lines · Roland McGrath
Quoted 2 lines
can you see any danger to providing a
/proc/self_task/link? (or can you think of a better name/API/approach)That is a poor name to choose given
/proc/self/taskexists as something else (just try writing a sentence comparing them and then read it aloud). Probably/proc/self/task/selfis what makes the most sense structurally. I don't know if it matters to whatever use you are concerned with to have two more steps in the lookup.
Well the only case it could matter is if you aren't allowed to access /proc/<tgid> which I think may actually be the current selinux behavior.
So if we can't fix /proc/self we need to introduce /proc/task-self at the top level, just to be certain we don't run into weird cases like that. Otherwise /proc/self/task/self sounds like a wonderful suggestion.
Eric
Quoted 9 lines · Ulrich Drepper
Quoted 4 lines · Roland McGrath
Oh, it seems it has indeed been that way for a very long time, so I was mistaken. It still seems a little odd to me. Ulrich can say definitively whether the kind of concern I mentioned really matters one way or the other for glibc.
glibc cannot survive (at least NPTL) if somebody uses funny CLONE_* flags to separate various pieces of information, e.g., file descriptors.
So, all the information in each thread's/proc/selfshould be identical.
Which seems to confirm that glibc and native pthread can't care.
Quoted 2 lines
When the information is not the same, the current semantics seems to be more useful. So I guess, no change is the way to go here.
Could you elaborate a bit on how the semantics of returning the wrong information are more useful?
In particular if a thread does the logical equivalent of:
grep Pid: /proc/self/status. It always get the tgid despite having a different process id.
How can that possibly be useful or correct?
Quoted 1 line
From the kernel side I really think the current semantics of
/proc/self
in the context of threads is a bug and confusing. All of the kernel developers first reaction when this was pointed out was that this is a regression.
If it is truly useful to user space we can preserve this API design bug forever. I just want to make certain we are not being bug compatible without a good reason.
Currently we have several kernel side bugs with threaded programs because /proc/self does not do the intuitive thing. Unless something has changed recently selinux will cause accesses by a non-leader thread to fail when accessing files through /proc/self.
So far the more I look at the current /proc/self behavior the more I am convinced it is broken, and useless. Please help me see where it is useful, so we can justify keeping it.
Thanks,
Eric
Quoted 9 lines · Guillaume Chazarain
Quoted 2 lines · Ingo Molnar
i guess it was a v2.6.24 change, hence a regression that needs to be fixed?
It seems to be
http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commitdiff;h=01660410
So, linux 2.6.0-test6
grumble :-/ So i guess /proc/self-task it has to be.
Ingo
PATCH 1/1 proc: Fix the threaded /proc/self.
In reply to Ingo MolnarLong ago when the CLONE_THREAD support first went it someone thought it would be wise to point /proc/self at /proc/<tgid> instead of /proc/<pid>.
Given that /proc/<tgid> can return information about a very different task (if enough things have been unshared) then our current process /proc/<tgid> seems blatantly wrong. So far I have yet to think up an example where the current behavior would be advantageous, and I can see several places where it is seriously non-intuitive.
We may be stuck with the current broken behavior for backwards compatibility reasons but lets try fixing our ancient bug for the 2.6.25 time frame and see if anyone screams.
--- fs/proc/base.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
fs/proc/base.c +6 -6
@@ -2050,22 +2050,22 @@ static int proc_self_readlink(struct dentry *dentry, char __user *buffer, int buflen) {struct pid_namespace *ns = dentry->d_sb->s_fs_info; -pid_t tgid =task_tgid_nr_ns (current, ns); +pid_t pid =task_pid_nr_ns (current, ns);char tmp[PROC_NUMBUF ]; -if (!tgid) +if (!pid)return -ENOENT ; -sprintf (tmp,"%d" , tgid); +sprintf (tmp,"%d" , pid);return vfs_readlink (dentry,buffer,buflen,tmp); }static void *proc_self_follow_link (struct dentry *dentry,struct nameidata *nd) {struct pid_namespace *ns = dentry->d_sb->s_fs_info; -pid_t tgid =task_tgid_nr_ns (current, ns); +pid_t pid =task_pid_nr_ns (current, ns);char tmp[PROC_NUMBUF ]; -if (!tgid) +if (!pid)return ERR_PTR (-ENOENT ); -sprintf (tmp,"%d" ,task_tgid_nr_ns (current, ns)); +sprintf (tmp,"%d" , pid);return ERR_PTR (vfs_follow_link (nd,tmp)); }
Signature · 1 line
1.5.3.rc6.17.g1911
Re: [PATCH] proc: Fix the threaded /proc/self.
Hello Eric,
This fills a need I had to get the current TID in a Java program, so I'm very interested in this change. OTOH, how will someone not reading LKML discover that the current TID is now in /proc/self and that it was not always the case?
I would put my 2 cents in /proc/self/task/self, this way TGID are always in /proc and TID in /proc/TGID/task.
Signature · 1 line
Guillaume
Re: [PATCH] proc: Fix the threaded /proc/self.
In reply to Eric W. BiedermanQuoted 3 lines · Eric W. Biederman
We may be stuck with the current broken behavior for backwards compatibility reasons but lets try fixing our ancient bug for the 2.6.25 time frame and see if anyone screams.
to make sure i got you right - do you agree that this is a regression and that we need the patch below included in 2.6.24?
Quoted 1 line
Signed-off-by Eric W. Biederman <[email protected]>
Quoted 39 lines
--- fs/proc/base.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)fs/proc/base.c +6
-6@@ -2050,22 +2050,22 @@ static int proc_self_readlink(struct dentry *dentry, char __user *buffer, int buflen) {struct pid_namespace *ns = dentry->d_sb->s_fs_info; -pid_t tgid =task_tgid_nr_ns (current, ns); +pid_t pid =task_pid_nr_ns (current, ns);char tmp[PROC_NUMBUF ]; -if (!tgid) +if (!pid)return -ENOENT ; -sprintf (tmp,"%d" , tgid); +sprintf (tmp,"%d" , pid);return vfs_readlink (dentry,buffer,buflen,tmp); }static void *proc_self_follow_link (struct dentry *dentry,struct nameidata *nd) {struct pid_namespace *ns = dentry->d_sb->s_fs_info; -pid_t tgid =task_tgid_nr_ns (current, ns); +pid_t pid =task_pid_nr_ns (current, ns);char tmp[PROC_NUMBUF ]; -if (!tgid) +if (!pid)return ERR_PTR (-ENOENT ); -sprintf (tmp,"%d" ,task_tgid_nr_ns (current, ns)); +sprintf (tmp,"%d" , pid);return ERR_PTR (vfs_follow_link (nd,tmp)); }Signature · 1 line
1.5.3.rc6.17.g1911
Ingo
Quoted 55 lines · Pavel Emelyanov
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Pavel Machek
Hi!
I think that this worked before:
Quoted 8 lines
Quoted 6 lines
root@amd:/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. root@amd:/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 directoryand 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/netand dumps the/proc/self/fdfiles 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 ...This all is somehow related to the shadow proc files.
E.g. the first problem (with-ENOTDIR) is due to the shadow/proc/netdentry doesn't implement the .readdir method:
Quoted 8 lines
static const struct
file_operationsproc_net_dir_operations= {
.read =generic_read_dir,
};And I haven't managed to find out why the rest problems occur...
Eric, do you have fixes for it?
Duh. There is one other possible solution I forgot to mention and at least as a first pass it should be relatively simple. Have the mount of proc capture the network namespace. I'm not certain if it is what we want long term but it should be simple and relatively easy to implement.
I don't like capturing the network namespace when we mount proc but it is easier then implementing /proc/self/net. Which is the other real alternative.
Eric
Below is a preliminary patch. It solves the directory issue but it doesn't play well with proc_mnt and proc_flush_task. It works by simply caching the network namespace when we mount proc so we don't have to be fancy and dynamic.
Something for the discussion anyway.
I will start sorting out what makes sense tomorrow.
Eric
Quoted 1 line
From
f359fde2469ba8be2123a465e788a83c7e6831e9Mon Sep 17 00:00:00 2001
From: Eric W. Biederman <[email protected]>
Date: Tue, 20 Nov 2007 19:36:05 -0700
Subject: [PATCH] proc: Fix /proc/net directory listings.
Having proc dynamically display the contents of /proc/net is hard. So make life simpler by capturing the network namespace when we mount proc and only displaying that network namespace.
--- fs/proc/base.c | 8 ++-- fs/proc/generic.c | 4 ++- fs/proc/internal.h | 13 +++++++ fs/proc/proc_net.c | 89 ++++------------------------------------------- fs/proc/root.c | 50 ++++++++++++++++++-------- include/linux/proc_fs.h | 4 ++ 6 files changed, 66 insertions(+), 102 deletions(-)
fs/proc/base.c +4 -4
@@ -2395,7 +2395,7 @@ struct dentry *proc_pid_lookup(struct inode *dir, struct dentry * dentry, structif (tgid == ~0U ) goto out; - ns = dentry->d_sb->s_fs_info; + ns =proc_sbi (dentry->d_sb)->pid_ns;rcu_read_lock (); task =find_task_by_pid_ns (tgid, ns);if (task)
@@ -2476,7 +2476,7 @@ int proc_pid_readdir(struct file * filp, void * dirent, filldir_t filldir) goto out; } - ns = filp->f_dentry->d_sb->s_fs_info; + ns =proc_sbi (filp->f_dentry->d_sb)->pid_ns; tgid = filp->f_pos -TGID_OFFSET ; for (task = next_tgid(tgid, ns); task;
@@ -2615,7 +2615,7 @@ static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentryif (tid == ~0U ) goto out; - ns = dentry->d_sb->s_fs_info; + ns =proc_sbi (dentry->d_sb)->pid_ns;rcu_read_lock (); task =find_task_by_pid_ns (tid, ns);if (task)
@@ -2758,7 +2758,7 @@ static int proc_task_readdir(struct file * filp, void * dirent, filldir_t filldi/* f_version caches the tgid value that the last readdir call couldn't * return. lseek aka telldir automagically resets f_version to 0. */ - ns = filp->f_dentry->d_sb->s_fs_info; + ns =proc_sbi (filp->f_dentry->d_sb)->pid_ns; tid = (int )filp->f_version; filp->f_version =0 ; for (task = first_tid(leader, tid, pos - 2, ns);
fs/proc/generic.c +3 -1
@@ -398,7 +398,9 @@ struct dentry *proc_lookup(struct inode * dir, struct dentry *dentry, struct namcontinue ;if (!memcmp (dentry->d_name.name, de->name, de->namelen)) {unsigned int ino = de->low_ino; - + +if (de->shadow_proc) + de = de->shadow_proc (dentry->d_sb, de);de_get (de);spin_unlock (&proc_subdir_lock); error = -EINVAL ;
fs/proc/internal.h +13 -0
@@ -11,6 +11,18 @@#include <linux/proc_fs.h> +struct pid_namespace ; +struct net ; +struct proc_sb_info { +struct pid_namespace *pid_ns; +struct net *net_ns; +}; + +static inline struct proc_sb_info *proc_sbi (struct super_block *sb) +{ +return sb->s_fs_info; +} +#ifdef CONFIG_PROC_SYSCTL extern int proc_sys_init (void );#else
@@ -78,3 +90,4 @@ static inline int proc_fd(struct inode *inode) {return PROC_I (inode)->fd; } +
fs/proc/proc_net.c +7 -82
105 lines not on this page · raw message
fs/proc/root.c +35 -15
119 lines not on this page · raw message
include/linux/proc_fs.h +4 -0
@@ -48,6 +48,9 @@ typedef int (read_proc_t)(char *page, char **start, off_t off,typedef int (write_proc_t )(struct file *file,const char __user *buffer,unsigned long count,void *data);typedef int (get_info_t )(char *,char **,off_t ,int ); +struct proc_dir_entry ; +typedef struct proc_dir_entry *(shadow_proc_t )(struct super_block *sb, +struct proc_dir_entry *pde);struct proc_dir_entry {unsigned int low_ino;
@@ -79,6 +82,7 @@ struct proc_dir_entry {int pde_users;/* number of callers into module in progress */ spinlock_t pde_unload_lock;/* proc_fops checks and pde_users bumps */ struct completion *pde_unload_completion; +shadow_proc_t *shadow_proc; };struct kcore_list {
Signature · 1 line
1.5.3.rc6.17.g1911
Quoted 3 lines · Eric W. Biederman
Below is a preliminary patch. It solves the directory issue but it doesn't play well with
proc_mntandproc_flush_task. It works by simply caching the network namespace when we mount proc so we don't have to be fancy and dynamic.
Nice... Where should we apply this patch to?
Quoted 372 lines
Something for the discussion anyway.
I will start sorting out what makes sense tomorrow.
Eric
Quoted 1 line
From
f359fde2469ba8be2123a465e788a83c7e6831e9Mon Sep 17 00:00:00 2001From: Eric W. Biederman <[email protected]>
Date: Tue, 20 Nov 2007 19:36:05 -0700
Subject: [PATCH] proc: Fix/proc/netdirectory listings.Having proc dynamically display the contents of
/proc/netis hard. So make life simpler by capturing the network namespace when we mount proc and only displaying that network namespace.--- fs/proc/base.c | 8 ++-- fs/proc/generic.c | 4 ++- fs/proc/internal.h | 13 +++++++ fs/proc/proc_net.c | 89 ++++------------------------------------------- fs/proc/root.c | 50 ++++++++++++++++++-------- include/linux/proc_fs.h | 4 ++ 6 files changed, 66 insertions(+), 102 deletions(-)fs/proc/base.c +4
-4@@ -2395,7 +2395,7 @@ struct dentry *proc_pid_lookup(struct inode *dir, struct dentry * dentry, structif (tgid == ~0U ) goto out; - ns = dentry->d_sb->s_fs_info; + ns =proc_sbi (dentry->d_sb)->pid_ns;rcu_read_lock (); task =find_task_by_pid_ns (tgid, ns);if (task)@@ -2476,7 +2476,7 @@ int proc_pid_readdir(struct file * filp, void * dirent, filldir_t filldir) goto out; } - ns = filp->f_dentry->d_sb->s_fs_info; + ns =proc_sbi (filp->f_dentry->d_sb)->pid_ns; tgid = filp->f_pos -TGID_OFFSET ; for (task = next_tgid(tgid, ns); task;@@ -2615,7 +2615,7 @@ static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentryif (tid == ~0U ) goto out; - ns = dentry->d_sb->s_fs_info; + ns =proc_sbi (dentry->d_sb)->pid_ns;rcu_read_lock (); task =find_task_by_pid_ns (tid, ns);if (task)@@ -2758,7 +2758,7 @@ static int proc_task_readdir(struct file * filp, void * dirent, filldir_t filldi/* f_version caches the tgid value that the last readdir call couldn't * return. lseek aka telldir automagically resets f_version to 0. */ - ns = filp->f_dentry->d_sb->s_fs_info; + ns =proc_sbi (filp->f_dentry->d_sb)->pid_ns; tid = (int )filp->f_version; filp->f_version =0 ; for (task = first_tid(leader, tid, pos - 2, ns);fs/proc/generic.c +3
-1@@ -398,7 +398,9 @@ struct dentry *proc_lookup(struct inode * dir, struct dentry *dentry, struct namcontinue ;if (!memcmp (dentry->d_name.name, de->name, de->namelen)) {unsigned int ino = de->low_ino; - + +if (de->shadow_proc) + de = de->shadow_proc (dentry->d_sb, de);de_get (de);spin_unlock (&proc_subdir_lock); error = -EINVAL ;fs/proc/internal.h +13
-0@@ -11,6 +11,18 @@#include <linux/proc_fs.h> +struct pid_namespace ; +struct net ; +struct proc_sb_info { +struct pid_namespace *pid_ns; +struct net *net_ns; +}; + +static inline struct proc_sb_info *proc_sbi (struct super_block *sb) +{ +return sb->s_fs_info; +} +#ifdef CONFIG_PROC_SYSCTL extern int proc_sys_init (void );#else @@ -78,3 +90,4 @@ static inline int proc_fd(struct inode *inode) {return PROC_I (inode)->fd; } +fs/proc/proc_net.c +7
-82@@ -50,89 +50,15 @@ 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, -struct proc_dir_entry *de) +static struct proc_dir_entry *proc_net_shadow (struct super_block *sb, +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) -return ERR_PTR (-ENOENT ); - -dput (nd->dentry); -/* My dentry count is 1 and that should be enough as the -* shadow dentry is thrown away immediately. -*/ - nd->dentry = shadow; -return NULL ; +struct proc_sb_info *sbi =proc_sbi (sb); +return sbi->net_ns->proc_net; } -static struct dentry *proc_net_lookup (struct inode *dir,struct dentry *dentry, -struct nameidata *nd) -{ -struct net *net = current->nsproxy->net_ns; -struct dentry *shadow; - - shadow =proc_net_shadow_dentry (nd->dentry, net->proc_net); -if (!shadow) -return ERR_PTR (-ENOENT ); - -dput (nd->dentry); - nd->dentry = shadow; - -return shadow->d_inode->i_op->lookup (shadow->d_inode, dentry, nd); -} - -static int proc_net_setattr (struct dentry *dentry,struct iattr *iattr) -{ -struct net *net = current->nsproxy->net_ns; -struct dentry *shadow; -int ret; - - shadow =proc_net_shadow_dentry (dentry->d_parent, net->proc_net); -if (!shadow) -return -ENOENT ; - ret = shadow->d_inode->i_op->setattr (shadow, iattr); -dput (shadow); -return ret; -} - -static const struct file_operations proc_net_dir_operations = { - .read = generic_read_dir, -}; - -static struct inode_operations proc_net_dir_inode_operations = { - .follow_link = proc_net_follow_link, - .lookup = proc_net_lookup, - .setattr = proc_net_setattr, -}; -static __net_init int proc_net_ns_init (struct net *net) {struct proc_dir_entry *root, *netd, *net_statd;@@ -185,9 +111,8 @@ static struct pernet_operations __net_initdata proc_net_ns_ops = {int __init proc_net_init (void ) { - proc_net_shadow =proc_mkdir ("net" ,NULL ); - proc_net_shadow->proc_iops = &proc_net_dir_inode_operations; - proc_net_shadow->proc_fops = &proc_net_dir_operations; + shadow_pde =proc_mkdir ("net" ,NULL ); + shadow_pde->shadow_proc = proc_net_shadow;return register_pernet_subsys (&proc_net_ns_ops); }fs/proc/root.c +35
-15@@ -19,6 +19,7 @@#include <linux/smp_lock.h> #include <linux/mount.h> #include <linux/pid_namespace.h> +#include <net/net_namespace.h> #include "internal.h" @@ -26,15 +27,23 @@ struct proc_dir_entry *proc_bus, *proc_root_fs, *proc_root_driver;static int proc_test_super (struct super_block *sb,void *data) { -return sb->s_fs_info == data; +struct proc_sb_info *sbi =proc_sbi (sb), *info = data; +return (sbi->pid_ns == info->pid_ns) && + (sbi->net_ns == info->net_ns); }static int proc_set_super (struct super_block *sb,void *data) { -struct pid_namespace *ns; - - ns = (struct pid_namespace *)data; - sb->s_fs_info =get_pid_ns (ns); + +struct proc_sb_info *new, *info = data; + + new =kzalloc (sizeof (*new),GFP_KERNEL ); +if (!new) +return -ENOMEM ; + *new = *info; +get_pid_ns (new->pid_ns); +get_net (new->net_ns); + sb->s_fs_info = new;return set_anon_super (sb,NULL ); }@@ -43,7 +52,7 @@ static int proc_get_sb(struct file_system_type *fs_type, {int err;struct super_block *sb; -struct pid_namespace *ns; +struct proc_sb_info info, *sbi;struct proc_inode *ei;if (proc_mnt) {@@ -57,12 +66,14 @@ static int proc_get_sb(struct file_system_type *fs_type, ei->pid =find_get_pid (1 ); } + info.pid_ns = current->nsproxy->pid_ns; + info.net_ns = current->nsproxy->net_ns;if (flags &MS_KERNMOUNT ) - ns = (struct pid_namespace *)data; + sbi = data;else - ns = current->nsproxy->pid_ns; + sbi = &info; - sb =sget (fs_type, proc_test_super, proc_set_super, ns); + sb =sget (fs_type, proc_test_super, proc_set_super, sbi);if (IS_ERR (sb))return PTR_ERR (sb);@@ -78,12 +89,13 @@ static int proc_get_sb(struct file_system_type *fs_type, ei =PROC_I (sb->s_root->d_inode);if (!ei->pid) {rcu_read_lock (); - ei->pid =get_pid (find_pid_ns (1 , ns)); + ei->pid =get_pid (find_pid_ns (1 , sbi->pid_ns));rcu_read_unlock (); } sb->s_flags |=MS_ACTIVE ; - ns->proc_mnt = mnt; +if (!sbi->pid_ns->proc_mnt) + sbi->pid_ns->proc_mnt = mnt; }return simple_set_mnt (mnt, sb);@@ -91,11 +103,13 @@ static int proc_get_sb(struct file_system_type *fs_type,static void proc_kill_sb (struct super_block *sb) { -struct pid_namespace *ns; +struct proc_sb_info *sbi; - ns = (struct pid_namespace *)sb->s_fs_info; + sbi =proc_sbi (sb);kill_anon_super (sb); -put_pid_ns (ns); +put_pid_ns (sbi->pid_ns); +put_net (sbi->net_ns); +kfree (sbi); }static struct file_system_type proc_fs_type = {@@ -106,13 +120,16 @@ static struct file_system_type proc_fs_type = {void __init proc_root_init (void ) { +struct proc_sb_info info;int err =proc_init_inodecache ();if (err)return ; err =register_filesystem (&proc_fs_type);if (err)return ; - proc_mnt =kern_mount_data (&proc_fs_type, &init_pid_ns); + info.pid_ns = &init_pid_ns; + info.net_ns = current->nsproxy->net_ns; + proc_mnt =kern_mount_data (&proc_fs_type, &info); err =PTR_ERR (proc_mnt);if (IS_ERR (proc_mnt)) {unregister_filesystem (&proc_fs_type);@@ -214,8 +231,11 @@ struct proc_dir_entry proc_root = {int pid_ns_prepare_proc (struct pid_namespace *ns) { +struct proc_sb_info info;struct vfsmount *mnt; + info.pid_ns = ns; + info.net_ns = current->nsproxy->net_ns; mnt =kern_mount_data (&proc_fs_type, ns);if (IS_ERR (mnt))return PTR_ERR (mnt);include/linux/proc_fs.h +4
-0@@ -48,6 +48,9 @@ typedef int (read_proc_t)(char *page, char **start, off_t off,typedef int (write_proc_t )(struct file *file,const char __user *buffer,unsigned long count,void *data);typedef int (get_info_t )(char *,char **,off_t ,int ); +struct proc_dir_entry ; +typedef struct proc_dir_entry *(shadow_proc_t )(struct super_block *sb, +struct proc_dir_entry *pde);struct proc_dir_entry {unsigned int low_ino;@@ -79,6 +82,7 @@ struct proc_dir_entry {int pde_users;/* number of callers into module in progress */ spinlock_t pde_unload_lock;/* proc_fops checks and pde_users bumps */ struct completion *pde_unload_completion; +shadow_proc_t *shadow_proc; };struct kcore_list {
PATCH 1/1 proc_net: Remove userspace visible changes.
In reply to Pavel EmelyanovOk. I have kicked around a lot implementation ideas and took a good hard look at my /proc/net implementation. The patch below should close all of the holes with /proc/net that I am aware of.
Bind mounts work and properly capture /proc/net/
stat of /proc/net and /proc/net/ return the same information. cd /proc/net/ ; ls .. works
The dentry has the proper parent and no longer appears deleted.
As well as few more theoretical cases I have been able to imagine, like open("/proc/net", O_NOFOLLOW | O_DIRECTORY) getdents...
Please take a look and kick this patch around. I don't expect anyone to find any issues but a few more eyeballs before I send this along to Linus would be appreciated. Thanks.
From: Eric W. Biederman <[email protected]>
Subject: [PATCH] proc_net: Remove userspace visible changes.
This patch fixes some bugs in corner cases of the /proc/net implementation.
In proc_net_shadow_dentry.
- Set the parent dentry properly.
- Make the dentry appear hashed so .. works.
Remove the unreachable proc_net_lookup.
Implement proc_net_getattr to complete the
set of implemented inode operations.
Implement proc_net_open which changes the directory we are openting to remove the need to implement any other file operations.
Add a big fat comment on how /proc/net works to make it easier for someone else to look at and understand this code.
This patch should remove the last of the accidental user visible artifacts that arose from adding network namespace support to /proc/net.
--- fs/proc/proc_net.c | 116 +++++++++++++++++++++++++++++++++++++++++---------- 1 files changed, 93 insertions(+), 23 deletions(-)
fs/proc/proc_net.c +93 -23
@@ -50,24 +50,69 @@ struct net *get_proc_net(const struct inode *inode) }EXPORT_SYMBOL_GPL (get_proc_net); +/* +* The contents of the files under /proc/net depend on which network +* namespace you are in. +* +* This implementation relies on the following properties. +* +* - Each network namespaces has it's own /proc/net dcache tree. +* - A directory with a follow_link method never calls lookup +* - It is possible in ->open to competely change which underlying +* filesystem, path, and inode the struct file refers to. +* - A dcache entry with DCACHE_UNHASHED clear and pprev set +* appares hashed (and thus valid) to the dcache. +* +* To give each network namespace it's own /proc/net directory +* in a manner transparent to user space (and not requiring /proc) +* be remounted we do the following things: +* +* Keep a different dentry tree for each network namespace under +* /proc/net. +* +* Have the root of the /proc/net dentry tree be a ``unhashed'' +* dentry with it's root pointing at the /proc dentry. Making +* it appear in parallel with the normal /proc/net. +* +* Redirect all opens of the normal /proc/net to the one appropriate +* for the opening process in ->open. +* +* Redirect all directory traversals onto the appropriate /proc/net +* with a follow_link method. +* +* Wrap all other applicable inode operations so they appear to +* happen not on the normal /proc/net but on the network namespace +* specific one. +* +* Currently we can use a bind mount inside a network namespace +* to /proc/net visible to processes outside that network namespace. +* Long term /proc/net should migrate to /proc/<pid>/net removing +* the need for the bind mount for monitoring processes. +*/ +static struct proc_dir_entry *proc_net_shadow; -static struct dentry *proc_net_shadow_dentry (struct dentry *parent, -struct proc_dir_entry *de) +static struct dentry *proc_net_shadow_dentry (struct net *net, +struct dentry *dentry) { +struct proc_dir_entry *de = net->proc_net;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); + inode =proc_get_inode (dentry->d_sb, de->low_ino, de);if (!inode) goto out_de_put; - shadow =d_alloc_name (parent, de->name); + shadow =d_alloc (dentry->d_parent, &dentry->d_name);if (!shadow) goto out_iput; - shadow->d_op = parent->d_op;/* proc_dentry_operations */ + shadow->d_op = dentry->d_op;/* proc_dentry_operations */ d_instantiate (shadow, inode); + +/* Make the dentry looked hashed */ + shadow->d_hash.pprev = &shadow->d_hash.next; + shadow->d_flags &= ~DCACHE_UNHASHED ; out:return shadow; out_iput:
@@ -77,36 +122,36 @@ out_de_put: goto out; } -static void *proc_net_follow_link (struct dentry *parent,struct nameidata *nd) +static void *proc_net_follow_link (struct dentry *dentry,struct nameidata *nd) {struct net *net = current->nsproxy->net_ns;struct dentry *shadow; - shadow =proc_net_shadow_dentry (parent, net->proc_net); + + shadow =proc_net_shadow_dentry (net, dentry);if (!shadow) -return ERR_PTR (-ENOENT ); + goto out_err;dput (nd->dentry); -/* My dentry count is 1 and that should be enough as the -* shadow dentry is thrown away immediately. -*/ nd->dentry = shadow; +return NULL ; +out_err: +return ERR_PTR (-ENOENT ); } -static struct dentry *proc_net_lookup (struct inode *dir,struct dentry *dentry, -struct nameidata *nd) +static int proc_net_getattr (struct vfsmount *mnt,struct dentry *dentry, +struct kstat *stat) {struct net *net = current->nsproxy->net_ns;struct dentry *shadow; +int ret; - shadow =proc_net_shadow_dentry (nd->dentry, net->proc_net); + shadow =proc_net_shadow_dentry (net, dentry);if (!shadow) -return ERR_PTR (-ENOENT ); - -dput (nd->dentry); - nd->dentry = shadow; - -return shadow->d_inode->i_op->lookup (shadow->d_inode, dentry, nd); +return -ENOENT ; + ret = shadow->d_inode->i_op->getattr (mnt, shadow, stat); +dput (shadow); +return ret; }static int proc_net_setattr (struct dentry *dentry,struct iattr *iattr)
@@ -115,7 +160,7 @@ static int proc_net_setattr(struct dentry *dentry, struct iattr *iattr)struct dentry *shadow;int ret; - shadow =proc_net_shadow_dentry (dentry->d_parent, net->proc_net); + shadow =proc_net_shadow_dentry (net, dentry);if (!shadow)return -ENOENT ; ret = shadow->d_inode->i_op->setattr (shadow, iattr);
@@ -123,13 +168,38 @@ static int proc_net_setattr(struct dentry *dentry, struct iattr *iattr)return ret; } +static int proc_net_open (struct inode *inode,struct file *filp) +{ +struct net *net = current->nsproxy->net_ns; +struct dentry *shadow; +int ret; + + shadow =proc_net_shadow_dentry (net, filp->f_dentry); +if (!shadow) +return -ENOENT ; + + inode = shadow->d_inode; + +fops_put (filp->f_op); +dput (filp->f_dentry); + + filp->f_mapping = inode->i_mapping; + filp->f_op =fops_get (inode->i_fop); + filp->f_dentry = shadow; + + ret =0 ; +if (filp->f_op && filp->f_op->open) + ret = filp->f_op->open (inode, filp); +return ret; +} +static const struct file_operations proc_net_dir_operations = { - .read = generic_read_dir, + .open = proc_net_open, };static struct inode_operations proc_net_dir_inode_operations = { .follow_link = proc_net_follow_link, - .lookup = proc_net_lookup, + .getattr = proc_net_getattr, .setattr = proc_net_setattr, };
Signature · 1 line
1.5.3.rc6.17.g1911
Re: [CFT][PATCH] proc_net: Remove userspace visible changes.
Forget this patch. It works but I have found something better. Full explanation in the morning.
Eric
PATCH 1/1 Fix /proc/net breakage
In reply to Pavel EmelyanovQuoted 36 lines · Pavel Emelyanov
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Pavel Machek
Hi!
I think that this worked before:
Quoted 8 lines
Quoted 6 lines
root@amd:/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. root@amd:/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 directoryand 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/netand dumps the/proc/self/fdfiles produces the following:
Quoted 14 lines
# 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.
--- fs/proc/generic.c | 12 ++++++- fs/proc/proc_net.c | 86 +++-------------------------------------------- include/linux/proc_fs.h | 3 ++ 3 files changed, 19 insertions(+), 82 deletions(-)
fs/proc/generic.c +11 -1
@@ -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 namif (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 ;
fs/proc/proc_net.c +5 -81
103 lines not on this page · raw message
include/linux/proc_fs.h +3 -0
@@ -48,6 +48,8 @@ typedef int (read_proc_t)(char *page, char **start, off_t off,typedef int (write_proc_t )(struct file *file,const char __user *buffer,unsigned long count,void *data);typedef int (get_info_t )(char *,char **,off_t ,int ); +typedef struct proc_dir_entry *(shadow_proc_t )(struct task_struct *task, +struct proc_dir_entry *pde);struct proc_dir_entry {unsigned int low_ino;
@@ -79,6 +81,7 @@ struct proc_dir_entry {int pde_users;/* number of callers into module in progress */ spinlock_t pde_unload_lock;/* proc_fops checks and pde_users bumps */ struct completion *pde_unload_completion; +shadow_proc_t *shadow_proc; };struct kcore_list {
Signature · 1 line
1.5.3.rc6.17.g1911
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
[snip]
Quoted 16 lines
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_entrydentries 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 fragilefollow_linkmethod.To accomplish that this patch:
- introduces ashadow_procmethod to allow different dentries to be returned fromproc_lookup.
- Removes the old/proc/netfollow_linkmagic
- Fixes a weakness in our not caching of proc generic dentries.
Quoted 5 lines
As
shadow_procuses 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 theshadow_procmethod.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
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 25 lines · Pavel Emelyanov
[snip]
Quoted 10 lines
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_entrydentries 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 fragilefollow_linkmethod.Quoted 8 lines
To accomplish that this patch:
- introduces ashadow_procmethod to allow different dentries to be returned fromproc_lookup.
- Removes the old/proc/netfollow_linkmagic
- Fixes a weakness in our not caching of proc generic dentries.As
shadow_procuses a task struct to decided which dentry to return we can go back later and fix the proc generic caching without modifying any codethat
Quoted 3 lines
uses the
shadow_procmethod.Signed-off-by Eric W. Biederman <[email protected]>
Quoted 7 lines
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
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
In reply to Eric W. BiedermanQuoted 19 lines · Eric W. Biederman
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_entrydentries 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 fragilefollow_linkmethod.To accomplish that this patch:
- introduces ashadow_procmethod to allow different dentries to be returned fromproc_lookup.
- Removes the old/proc/netfollow_linkmagic
- Fixes a weakness in our not caching of proc generic dentries.As
shadow_procuses 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 theshadow_procmethod.
Quoted 6 lines
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.
Signature · 1 line
dwmw2
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 27 lines · On Fri, 07 Dec 2007 04:51:37 +0000 David Woodhouse <[email protected]> wrote:
Quoted 19 lines · Eric W. Biederman
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_entrydentries 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 fragilefollow_linkmethod.To accomplish that this patch:
- introduces ashadow_procmethod to allow different dentries to be returned fromproc_lookup.
- Removes the old/proc/netfollow_linkmagic
- Fixes a weakness in our not caching of proc generic dentries.As
shadow_procuses 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 theshadow_procmethod.Quoted 6 lines
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(-)
Quoted 5 lines
(commit
2b1e300a9dfc3196ccddf6f1d74b91b7af55e416)This seems to have broken the use of
/proc/bus/usbas 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?
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 28 lines · Andrew Morton
Quoted 26 lines · On Fri, 07 Dec 2007 04:51:37 +0000 David Woodhouse <[email protected]> wrote:
Quoted 25 lines · Eric W. Biederman
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_entrydentries 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 fragilefollow_linkmethod.To accomplish that this patch:
- introduces ashadow_procmethod to allow different dentries to be returned fromproc_lookup.
- Removes the old/proc/netfollow_linkmagic
- Fixes a weakness in our not caching of proc generic dentries.As
shadow_procuses 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 theshadow_procmethod.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(-)
Quoted 9 lines
Quoted 5 lines
(commit
2b1e300a9dfc3196ccddf6f1d74b91b7af55e416)This seems to have broken the use of
/proc/bus/usbas 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
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
In reply to Andrew MortonHi,
Quoted 8 lines · Andrew Morton
Quoted 5 lines
(commit
2b1e300a9dfc3196ccddf6f1d74b91b7af55e416)This seems to have broken the use of
/proc/bus/usbas 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?
"Soonish" being rather earlier than 20071227?
'cause it's still throwing a fit for me on 2.6.24-rc6-mm1(!) (plus hotfix), nothing visible in /proc/bus/usb, thus WLAN driver won't probe anything.
Side note: later upgraded from hotplug-based setup to udev,
but didn't change status quo of problem of empty mount.
Now back again to a 2.6.19ish kernel, since this is the only one that I could always fall back on after a sizeable number of attempts to get to run anything newer trouble-free in this environment. Frankly I'm slightly getting fed up with the number of regressions I'm hitting in my smallish environment lately. But that's sorta what you pay for when going experimental.
Just the same day I also had to discover that e100 doesn't usefully support (IOW, fatal resource probing error) my Intel BNC combo 645477-004, as opposed to eepro100. Swapped the card from this ""production"" headless box to an easily accessible one for followup debugging of e100.
K6-3@150 headless, Debian stable.
mount 2.12r-19
usbutils 0.72-7
fstab:
none /proc/bus/usb usbfs defaults 0 0
lsmod (NOTE: from 2.6.19-cks2, sorry):
Module Size Used by act_police 6980 1 sch_ingress 3712 1 cls_u32 7300 5 sch_tbf 6400 1 sch_sfq 5888 4 sch_htb 16128 1 ppp_async 11648 1 crc_ccitt 2304 1 ppp_async ipt_ULOG 7940 1 xt_state 2304 4 xt_limit 2816 11 xt_tcpudp 3200 48 xt_multiport 3200 10 iptable_mangle 2944 0 iptable_nat 7044 1 iptable_filter 3200 1 ip_tables 12360 3 iptable_mangle,iptable_nat,iptable_filter ip_nat_tftp 2048 0 ip_conntrack_tftp 4504 1 ip_nat_tftp ip_nat_h323 7296 0 ip_conntrack_h323 48028 1 ip_nat_h323 ip_nat_irc 2816 0 ip_nat_ftp 3456 0 ip_conntrack_irc 6928 1 ip_nat_irc ip_conntrack_ftp 7312 1 ip_nat_ftp ipt_MASQUERADE 3840 1 ip_nat 16684 6 iptable_nat,ip_nat_tftp,ip_nat_h323,ip_nat_irc,ip_nat_ftp,ipt_MASQUERADE ip_conntrack 43884 12 xt_state,iptable_nat,ip_nat_tftp,ip_conntrack_tftp,ip_nat_h323,ip_conntrack_h323,ip_nat_irc,ip_nat_ftp,ip_conntrack_irc,ip_conntrack_ftp,ipt_MASQUERADE,ip_nat ipt_REJECT 4608 0 ipt_LOG 6784 11 x_tables 14468 10 ipt_ULOG,xt_state,xt_limit,xt_tcpudp,xt_multiport,iptable_nat,ip_tables,ipt_MASQUERADE,ipt_REJECT,ipt_LOG isdn 109920 0 sis5595 14472 0 i2c_isa 6528 1 sis5595 at76c503_rfmd 5900 0 firmware_class 9728 1 at76c503_rfmd i2c_sis5595 8196 0 at76c503 85856 1 at76c503_rfmd at76_usbdfu 5636 1 at76c503 i2c_sis630 9100 0 i2c_core 22656 4 sis5595,i2c_isa,i2c_sis5595,i2c_sis630 ohci_hcd 31748 0 usbcore 135044 5 at76c503_rfmd,at76c503,at76_usbdfu,ohci_hcd e100 33800 0 3c59x 41384 0
lspci -v (2.6.19-cks2):
00:00.0 Host bridge: Silicon Integrated Systems [SiS] 5591/5592 Host (rev 02)
Flags: bus master, medium devsel, latency 64
Memory at e8000000 (32-bit, non-prefetchable) [size=64M]
Capabilities: [c0] AGP version 1.0
00:00.1 IDE interface: Silicon Integrated Systems [SiS] 5513 [IDE] (rev d0) (prog-if 8a [Master SecP PriP])
Flags: bus master, fast devsel, latency 64, IRQ 14
I/O ports at <ignored>
I/O ports at <ignored>
I/O ports at <ignored>
I/O ports at <ignored>
I/O ports at 4000 [size=16]
00:01.0 ISA bridge: Silicon Integrated Systems [SiS] SiS85C503/5513 (LPC Bridge) (rev 01)
Flags: bus master, medium devsel, latency 0
00:01.1 Class ff00: Silicon Integrated Systems [SiS] ACPI
Flags: medium devsel
00:01.2 USB Controller: Silicon Integrated Systems [SiS] USB 1.0 Controller (rev 11) (prog-if 10 [OHCI])
Flags: bus master, medium devsel, latency 64, IRQ 12 Memory at f0102000 (32-bit, non-prefetchable) [size=4K]
00:02.0 PCI bridge: Silicon Integrated Systems [SiS] Virtual PCI-to-PCI bridge (AGP) (prog-if 00 [Normal decode])
Flags: bus master, fast devsel, latency 0
Bus: primary=00, secondary=01, subordinate=01, sec-latency=64
I/O behind bridge: 0000d000-0000dfff
Memory behind bridge: ec000000-edffffff
Prefetchable memory behind bridge: e0000000-e7ffffff
00:09.0 Ethernet controller: Intel Corporation 82557/8/9 [Ethernet Pro 100] (rev 08)
Subsystem: Intel Corporation EtherExpress PRO/100+ Management Adapter Flags: bus master, medium devsel, latency 64, IRQ 11 Memory at f0100000 (32-bit, non-prefetchable) [size=4K] I/O ports at e000 [size=64] Memory at f0000000 (32-bit, non-prefetchable) [size=1M] Expansion ROM at ee000000 [disabled] [size=1M] Capabilities: [dc] Power Management version 2
00:0f.0 Ethernet controller: 3Com Corporation 3c905B Deluxe Etherlink 10/100/BNC [Cyclone]
Subsystem: 3Com Corporation 3c905B Deluxe Etherlink 10/100/BNC [Cyclone] Flags: bus master, medium devsel, latency 64, IRQ 10 I/O ports at e400 [size=128] Memory at f0101000 (32-bit, non-prefetchable) [size=128] Expansion ROM at ef000000 [disabled] [size=128K] Capabilities: [dc] Power Management version 1
01:00.0 VGA compatible controller: ATI Technologies Inc Radeon RV100 QY [Radeon 7000/VE] (prog-if 00 [VGA])
Subsystem: ATI Technologies Inc Unknown device 110a Flags: bus master, stepping, 66MHz, medium devsel, latency 64 Memory at e0000000 (32-bit, prefetchable) [size=128M] I/O ports at d000 [size=256] Memory at ed000000 (32-bit, non-prefetchable) [size=64K] [virtual] Expansion ROM at ec000000 [disabled] [size=128K] Capabilities: [58] AGP version 2.0 Capabilities: [50] Power Management version 2
Thanks,
Andreas Mohr
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 7 lines · Andreas Mohr
Quoted 6 lines · Andrew Morton
Quoted 5 lines
(commit
2b1e300a9dfc3196ccddf6f1d74b91b7af55e416)This seems to have broken the use of
/proc/bus/usbas a mountpoint. It always appears empty now, whatever's supposed to be mounted there.
Quoted 7 lines
Quoted 2 lines
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?
"Soonish" being rather earlier than 20071227?
'cause it's still throwing a fit for me on 2.6.24-rc6-mm1(!) (plus hotfix), nothing visible in/proc/bus/usb, thus WLAN driver won't probe anything.
Patch which restores usual behaviour was merged in 2.6.24-rc5 (3790ee4bd86396558eedd86faac1052cb782e4e1 "proc: remove/Fix proc generic d_revalidate") so no hotfixes are needed. I just checked with bind mounting / to /proc/bus/usb -- it works.
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Hi,
Quoted 11 lines · Alexey Dobriyan
Quoted 10 lines · Andreas Mohr
Quoted 8 lines · Andrew Morton
Quoted 5 lines
(commit
2b1e300a9dfc3196ccddf6f1d74b91b7af55e416)This seems to have broken the use of
/proc/bus/usbas 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?
Quoted 7 lines
Quoted 4 lines
"Soonish" being rather earlier than 20071227?
'cause it's still throwing a fit for me on 2.6.24-rc6-mm1(!) (plus hotfix), nothing visible in/proc/bus/usb, thus WLAN driver won't probe anything.Patch which restores usual behaviour was merged in 2.6.24-rc5 (
3790ee4bd86396558eedd86faac1052cb782e4e1"proc: remove/Fix proc genericd_revalidate")
Via a kerneltrap.org article (problematic internet setup here currently, non-C&P text terminal only) I could see that this is the patch which removes the d_revalidate member and the corresponding proc_revalidate_dentry(). And this is the state that my 2.6.24-rc_six_-mm1 tree is in already. So either it didn't help here or it broke again by some later change or there's some dumb PEBKAC error here.
Quoted 2 lines
so no hotfixes are needed. I just checked with bind mounting / to
/proc/bus/usb-- it works.
OK, I'll try to re-check manual, raw, bare-metal mounting (bind etc.) soonish.
"CONFIG_NETNS" as mentioned in the patch description actually seems to be "CONFIG_NET_NS", BTW.
(which I DON'T have set at the moment, if this happens to make a difference)
Thanks,
Andreas Mohr
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 19 lines · Andreas Mohr
Hi,
Quoted 16 lines · Alexey Dobriyan
Quoted 14 lines · Andreas Mohr
Quoted 8 lines · Andrew Morton
Quoted 5 lines
(commit
2b1e300a9dfc3196ccddf6f1d74b91b7af55e416)This seems to have broken the use of
/proc/bus/usbas 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?
"Soonish" being rather earlier than 20071227?
'cause it's still throwing a fit for me on 2.6.24-rc6-mm1(!) (plus hotfix), nothing visible in/proc/bus/usb, thus WLAN driver won't probe anything.
Quoted 7 lines
Quoted 2 lines
Patch which restores usual behaviour was merged in 2.6.24-rc5 (
3790ee4bd86396558eedd86faac1052cb782e4e1"proc: remove/Fix proc genericd_revalidate")Via a kerneltrap.org article (problematic internet setup here currently, non-C&P text terminal only) I could see that this is the patch which removes the
d_revalidatemember and the correspondingproc_revalidate_dentry(). And this is the state that my 2.6.24-rc_six_-mm1 tree is in already.
OK.
Quoted 2 lines
So either it didn't help here or it broke again by some later change or there's some dumb PEBKAC error here.
Do you by chance forgot CONFIG_USB_?HCI_HCD=y ? I see empty usbfs here if they are deselected.
Quoted 8 lines
Quoted 2 lines
so no hotfixes are needed. I just checked with bind mounting / to
/proc/bus/usb-- it works.OK, I'll try to re-check manual, raw, bare-metal mounting (bind etc.) soonish.
"
CONFIG_NETNS" as mentioned in the patch description actually seems to be "CONFIG_NET_NS", BTW.
(which I DON'T have set at the moment, if this happens to make a difference)
I shouldn't.
Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Hi,
Quoted 10 lines · Alexey Dobriyan
Quoted 1 line · Andreas Mohr
And this is the state that my 2.6.24-rc_six_-mm1 tree is in already.
OK.
Quoted 2 lines
So either it didn't help here or it broke again by some later change or there's some dumb PEBKAC error here.
Do you by chance forgot CONFIG_USB_?HCI_HCD=y ? I see empty usbfs here if they are deselected.
Quite well-educated guess! In fact I had already discovered that ohci-hcd fails to load, at all. Doh.
(however after going through all recent ohci.*hcd related LKML postings it seems there's no report about such problems yet)
Sorry for barking up the entirely wrong tree!
For giggles, here's the output:
# modprobe ohci-hcd FATAL: Error inserting ohci_hcd (/lib/modules/2.6.24-rc6-mm1-gate/kernel/drivers/usb/host/ohci-hcd.ko): No such device
dmesg:ohci_hcd: 2006 August 04 USB 1.1 'Open' Host Controller (OHCI) Driverohci_hcd: block sizes: ed 64 td 64
(yes, that's all there is, despite CONFIG_USB_DEBUG being set)
The LED of a usb stick isn't active either, for obvious reasons.
And keep in mind that this is a (relatively old) OHCI-only machine... (which had the 2.6.19 lsmod showing ohci-hcd just fine and working fine with WLAN USB)
Now pondering whether to try -rc6 proper or whether to revert specific guilty-looking USB changes...
And wondering how to properly elevate this issue (prompt Greg about it, new thread, bug #, ...?)
Thanks,
Andreas Mohr
[usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 11 lines · Andreas Mohr
(yes, that's all there is, despite
CONFIG_USB_DEBUGbeing set)The LED of a usb stick isn't active either, for obvious reasons.
And keep in mind that this is a (relatively old) OHCI-only machine... (which had the 2.6.19 lsmod showing ohci-hcd just fine and working fine with WLAN USB)
Now pondering whether to try -rc6 proper or whether to revert specific guilty-looking USB changes... And wondering how to properly elevate this issue (prompt Greg about it, new thread, bug #, ...?)
Ingo
Alan Stern
maintainerRe: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 6 lines · Ingo Molnar
Quoted 4 lines · Andreas Mohr
(yes, that's all there is, despite
CONFIG_USB_DEBUGbeing set)The LED of a usb stick isn't active either, for obvious reasons.
Quoted 7 lines
Quoted 7 lines
And keep in mind that this is a (relatively old) OHCI-only machine... (which had the 2.6.19 lsmod showing ohci-hcd just fine and working fine with WLAN USB)
Now pondering whether to try -rc6 proper or whether to revert specific guilty-looking USB changes... And wondering how to properly elevate this issue (prompt Greg about it, new thread, bug #, ...?)
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.
Alan Stern
drivers/usb/host/ohci-hcd.c +2 -9
@@ -1067,14 +1067,8 @@ static int __init ohci_hcd_mod_init(void#ifdef DEBUG ohci_debug_root =debugfs_create_dir ("ohci" ,NULL ); -if (!ohci_debug_root ||IS_ERR (ohci_debug_root)) { -if (!ohci_debug_root) - retval = -ENOENT ; -else - retval =PTR_ERR (ohci_debug_root); - - goto error_debug; - } +if (!ohci_debug_root) +return -ENOENT ;#endif #ifdef PS3_SYSTEM_BUS_DRIVER
@@ -1142,7 +1136,6 @@ static int __init ohci_hcd_mod_init(void#ifdef DEBUG debugfs_remove (ohci_debug_root); ohci_debug_root =NULL ; - error_debug:#endif return retval;
drivers/usb/host/ohci-dbg.c +7 -12
@@ -813,30 +813,29 @@ static inline void create_debug_files (sstruct device *dev = bus->dev; ohci->debug_dir =debugfs_create_dir (bus->bus_name, ohci_debug_root); -if (!ohci->debug_dir ||IS_ERR (ohci->debug_dir)) { - ohci->debug_dir =NULL ; - goto done; - } +if (!ohci->debug_dir) +return ; ohci->debug_async =debugfs_create_file ("async" ,S_IRUGO , ohci->debug_dir, dev, &debug_async_fops); -if (!ohci->debug_async ||IS_ERR (ohci->debug_async)) +if (!ohci->debug_async) goto async_error; ohci->debug_periodic =debugfs_create_file ("periodic" ,S_IRUGO , ohci->debug_dir, dev, &debug_periodic_fops); -if (!ohci->debug_periodic ||IS_ERR (ohci->debug_periodic)) +if (!ohci->debug_periodic) goto periodic_error; ohci->debug_registers =debugfs_create_file ("registers" ,S_IRUGO , ohci->debug_dir, dev, &debug_registers_fops); -if (!ohci->debug_registers ||IS_ERR (ohci->debug_registers)) +if (!ohci->debug_registers) goto registers_error; - goto done; +ohci_dbg (ohci,"created debug files\n" ); +return ; registers_error:debugfs_remove (ohci->debug_periodic);
@@ -847,10 +846,6 @@ periodic_error: async_error:debugfs_remove (ohci->debug_dir); ohci->debug_dir =NULL ; -done: -return ; - -ohci_dbg (ohci,"created debug files\n" ); }static inline void remove_debug_files (struct ohci_hcd *ohci)
drivers/usb/host/ehci-hcd.c +2 -8
@@ -1019,14 +1019,8 @@ static int __init ehci_hcd_init(void)#ifdef DEBUG ehci_debug_root =debugfs_create_dir ("ehci" ,NULL ); -if (!ehci_debug_root ||IS_ERR (ehci_debug_root)) { -if (!ehci_debug_root) - retval = -ENOENT ; -else - retval =PTR_ERR (ehci_debug_root); - -return retval; - } +if (!ehci_debug_root) +return -ENOENT ;#endif #ifdef PLATFORM_DRIVER
drivers/usb/host/ehci-dbg.c +6 -8
@@ -914,30 +914,28 @@ static inline void create_debug_files (sstruct usb_bus *bus = &ehci_to_hcd (ehci)->self; ehci->debug_dir =debugfs_create_dir (bus->bus_name, ehci_debug_root); -if (!ehci->debug_dir ||IS_ERR (ehci->debug_dir)) { - ehci->debug_dir =NULL ; - goto done; - } +if (!ehci->debug_dir) +return ; ehci->debug_async =debugfs_create_file ("async" ,S_IRUGO , ehci->debug_dir, bus, &debug_async_fops); -if (!ehci->debug_async ||IS_ERR (ehci->debug_async)) +if (!ehci->debug_async) goto async_error; ehci->debug_periodic =debugfs_create_file ("periodic" ,S_IRUGO , ehci->debug_dir, bus, &debug_periodic_fops); -if (!ehci->debug_periodic ||IS_ERR (ehci->debug_periodic)) +if (!ehci->debug_periodic) goto periodic_error; ehci->debug_registers =debugfs_create_file ("registers" ,S_IRUGO , ehci->debug_dir, bus, &debug_registers_fops); -if (!ehci->debug_registers ||IS_ERR (ehci->debug_registers)) +if (!ehci->debug_registers) goto registers_error; - goto done; +return ; registers_error:debugfs_remove (ehci->debug_periodic);
@@ -948,9 +946,6 @@ periodic_error: async_error:debugfs_remove (ehci->debug_dir); ehci->debug_dir =NULL ;
Re: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 12 lines · Alan Stern
Quoted 10 lines · Ingo Molnar
Quoted 8 lines · Andreas Mohr
(yes, that's all there is, despite
CONFIG_USB_DEBUGbeing set)The LED of a usb stick isn't active either, for obvious reasons.
And keep in mind that this is a (relatively old) OHCI-only machine... (which had the 2.6.19 lsmod showing ohci-hcd just fine and working fine with WLAN USB)
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Now pondering whether to try -rc6 proper or whether to revert specific guilty-looking USB changes... And wondering how to properly elevate this issue (prompt Greg about it, new thread, bug #, ...?)
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 :(
Quoted 2 lines
Let me know if this patch fixes the problem. If it does, I'll submit it to Greg with all the proper accoutrements.
This isn't going to work if CONFIG_DEBUGFS is not enabled either :(
Quoted 8 lines
Index:
2.6.24-rc6-mm1/drivers/usb/host/ohci-hcd.c
===================================================================drivers/usb/host/ohci-hcd.c +0
-0@@ -1067,14 +1067,8 @@ static int __init ohci_hcd_mod_init(void#ifdef DEBUG ohci_debug_root =debugfs_create_dir ("ohci" ,NULL );
drivers/usb/host/ohci-hcd.c static int __init ohci_hcd_mod_init(void · line 1071
Quoted 10 lines
drivers/usb/host/ohci-hcd.c +2
-8-if (!ohci_debug_root ||IS_ERR (ohci_debug_root)) { -if (!ohci_debug_root) - retval = -ENOENT ; -else - retval =PTR_ERR (ohci_debug_root); - - goto error_debug; - } +if (!ohci_debug_root) +return -ENOENT ;
It needs to check for ERR_PTR(-ENODEV) which is the return value if debugfs is not enabled, and if so, just ignore things.
If NULL is returned, or anything else, it's a real error.
So, try something like:
if (!ohci_debug_root) {
retval = -ENOENT;
goto error_debug;
}
if (IS_ERR(ohci_debug_root) && PTR_ERR(ohci_debug_root) != -ENODEV) {
retval = PTR_ERR(ohci_debug_root);
goto error_debug;
}and let me know of that works for you.
Although the combination of CONFIG_USB_DEBUG and not CONFIG_DEBUGFS is strange, we could just enable CONFIG_DEBUGFS is USB_DEBUG is enabled and then simplify this logic a bunch at the same time.
thanks,
greg k-h
Alan Stern
maintainerRe: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 9 lines · Greg KH
Quoted 2 lines
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 :(
Quoted 2 lines
Let me know if this patch fixes the problem. If it does, I'll submit it to Greg with all the proper accoutrements.
This isn't going to work if
CONFIG_DEBUGFSis not enabled either :(
Why not? The debugging files won't be created, true, but the driver should work just fine regardless. This is exactly the way uhci-hcd behaves, and it hasn't caused any problems.
Quoted 6 lines
Quoted 6 lines
Index:
2.6.24-rc6-mm1/drivers/usb/host/ohci-hcd.c
===================================================================drivers/usb/host/ohci-hcd.c +0
-0@@ -1067,14 +1067,8 @@ static int __init ohci_hcd_mod_init(void
Quoted 15 lines
drivers/usb/host/ohci-hcd.c static int __init ohci_hcd_mod_init(void · line 1071
Quoted 12 lines
drivers/usb/host/ohci-hcd.c +2
-8#ifdef DEBUG ohci_debug_root =debugfs_create_dir ("ohci" ,NULL ); -if (!ohci_debug_root ||IS_ERR (ohci_debug_root)) { -if (!ohci_debug_root) - retval = -ENOENT ; -else - retval =PTR_ERR (ohci_debug_root); - - goto error_debug; - } +if (!ohci_debug_root) +return -ENOENT ;It needs to check for
ERR_PTR(-ENODEV)which is the return value if debugfs is not enabled, and if so, just ignore things.
No. That is the mistake you made before. If debugfs isn't enabled then the driver should just ignore things, yes -- and in particular it should ignore the fact that the return code is ERR_PTR(-ENODEV). No extra checking is required.
Quoted 1 line
If NULL is returned, or anything else, it's a real error.
If NULL is returned, it's a real error, agreed. But if anything else is returned then the call was successful as far as the driver is concerned.
Quoted 11 lines
So, try something like:
if (!ohci_debug_root) { retval = -ENOENT; goto error_debug; } if (IS_ERR(ohci_debug_root) && PTR_ERR(ohci_debug_root) != -ENODEV) { retval = PTR_ERR(ohci_debug_root); goto error_debug; }and let me know of that works for you.
Greg, it sounds like you have forgotten the rationale you originally used for specifying the return codes in debugfs! The idea was to return non-NULL if CONFIG_DEBUGFS was disabled, so that drivers could pretend the calls had succeeded and not need to worry about matters beyond their control.
Only a NULL return indicates a genuine error. The kerneldoc for debugfs_create_dir says this very plainly.
Quoted 3 lines
Although the combination of
CONFIG_USB_DEBUGand notCONFIG_DEBUGFSis strange, we could just enableCONFIG_DEBUGFSisUSB_DEBUGis enabled and then simplify this logic a bunch at the same time.
Most distributions enable CONFIG_DEBUGFS by default, don't they? So this problem only comes up when people make up their own configs. Having USB_DEBUG enabled and DEBUGFS disabled seems like a perfectly reasonable combination to me. I wouldn't change any aspect of the configuration logic.
Alan Stern
Re: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 7 lines · Alan Stern
Quoted 5 lines · Greg KH
Quoted 2 lines
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 :(
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Let me know if this patch fixes the problem. If it does, I'll submit it to Greg with all the proper accoutrements.
This isn't going to work if
CONFIG_DEBUGFSis not enabled either :(Why not? The debugging files won't be created, true, but the driver should work just fine regardless. This is exactly the way uhci-hcd behaves, and it hasn't caused any problems.
Ok, you are right, it was late and after some wine and I shouldn't have been responding to email :)
I'll rework this, and send out a patch tonight or tomorrow when I get a chance (am on the road right now). We should only care about NULL being returned here, that's the only "real" error.
And in thinking about it some more, I should go audit all the debugfs callers to make sure they are doing something sane too, it shouldn't be this complex at all...
thanks,
greg k-h
Re: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 7 lines · Greg KH
Quoted 6 lines · Alan Stern
Quoted 4 lines · Greg KH
Quoted 2 lines
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
Re: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Hi,
Quoted 9 lines · Greg KH
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_DEBUGFSis not enabled.
Quoted 4 lines
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?
Quoted 2 lines
Hm, I wonder if this means I can go back to drinking more holiday wine... :)
.....even more confusion? :)
Thanks for your help,
Andreas Mohr
Re: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
Quoted 15 lines · Andreas Mohr
Hi,
Quoted 12 lines · Greg KH
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_DEBUGFSis not enabled.So, for the 2.6.24 release, nothing needs to be changed, all is good, and there is no regression.
Quoted 8 lines
Quoted 1 line
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 linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
maintainerRe: [usb regression] Re: [PATCH 2.6.24-rc3] Fix /proc/net breakage
In reply to Greg KHQuoted 7 lines · Greg KH
Quoted 5 lines · Greg KH
Quoted 4 lines · Alan Stern
Quoted 2 lines · Greg KH
Quoted 2 lines
It looks like Greg misused the debugfs API -- which is ironic, because he wrote debugfs in the first place! :-)
Quoted 8 lines
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_DEBUGFSis 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?
Quoted 4 lines
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.
Quoted 4 lines
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 linux-usb" 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
Quoted 2 lines · Alan Stern
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 linux-usb" 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
In reply to Alan SternHi,
Quoted 5 lines · Alan Stern
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