Skip to contentSkip to conversation

v2.6.31-rc6 inotify not reporting deleted files

PATCH In review Eric W. Biederman 3 participants 8 messages Aug 26, 2009 – Aug 28, 2009

Subsystems (3)
VFS Fsnotify: filesystem notification infrastructure Inotify
lore
Share & tools
lore

Eric W. Biederman

author
#0 lore raw

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

#1 lore raw
Quoted 7 lines · 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.

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
#2 lore raw
In reply to Eric W. Biederman
Quoted 4 lines · 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.

Are you dealing with hard linked files?

-Eric

author
#3 lore raw
Quoted 7 lines · Eric Paris
Quoted 4 lines · 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.

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

author
#4 lore raw

PATCH 1/1 inotify: Ensure we alwasy write the terminating NULL.

In reply to Eric Paris

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 <[email protected]>
---
 fs/notify/inotify/inotify_user.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
fs/notify/inotify/inotify_user.c +6 -7
index f30d9bb..90ae8ad 100644
@@ -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;
Signature · 1 line
1.6.2.5
#5 lore raw

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

Quoted 4 lines · 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!

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.

Quoted 3 lines

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.

Quoted 4 lines

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

Quoted 43 lines
Signed-off-by Eric W. Biederman <[email protected]>
---
 fs/notify/inotify/inotify_user.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
fs/notify/inotify/inotify_user.c +6 -7
index f30d9bb..90ae8ad 100644
@@ -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;
#6 lore raw

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

In reply to Eric W. Biederman
Quoted 5 lines · 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!

Quoted 9 lines

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 <[email protected]>

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.

Review

Quoted 3 lines
(quoted diff) +1 -1
-	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:

Quoted 1 line

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.

Quoted 1 line

From 1be7a610013b47be1257d2b0296d872d6bed7416 Mon Sep 17 00:00:00 2001

From: Brian Rogers <[email protected]>
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 <[email protected]>
---
 fs/notify/inotify/inotify_user.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
fs/notify/inotify/inotify_user.c +4 -1
index 0e781bc..d94ce8b 100644
@@ -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);
Signature · 1 line
1.6.3.3
#7 lore raw

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

Quoted 25 lines · Brian Rogers
Quoted 5 lines · 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!

Quoted 9 lines

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 <[email protected]>

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.

Review

Quoted 3 lines
(quoted diff) +1 -1
-	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:

Quoted 5 lines
Quoted 1 line

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