v2.6.31-rc6 inotify not reporting deleted files

v2.6.31-rc6 inotify not reporting deleted files

[Eric W. Biederman]

I don't have a small test case yet, but I have an application that
uses inotify and watches a file and takes action when that file has been
deleted.  Running that application on 2.6.31-rc6 the application is
no longer seeing the file being deleted.

With luck a word to the wise is all that is required for you to reproduce
this.  Otherwise I will spend some time tomorrow making a small reproducer.

Eric

Re: v2.6.31-rc6 inotify not reporting deleted files

[Eric Paris]

On Wed, 2009-08-26 at 02:06 -0700, Eric W. Biederman wrote:
> I don't have a small test case yet, but I have an application that
> uses inotify and watches a file and takes action when that file has been
> deleted.  Running that application on 2.6.31-rc6 the application is
> no longer seeing the file being deleted.
> 
> With luck a word to the wise is all that is required for you to reproduce
> this.  Otherwise I will spend some time tomorrow making a small reproducer.

I'm probably going to need a little more to go on.  The most simple
tests seem to work for me...

kernel-2.6.31-0.167.rc6.git6.fc12.x86_64

Watch /tmp/ while deleting /tmp/tmp:
$ touch /tmp/tmp
$ inotifywait -m /tmp
Setting up watches.  
Watches established.
/tmp/ DELETE tmp
^C

Watch /tmp/tmp while deleting /tmp/tmp
$ touch /tmp/tmp
$ inotifywait -m /tmp/tmp
Setting up watches.  
Watches established.
/tmp/tmp ATTRIB 
/tmp/tmp DELETE_SELF 
/tmp/tmp IGNORED

Delete 2 files while inotifywait is asleep
$touch /tmp/tmp1 /tmp/tmp2
$inotifywait -m /tmp
Setting up watches.  
Watches established.
^Z
[1]+  Stopped                 inotifywait -m /tmp
$ rm -f /tmp/tmp1
$ rm -f /tmp/tmp2
$ fg
inotifywait -m /tmp
/tmp/ DELETE tmp1
/tmp/ DELETE tmp2
^C

Delete 2 directories while inotifywait is asleep
$ mkdir /tmp/tmp1 /tmp/tmp2
$ inotifywait -m /tmp
Setting up watches.  
Watches established.
^Z
[1]+  Stopped                 inotifywait -m /tmp
$ rmdir /tmp/tmp1
$ rmdir /tmp/tmp2
$ fg
inotifywait -m /tmp
/tmp/ DELETE,ISDIR tmp1
/tmp/ DELETE,ISDIR tmp2

Re: v2.6.31-rc6 inotify not reporting deleted files

[Eric Paris]

On Wed, 2009-08-26 at 02:06 -0700, Eric W. Biederman wrote:
> I don't have a small test case yet, but I have an application that
> uses inotify and watches a file and takes action when that file has been
> deleted.  Running that application on 2.6.31-rc6 the application is
> no longer seeing the file being deleted.

Are you dealing with hard linked files?

-Eric

Re: v2.6.31-rc6 inotify not reporting deleted files

[Eric W. Biederman]

Eric Paris <eparis@redhat.com> writes:

> On Wed, 2009-08-26 at 02:06 -0700, Eric W. Biederman wrote:
>> I don't have a small test case yet, but I have an application that
>> uses inotify and watches a file and takes action when that file has been
>> deleted.  Running that application on 2.6.31-rc6 the application is
>> no longer seeing the file being deleted.
>
> Are you dealing with hard linked files?

I shouldn't be.  When I skimmed through everything last night it
looked like add a watch for create/delete/modify.  Wait.

File created (see it) file deleted (miss it).

I will go verify I can cook up a reproducer, on my box where it
has been failing consistently.

Eric

[PATCH] inotify: Ensure we alwasy write the terminating NULL.

[Eric W. Biederman]

Before the rewrite copy_event_to_user always wrote a terqminating '\0'
byte to user space after the filename.  Since the rewrite that
terminating byte was skipped if your filename is exactly a multiple of
event_size.  Ouch!

So add one byte to name_size before we round up and use clear_user to
set userspace to zero like /dev/zero does instead of copying the
strange nul_inotify_event.  I can't quite convince myself len_to_zero
will never exceed 16 and even if it doesn't clear_user should be more
efficient and a more accurate reflection of what the code is trying to
do.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 fs/notify/inotify/inotify_user.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index f30d9bb..90ae8ad 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -47,9 +47,6 @@
 
 static struct vfsmount *inotify_mnt __read_mostly;
 
-/* this just sits here and wastes global memory.  used to just pad userspace messages with zeros */
-static struct inotify_event nul_inotify_event;
-
 /* these are configurable via /proc/sys/fs/inotify/ */
 static int inotify_max_user_instances __read_mostly;
 static int inotify_max_queued_events __read_mostly;
@@ -199,8 +196,10 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 		inotify_free_event_priv(fsn_priv);
 	}
 
-	/* round up event->name_len so it is a multiple of event_size */
-	name_len = roundup(event->name_len, event_size);
+	/* round up event->name_len so it is a multiple of event_size
+	 * plus an extra byte for the terminating '\0'.
+	 */
+	name_len = roundup(event->name_len + 1, event_size);
 	inotify_event.len = name_len;
 
 	inotify_event.mask = inotify_mask_to_arg(event->mask);
@@ -224,8 +223,8 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 			return -EFAULT;
 		buf += event->name_len;
 
-		/* fill userspace with 0's from nul_inotify_event */
-		if (copy_to_user(buf, &nul_inotify_event, len_to_zero))
+		/* fill userspace with 0's */
+		if (clear_user(buf, len_to_zero))
 			return -EFAULT;
 		buf += len_to_zero;
 		event_size += name_len;
-- 
1.6.2.5

Re: [PATCH] inotify: Ensure we alwasy write the terminating NULL.

[Eric Paris]

On Thu, 2009-08-27 at 03:20 -0700, Eric W. Biederman wrote:
> Before the rewrite copy_event_to_user always wrote a terqminating '\0'
> byte to user space after the filename.  Since the rewrite that
> terminating byte was skipped if your filename is exactly a multiple of
> event_size.  Ouch!

All of my testing code was carefully using the len field rather than
counting on it being nul terminated.  You are clearly right, it's
supposed to be nul terminated.

> So add one byte to name_size before we round up and use clear_user to
> set userspace to zero like /dev/zero does instead of copying the
> strange nul_inotify_event.

I already did this in linux-next but haven't passed it to Linus since
although stupid it's right and works.  I guess I'll pass it along rolled
up in your patch now.

>   I can't quite convince myself len_to_zero
> will never exceed 16 and even if it doesn't clear_user should be more
> efficient and a more accurate reflection of what the code is trying to
> do.

len_to_zero has to be <= sizeof(struct inotify_event) since it's defined
as:
        size_t event_size = sizeof(struct inotify_event);
        name_len = roundup(event->name_len, event_size);
        unsigned int len_to_zero = name_len - event->name_len;

So name_len can never be more than 'event_size' larger than
event->name_len.  Then len_to_zero could never be > 16.

Adding to my tree now.

-Eric

> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> ---
>  fs/notify/inotify/inotify_user.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index f30d9bb..90ae8ad 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -47,9 +47,6 @@
>  
>  static struct vfsmount *inotify_mnt __read_mostly;
>  
> -/* this just sits here and wastes global memory.  used to just pad userspace messages with zeros */
> -static struct inotify_event nul_inotify_event;
> -
>  /* these are configurable via /proc/sys/fs/inotify/ */
>  static int inotify_max_user_instances __read_mostly;
>  static int inotify_max_queued_events __read_mostly;
> @@ -199,8 +196,10 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>  		inotify_free_event_priv(fsn_priv);
>  	}
>  
> -	/* round up event->name_len so it is a multiple of event_size */
> -	name_len = roundup(event->name_len, event_size);
> +	/* round up event->name_len so it is a multiple of event_size
> +	 * plus an extra byte for the terminating '\0'.
> +	 */
> +	name_len = roundup(event->name_len + 1, event_size);
>  	inotify_event.len = name_len;
>  
>  	inotify_event.mask = inotify_mask_to_arg(event->mask);
> @@ -224,8 +223,8 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>  			return -EFAULT;
>  		buf += event->name_len;
>  
> -		/* fill userspace with 0's from nul_inotify_event */
> -		if (copy_to_user(buf, &nul_inotify_event, len_to_zero))
> +		/* fill userspace with 0's */
> +		if (clear_user(buf, len_to_zero))
>  			return -EFAULT;
>  		buf += len_to_zero;
>  		event_size += name_len;

Re: [PATCH] inotify: Ensure we alwasy write the terminating NULL.

[Brian Rogers]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

Eric W. Biederman wrote:
> Before the rewrite copy_event_to_user always wrote a terqminating '\0'
> byte to user space after the filename.  Since the rewrite that
> terminating byte was skipped if your filename is exactly a multiple of
> event_size.  Ouch!
>
> So add one byte to name_size before we round up and use clear_user to
> set userspace to zero like /dev/zero does instead of copying the
> strange nul_inotify_event.  I can't quite convince myself len_to_zero
> will never exceed 16 and even if it doesn't clear_user should be more
> efficient and a more accurate reflection of what the code is trying to
> do.
>
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
>   

I found that this change prevents my Ubuntu Karmic system from booting. 
It just idles forever very early in the process. Probably a boot script 
is waiting for an event that it doesn't receive properly.

> -	name_len = roundup(event->name_len, event_size);
> +	name_len = roundup(event->name_len + 1, event_size);
>   

This means the test later on will now always evaluate to true:
> if (name_len) {

And in cases where that test previously would have failed, the code now 
outputs a block full of zeros. Assuming that's bad and the test was 
important, I coded the attached naive fix, which is working for me.


[-- Attachment #2: 0001-inotify-Fix-events-with-no-pathname.patch --]
[-- Type: text/x-patch, Size: 1320 bytes --]

>From 1be7a610013b47be1257d2b0296d872d6bed7416 Mon Sep 17 00:00:00 2001
From: Brian Rogers <brian@xyzw.org>
Date: Fri, 28 Aug 2009 04:46:51 -0700
Subject: [PATCH] inotify: Fix events with no pathname

When an event has no pathname, there's no need to pad it with a null byte and
therefore generate an inotify_event sized block of zeros. This fixes a
regression introduced by commit 0db501bd0610ee0c0aca84d927f90bcccd09e2bd where
my system wouldn't finish booting because some process was being confused by
this.

Signed-off-by: Brian Rogers <brian@xyzw.org>
---
 fs/notify/inotify/inotify_user.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 0e781bc..d94ce8b 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -199,7 +199,10 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	/* round up event->name_len so it is a multiple of event_size
 	 * plus an extra byte for the terminating '\0'.
 	 */
-	name_len = roundup(event->name_len + 1, event_size);
+	if (event->name_len > 0)
+		name_len = roundup(event->name_len + 1, event_size);
+	else
+		name_len = 0;
 	inotify_event.len = name_len;
 
 	inotify_event.mask = inotify_mask_to_arg(event->mask);
-- 
1.6.3.3

Re: [PATCH] inotify: Ensure we alwasy write the terminating NULL.

[Eric Paris]

On Fri, 2009-08-28 at 06:29 -0700, Brian Rogers wrote:
> Eric W. Biederman wrote:
> > Before the rewrite copy_event_to_user always wrote a terqminating '\0'
> > byte to user space after the filename.  Since the rewrite that
> > terminating byte was skipped if your filename is exactly a multiple of
> > event_size.  Ouch!
> >
> > So add one byte to name_size before we round up and use clear_user to
> > set userspace to zero like /dev/zero does instead of copying the
> > strange nul_inotify_event.  I can't quite convince myself len_to_zero
> > will never exceed 16 and even if it doesn't clear_user should be more
> > efficient and a more accurate reflection of what the code is trying to
> > do.
> >
> > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> >   
> 
> I found that this change prevents my Ubuntu Karmic system from booting. 
> It just idles forever very early in the process. Probably a boot script 
> is waiting for an event that it doesn't receive properly.
> 
> > -	name_len = roundup(event->name_len, event_size);
> > +	name_len = roundup(event->name_len + 1, event_size);
> >   
> 
> This means the test later on will now always evaluate to true:
> > if (name_len) {
> 
> And in cases where that test previously would have failed, the code now 
> outputs a block full of zeros. Assuming that's bad and the test was 
> important, I coded the attached naive fix, which is working for me.

Thanks, I ask Linus to pull a (slightly modified) version.  Don't know
if he'll get it before he runs out on vacation.  Here's hoping.

-Eric