* 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®