Re: [PATCH] procfs fixes for inotify/dnotify

2007-07-19 Thread Alexey Dobriyan 
On Thu, Jul 19, 2007 at 03:03:55PM -0700, Andrew Morton wrote:
>   http://bugzilla.kernel.org/show_bug.cgi?id=8782

[fsnotify and /proc]

> > --- linux-2.6.15/fs/proc/base.c
> > +++ linux-2.6.15.4/fs/proc/base.c
> > @@ -1945,6 +1945,7 @@
> > if(proc_dentry != NULL) {
> > shrink_dcache_parent(proc_dentry);
> > dput(proc_dentry);
> > +   fsnotify_nameremove (proc_dentry, 1);
> > }
> 
> I wonder if this is racy - after the dput() we might no longer have a
> reference?

It is indeed racy. dput() => kill_it: => d_kill() => d_free() =>
__d_free() eventually. *boom*

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] procfs fixes for inotify/dnotify

2007-07-19 Thread Andrew Morton 
On Tue, 21 Feb 2006 22:16:23 +0100
Thomas Ogrisegg <[EMAIL PROTECTED]> wrote:

> Problem description:
> When a new process is created, the creation of the respective PID
> subdirectory of /proc is deferred until the /proc-directory is beeing
> read (by e.g. ps(1)). This causes file notification frameworks like
> dnotify and inotify to not work correctly with /proc.
> 
> This patch fixes the problem.
> 
> Signed-off-by: Thomas Ogrisegg <[EMAIL PROTECTED]>

Robin has raised this report:

http://bugzilla.kernel.org/show_bug.cgi?id=8782

pointing at your 15-month-old patch, which seemed to have got lost.

The patch rather doesn't work any more.  If you have time to update it then
please do so, and copy John and Eric on the patch?  

Thanks.

A few things I noticed:

> 
> diff -uNr -X linux-2.6.15/Documentation/dontdiff linux-2.6.15/kernel/fork.c 
> linux-2.6.15.4/kernel/fork.c
> --- linux-2.6.15/kernel/fork.c2006-01-03 04:21:10.0 +0100
> +++ linux-2.6.15.4/kernel/fork.c  2006-02-18 14:09:54.0 +0100
> @@ -43,6 +43,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -1141,6 +1142,7 @@
>   write_unlock_irq(_lock);
>   proc_fork_connector(p);
>   cpuset_fork(p);
> + proc_root_notify(pid);
>   retval = 0;
>  
>  fork_out:
> --- linux-2.6.15/include/linux/proc_fs.h  2006-01-03 04:21:10.0 
> +0100
> +++ linux-2.6.15.4/include/linux/proc_fs.h2006-02-18 13:55:52.0 
> +0100
> @@ -4,6 +4,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  
>  /*
> @@ -194,6 +196,13 @@
>   remove_proc_entry(name,proc_net);
>  }
>  
> +static inline void proc_root_notify (pid_t pid)
> +{
> + char buf[20];
> + snprintf (buf, sizeof (buf), "%d", pid);
> + fsnotify_mkdir (proc_mnt->mnt_root->d_inode, buf);
> +}

This shouldn't inlined.

Don't put spaces between the function name and the (

>  #else
>  
>  #define proc_root_driver NULL
> @@ -224,6 +233,8 @@
>   mode_t mode, struct proc_dir_entry *base, get_info_t *get_info)
>   { return NULL; }
>  
> +static inline void proc_root_notify (pid_t pid) {}

This can remain inlined in the header.

>  struct tty_driver;
>  static inline void proc_tty_register_driver(struct tty_driver *driver) {};
>  static inline void proc_tty_unregister_driver(struct tty_driver *driver) {};
> diff -uNr -X linux-2.6.15/Documentation/dontdiff linux-2.6.15/fs/proc/base.c 
> linux-2.6.15.4/fs/proc/base.c
> --- linux-2.6.15/fs/proc/base.c   2006-01-03 04:21:10.0 +0100
> +++ linux-2.6.15.4/fs/proc/base.c 2006-02-19 10:44:28.0 +0100
> @@ -1945,6 +1945,7 @@
>   if(proc_dentry != NULL) {
>   shrink_dcache_parent(proc_dentry);
>   dput(proc_dentry);
> + fsnotify_nameremove (proc_dentry, 1);
>   }

I wonder if this is racy - after the dput() we might no longer have a
reference?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] procfs fixes for inotify/dnotify

2007-07-19 Thread Andrew Morton 
On Tue, 21 Feb 2006 22:16:23 +0100
Thomas Ogrisegg [EMAIL PROTECTED] wrote:

 Problem description:
 When a new process is created, the creation of the respective PID
 subdirectory of /proc is deferred until the /proc-directory is beeing
 read (by e.g. ps(1)). This causes file notification frameworks like
 dnotify and inotify to not work correctly with /proc.
 
 This patch fixes the problem.
 
 Signed-off-by: Thomas Ogrisegg [EMAIL PROTECTED]

Robin has raised this report:

http://bugzilla.kernel.org/show_bug.cgi?id=8782

pointing at your 15-month-old patch, which seemed to have got lost.

The patch rather doesn't work any more.  If you have time to update it then
please do so, and copy John and Eric on the patch?  

Thanks.

A few things I noticed:

 
 diff -uNr -X linux-2.6.15/Documentation/dontdiff linux-2.6.15/kernel/fork.c 
 linux-2.6.15.4/kernel/fork.c
 --- linux-2.6.15/kernel/fork.c2006-01-03 04:21:10.0 +0100
 +++ linux-2.6.15.4/kernel/fork.c  2006-02-18 14:09:54.0 +0100
 @@ -43,6 +43,7 @@
  #include linux/rmap.h
  #include linux/acct.h
  #include linux/cn_proc.h
 +#include linux/proc_fs.h
  
  #include asm/pgtable.h
  #include asm/pgalloc.h
 @@ -1141,6 +1142,7 @@
   write_unlock_irq(tasklist_lock);
   proc_fork_connector(p);
   cpuset_fork(p);
 + proc_root_notify(pid);
   retval = 0;
  
  fork_out:
 --- linux-2.6.15/include/linux/proc_fs.h  2006-01-03 04:21:10.0 
 +0100
 +++ linux-2.6.15.4/include/linux/proc_fs.h2006-02-18 13:55:52.0 
 +0100
 @@ -4,6 +4,8 @@
  #include linux/config.h
  #include linux/slab.h
  #include linux/fs.h
 +#include linux/fsnotify.h
 +#include linux/mount.h
  #include asm/atomic.h
  
  /*
 @@ -194,6 +196,13 @@
   remove_proc_entry(name,proc_net);
  }
  
 +static inline void proc_root_notify (pid_t pid)
 +{
 + char buf[20];
 + snprintf (buf, sizeof (buf), %d, pid);
 + fsnotify_mkdir (proc_mnt-mnt_root-d_inode, buf);
 +}

This shouldn't inlined.

Don't put spaces between the function name and the (

  #else
  
  #define proc_root_driver NULL
 @@ -224,6 +233,8 @@
   mode_t mode, struct proc_dir_entry *base, get_info_t *get_info)
   { return NULL; }
  
 +static inline void proc_root_notify (pid_t pid) {}

This can remain inlined in the header.

  struct tty_driver;
  static inline void proc_tty_register_driver(struct tty_driver *driver) {};
  static inline void proc_tty_unregister_driver(struct tty_driver *driver) {};
 diff -uNr -X linux-2.6.15/Documentation/dontdiff linux-2.6.15/fs/proc/base.c 
 linux-2.6.15.4/fs/proc/base.c
 --- linux-2.6.15/fs/proc/base.c   2006-01-03 04:21:10.0 +0100
 +++ linux-2.6.15.4/fs/proc/base.c 2006-02-19 10:44:28.0 +0100
 @@ -1945,6 +1945,7 @@
   if(proc_dentry != NULL) {
   shrink_dcache_parent(proc_dentry);
   dput(proc_dentry);
 + fsnotify_nameremove (proc_dentry, 1);
   }

I wonder if this is racy - after the dput() we might no longer have a
reference?

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] procfs fixes for inotify/dnotify

2007-07-19 Thread Alexey Dobriyan 
On Thu, Jul 19, 2007 at 03:03:55PM -0700, Andrew Morton wrote:
   http://bugzilla.kernel.org/show_bug.cgi?id=8782

[fsnotify and /proc]

  --- linux-2.6.15/fs/proc/base.c
  +++ linux-2.6.15.4/fs/proc/base.c
  @@ -1945,6 +1945,7 @@
  if(proc_dentry != NULL) {
  shrink_dcache_parent(proc_dentry);
  dput(proc_dentry);
  +   fsnotify_nameremove (proc_dentry, 1);
  }
 
 I wonder if this is racy - after the dput() we might no longer have a
 reference?

It is indeed racy. dput() = kill_it: = d_kill() = d_free() =
__d_free() eventually. *boom*

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/