* Re: + procfs-directory-entry-cleanup.patch added to -mm tree
[not found] <200705210341.l4L3f1BH001692@shell0.pdx.osdl.net>
@ 2007-05-22 17:43 ` Alexey Dobriyan
2007-05-22 18:29 ` Andrew Morton
0 siblings, 1 reply; 3+ messages in thread
From: Alexey Dobriyan @ 2007-05-22 17:43 UTC (permalink / raw)
To: linux-kernel; +Cc: akpm, xiaosuo
On Sun, May 20, 2007 at 08:41:02PM -0700, akpm@linux-foundation.org wrote:
> Subject: procfs directory entry cleanup
> From: "Changli Gao" <xiaosuo@gmail.com>
>
> Function proc_register() will assign proc_dir_operations and
> proc_dir_inode_operations to ent's members proc_fops and proc_iops
> correctly if ent is a directory. So the early assignment isn't
> necessary.
> --- a/fs/proc/generic.c~procfs-directory-entry-cleanup
> +++ a/fs/proc/generic.c
> @@ -649,9 +649,6 @@ struct proc_dir_entry *proc_mkdir_mode(c
>
> ent = proc_create(&parent, name, S_IFDIR | mode, 2);
> if (ent) {
> - ent->proc_fops = &proc_dir_operations;
> - ent->proc_iops = &proc_dir_inode_operations;
> -
> if (proc_register(parent, ent) < 0) {
> kfree(ent);
> ent = NULL;
> @@ -686,10 +683,6 @@ struct proc_dir_entry *create_proc_entry
>
> ent = proc_create(&parent,name,mode,nlink);
> if (ent) {
> - if (S_ISDIR(mode)) {
> - ent->proc_fops = &proc_dir_operations;
> - ent->proc_iops = &proc_dir_inode_operations;
> - }
> if (proc_register(parent, ent) < 0) {
> kfree(ent);
> ent = NULL;
This should add race because new PDE for directory will be glued to tree
with NULL ->proc_iops and, at least, vfs_getattr() doesn't check for
NULL ->i_op.
I've tried to reproduce this scenario with udelay() inserted before
S_ISDIR checks in proc_register() but got some panic while in X,
so no cookie for me. Maybe it's impossible, indeed.
Anyway, this patch + moving ->proc_fops and ->proc_iops initialization
before gluing is OK with me.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: + procfs-directory-entry-cleanup.patch added to -mm tree
2007-05-22 17:43 ` + procfs-directory-entry-cleanup.patch added to -mm tree Alexey Dobriyan
@ 2007-05-22 18:29 ` Andrew Morton
2007-05-22 20:16 ` Alexey Dobriyan
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Morton @ 2007-05-22 18:29 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: linux-kernel, xiaosuo
On Tue, 22 May 2007 21:43:49 +0400
Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Sun, May 20, 2007 at 08:41:02PM -0700, akpm@linux-foundation.org wrote:
> > Subject: procfs directory entry cleanup
> > From: "Changli Gao" <xiaosuo@gmail.com>
> >
> > Function proc_register() will assign proc_dir_operations and
> > proc_dir_inode_operations to ent's members proc_fops and proc_iops
> > correctly if ent is a directory. So the early assignment isn't
> > necessary.
>
> > --- a/fs/proc/generic.c~procfs-directory-entry-cleanup
> > +++ a/fs/proc/generic.c
> > @@ -649,9 +649,6 @@ struct proc_dir_entry *proc_mkdir_mode(c
> >
> > ent = proc_create(&parent, name, S_IFDIR | mode, 2);
> > if (ent) {
> > - ent->proc_fops = &proc_dir_operations;
> > - ent->proc_iops = &proc_dir_inode_operations;
> > -
> > if (proc_register(parent, ent) < 0) {
> > kfree(ent);
> > ent = NULL;
> > @@ -686,10 +683,6 @@ struct proc_dir_entry *create_proc_entry
> >
> > ent = proc_create(&parent,name,mode,nlink);
> > if (ent) {
> > - if (S_ISDIR(mode)) {
> > - ent->proc_fops = &proc_dir_operations;
> > - ent->proc_iops = &proc_dir_inode_operations;
> > - }
> > if (proc_register(parent, ent) < 0) {
> > kfree(ent);
> > ent = NULL;
>
> This should add race because new PDE for directory will be glued to tree
> with NULL ->proc_iops and, at least, vfs_getattr() doesn't check for
> NULL ->i_op.
>
> I've tried to reproduce this scenario with udelay() inserted before
> S_ISDIR checks in proc_register() but got some panic while in X,
> so no cookie for me. Maybe it's impossible, indeed.
>
> Anyway, this patch + moving ->proc_fops and ->proc_iops initialization
> before gluing is OK with me.
Like this?
diff -puN fs/proc/generic.c~procfs-directory-entry-cleanup-fix fs/proc/generic.c
--- a/fs/proc/generic.c~procfs-directory-entry-cleanup-fix
+++ a/fs/proc/generic.c
@@ -529,12 +529,6 @@ static int proc_register(struct proc_dir
return -EAGAIN;
dp->low_ino = i;
- spin_lock(&proc_subdir_lock);
- dp->next = dir->subdir;
- dp->parent = dir;
- dir->subdir = dp;
- spin_unlock(&proc_subdir_lock);
-
if (S_ISDIR(dp->mode)) {
if (dp->proc_iops == NULL) {
dp->proc_fops = &proc_dir_operations;
@@ -550,6 +544,13 @@ static int proc_register(struct proc_dir
if (dp->proc_iops == NULL)
dp->proc_iops = &proc_file_inode_operations;
}
+
+ spin_lock(&proc_subdir_lock);
+ dp->next = dir->subdir;
+ dp->parent = dir;
+ dir->subdir = dp;
+ spin_unlock(&proc_subdir_lock);
+
return 0;
}
_
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: + procfs-directory-entry-cleanup.patch added to -mm tree
2007-05-22 18:29 ` Andrew Morton
@ 2007-05-22 20:16 ` Alexey Dobriyan
0 siblings, 0 replies; 3+ messages in thread
From: Alexey Dobriyan @ 2007-05-22 20:16 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, xiaosuo
On Tue, May 22, 2007 at 11:29:51AM -0700, Andrew Morton wrote:
> On Tue, 22 May 2007 21:43:49 +0400
> Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> > On Sun, May 20, 2007 at 08:41:02PM -0700, akpm@linux-foundation.org wrote:
> > > Subject: procfs directory entry cleanup
> > > From: "Changli Gao" <xiaosuo@gmail.com>
> > >
> > > Function proc_register() will assign proc_dir_operations and
> > > proc_dir_inode_operations to ent's members proc_fops and proc_iops
> > > correctly if ent is a directory. So the early assignment isn't
> > > necessary.
> >
> > > --- a/fs/proc/generic.c~procfs-directory-entry-cleanup
> > > +++ a/fs/proc/generic.c
> > > @@ -649,9 +649,6 @@ struct proc_dir_entry *proc_mkdir_mode(c
> > >
> > > ent = proc_create(&parent, name, S_IFDIR | mode, 2);
> > > if (ent) {
> > > - ent->proc_fops = &proc_dir_operations;
> > > - ent->proc_iops = &proc_dir_inode_operations;
> > > -
> > > if (proc_register(parent, ent) < 0) {
> > > kfree(ent);
> > > ent = NULL;
> > > @@ -686,10 +683,6 @@ struct proc_dir_entry *create_proc_entry
> > >
> > > ent = proc_create(&parent,name,mode,nlink);
> > > if (ent) {
> > > - if (S_ISDIR(mode)) {
> > > - ent->proc_fops = &proc_dir_operations;
> > > - ent->proc_iops = &proc_dir_inode_operations;
> > > - }
> > > if (proc_register(parent, ent) < 0) {
> > > kfree(ent);
> > > ent = NULL;
> >
> > This should add race because new PDE for directory will be glued to tree
> > with NULL ->proc_iops and, at least, vfs_getattr() doesn't check for
> > NULL ->i_op.
> >
> > I've tried to reproduce this scenario with udelay() inserted before
> > S_ISDIR checks in proc_register() but got some panic while in X,
> > so no cookie for me. Maybe it's impossible, indeed.
> >
> > Anyway, this patch + moving ->proc_fops and ->proc_iops initialization
> > before gluing is OK with me.
>
> Like this?
Yes, exactly.
> --- a/fs/proc/generic.c~procfs-directory-entry-cleanup-fix
> +++ a/fs/proc/generic.c
> @@ -529,12 +529,6 @@ static int proc_register(struct proc_dir
> return -EAGAIN;
> dp->low_ino = i;
>
> - spin_lock(&proc_subdir_lock);
> - dp->next = dir->subdir;
> - dp->parent = dir;
> - dir->subdir = dp;
> - spin_unlock(&proc_subdir_lock);
> -
> if (S_ISDIR(dp->mode)) {
> if (dp->proc_iops == NULL) {
> dp->proc_fops = &proc_dir_operations;
> @@ -550,6 +544,13 @@ static int proc_register(struct proc_dir
> if (dp->proc_iops == NULL)
> dp->proc_iops = &proc_file_inode_operations;
> }
> +
> + spin_lock(&proc_subdir_lock);
> + dp->next = dir->subdir;
> + dp->parent = dir;
> + dir->subdir = dp;
> + spin_unlock(&proc_subdir_lock);
> +
> return 0;
> }
>
> _
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2007-05-22 20:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <200705210341.l4L3f1BH001692@shell0.pdx.osdl.net>
2007-05-22 17:43 ` + procfs-directory-entry-cleanup.patch added to -mm tree Alexey Dobriyan
2007-05-22 18:29 ` Andrew Morton
2007-05-22 20:16 ` Alexey Dobriyan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®