[Fwd: [PATCH 2/3] inotify: check filename before dropping repeat events]

[Tim Gardner]

Scott James Remnant wrote:
> Please apply the following three patches to our tree early (they'll be
> in 2.6.31 anyway, but they're causing regressions now)
> 
> Scott
> 
> 
> ------------------------------------------------------------------------
> 
> Subject:
> [PATCH 2/3] inotify: check filename before dropping repeat events
> From:
> Eric Paris <eparis at redhat.com>
> Date:
> Mon, 13 Jul 2009 09:43:59 -0400
> To:
> linux-kernel at vger.kernel.org
> 
> To:
> linux-kernel at vger.kernel.org
> CC:
> scott at ubuntu.com, viro at ZenIV.linux.org.uk
> 
> 
> inotify drops events if the last event on the queue is the same as the
> current event.  But it does 2 things wrong.  First it is comparing old->inode
> with new->inode.  But after an event if put on the queue the ->inode is no
> longer allowed to be used.  It's possible between the last event and this new
> event the inode could be reused and we would falsely match the inode's memory
> address between two differing events.
> 
> The second problem is that when a file is removed fsnotify is passed the
> negative dentry for the removed object rather than the postive dentry from
> immediately before the removal.  This mean the (broken) inotify tail drop code
> was matching the NULL ->inode of differing events.
> 
> The fix is to check the file name which is stored with events when doing the
> tail drop instead of wrongly checking the address of the stored ->inode.
> 
> Reported-by: Scott James Remnant <scott at ubuntu.com>
> Signed-off-by: Eric Paris <eparis at redhat.com>
> ---
> 
>  fs/notify/notification.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/notify/notification.c b/fs/notify/notification.c
> index 959b73e..d1fbbea 100644
> --- a/fs/notify/notification.c
> +++ b/fs/notify/notification.c
> @@ -136,10 +136,14 @@ static bool event_compare(struct fsnotify_event *old, struct fsnotify_event *new
>  {
>  	if ((old->mask == new->mask) &&
>  	    (old->to_tell == new->to_tell) &&
> -	    (old->data_type == new->data_type)) {
> +	    (old->data_type == new->data_type) &&
> +	    (old->name_len == new->name_len)) {
>  		switch (old->data_type) {
>  		case (FSNOTIFY_EVENT_INODE):
> -			if (old->inode == new->inode)
> +			/* remember, after old was put on the wait_q we aren't
> +			 * allowed to look at the inode any more, only thing
> +			 * left to check was if the file_name is the same */
> +			if (old->name_len && !strcmp(old->file_name, new->file_name))
>  				return true;
>  			break;
>  		case (FSNOTIFY_EVENT_PATH):
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Subject:
> [PATCH 1/3] fsnotify: use def_bool in kconfig instead of letting the
> user choose
> From:
> Eric Paris <eparis at redhat.com>
> Date:
> Mon, 13 Jul 2009 09:43:53 -0400
> To:
> linux-kernel at vger.kernel.org
> 
> To:
> linux-kernel at vger.kernel.org
> CC:
> scott at ubuntu.com, viro at ZenIV.linux.org.uk
> 
> 
> fsnotify doens't give the user anything.  If someone chooses inotify or
> dnotify it should build fsnotify, if they don't select one it shouldn't be
> built.  This patch changes fsnotify to be a def_bool=n and makes everything
> else select it.  Also fixes the issue people complained about on lwn where
> gdm hung because they didn't have inotify and they didn't get the inotify
> build option.....
> 
> Signed-off-by: Eric Paris <eparis at redhat.com>
> ---
> 
>  fs/notify/Kconfig         |   12 +-----------
>  fs/notify/dnotify/Kconfig |    2 +-
>  fs/notify/inotify/Kconfig |    2 +-
>  3 files changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/notify/Kconfig b/fs/notify/Kconfig
> index 31dac7e..dffbb09 100644
> --- a/fs/notify/Kconfig
> +++ b/fs/notify/Kconfig
> @@ -1,15 +1,5 @@
>  config FSNOTIFY
> -	bool "Filesystem notification backend"
> -	default y
> -	---help---
> -	   fsnotify is a backend for filesystem notification.  fsnotify does
> -	   not provide any userspace interface but does provide the basis
> -	   needed for other notification schemes such as dnotify, inotify,
> -	   and fanotify.
> -
> -	   Say Y here to enable fsnotify suport.
> -
> -	   If unsure, say Y.
> +	def_bool n
>  
>  source "fs/notify/dnotify/Kconfig"
>  source "fs/notify/inotify/Kconfig"
> diff --git a/fs/notify/dnotify/Kconfig b/fs/notify/dnotify/Kconfig
> index 904ff8d..f9c1ca1 100644
> --- a/fs/notify/dnotify/Kconfig
> +++ b/fs/notify/dnotify/Kconfig
> @@ -1,6 +1,6 @@
>  config DNOTIFY
>  	bool "Dnotify support"
> -	depends on FSNOTIFY
> +	select FSNOTIFY
>  	default y
>  	help
>  	  Dnotify is a directory-based per-fd file change notification system
> diff --git a/fs/notify/inotify/Kconfig b/fs/notify/inotify/Kconfig
> index 5356884..3e56dbf 100644
> --- a/fs/notify/inotify/Kconfig
> +++ b/fs/notify/inotify/Kconfig
> @@ -15,7 +15,7 @@ config INOTIFY
>  
>  config INOTIFY_USER
>  	bool "Inotify support for userspace"
> -	depends on FSNOTIFY
> +	select FSNOTIFY
>  	default y
>  	---help---
>  	  Say Y here to enable inotify support for userspace, including the
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Subject:
> [PATCH 3/3] fsnotify: fix inotify tail drop check with path entries
> From:
> Eric Paris <eparis at redhat.com>
> Date:
> Mon, 13 Jul 2009 09:44:04 -0400
> To:
> linux-kernel at vger.kernel.org
> 
> To:
> linux-kernel at vger.kernel.org
> CC:
> scott at ubuntu.com, viro at ZenIV.linux.org.uk
> 
> 
> fsnotify drops new events when they are the same as the tail event on the
> queue to be sent to userspace.  The problem is that if the event comes with
> a path we forget to break out of the switch statement and fall into the
> code path which matches on events that do not have any type of file backed
> information (things like IN_UNMOUNT and IN_Q_OVERFLOW).  The problem is
> that this code thinks all such events should be dropped.  Fix is to add a
> break.
> 
> Signed-off-by: Eric Paris <eparis at redhat.com>
> ---
> 
>  fs/notify/notification.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/notify/notification.c b/fs/notify/notification.c
> index d1fbbea..99d840b 100644
> --- a/fs/notify/notification.c
> +++ b/fs/notify/notification.c
> @@ -150,6 +150,7 @@ static bool event_compare(struct fsnotify_event *old, struct fsnotify_event *new
>  			if ((old->path.mnt == new->path.mnt) &&
>  			    (old->path.dentry == new->path.dentry))
>  				return true;
> +			break;
>  		case (FSNOTIFY_EVENT_NONE):
>  			return true;
>  		};
> 
> 

ACK - I'll apply if they don't make it into -rc3 (which should be the
next upload)

rtg
-- 
Tim Gardner tim.gardner at canonical.com