Re: [PATCH] inotify: fix one-shot support

Re: [PATCH] inotify: fix one-shot support

[Ingo Oeser]

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

Hi Robert,
hi John,

just saw this commit.

On Wednesday 08 February 2006 02:05, you wrote:
> tree 5b5af4e03e627b66a9f37d25dd370a145ec72438
> parent 8e08b756869eeb08ace17ad64c2a8cb97b18e856
> author Robert Love <rml@novell.com> Wed, 08 Feb 2006 04:58:45 -0800
> committer Linus Torvalds <torvalds@g5.osdl.org> Wed, 08 Feb 2006 08:12:33 -0800
> 
> [PATCH] inotify: fix one-shot support
> 
> Fix one-shot support in inotify.  We currently drop the IN_ONESHOT flag
> during watch addition.  Fix is to not do that.
 
Yes, but now you can add a watch without any event attached. 
This would revert the original sense of the test.

> Signed-off-by: Robert Love <rml@novell.com>
> Cc: John McCutchan <ttb@tentacle.dhs.org>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> 
>  fs/inotify.c |    2 +-
>  1 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/inotify.c b/fs/inotify.c
> index 878ccca..3041503 100644
> --- a/fs/inotify.c
> +++ b/fs/inotify.c
> @@ -967,7 +967,7 @@ asmlinkage long sys_inotify_add_watch(in
>  		mask_add = 1;
>  
>  	/* don't let user-space set invalid bits: we don't want flags set */
> -	mask &= IN_ALL_EVENTS;
> +	mask &= IN_ALL_EVENTS | IN_ONESHOT;
>  	if (unlikely(!mask)) {
>  		ret = -EINVAL;
>  		goto out;

See, now you can just pass IN_ONESHOT behavior flag without any
events to shoot at, which you couldn't do before. But this makes only
sense, if we would like to set a multi-shot mask to one-shot now.

Does this transition (multi shot to single shot)makes sense at all? 
Is it race-free to allow this?.

So my suggested fix instead of yours would be:

/* don't let user-space set invalid bits: we don't want flags set */
mask &= IN_ALL_EVENTS | IN_ONESHOT;
if (unlikely((mask & IN_ALL_EVENTS) == 0 && !mask_add)) {
	ret = -EINVAL;
	goto out;
}

Would you like a patch on top of the one submitted by you?


Regards

Ingo Oeser


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] inotify: fix one-shot support

[Robert Love]

On Wed, 2006-02-08 at 08:52 +0100, Ingo Oeser wrote:

> See, now you can just pass IN_ONESHOT behavior flag without any
> events to shoot at, which you couldn't do before. But this makes only
> sense, if we would like to set a multi-shot mask to one-shot now.

Ack!

> Does this transition (multi shot to single shot)makes sense at all? 
> Is it race-free to allow this?.

It should be okay.  This was my intention in the patch.

> So my suggested fix instead of yours would be:
> 
> /* don't let user-space set invalid bits: we don't want flags set */
> mask &= IN_ALL_EVENTS | IN_ONESHOT;
> if (unlikely((mask & IN_ALL_EVENTS) == 0 && !mask_add)) {
> 	ret = -EINVAL;
> 	goto out;
> }
> 
> Would you like a patch on top of the one submitted by you?

Yes, because my patch was already merged by Linus.

	Robert Love

Re: [PATCH] inotify: fix one-shot support

[Ingo Oeser]

Hi Robert,
hi John,

On Wednesday 08 February 2006 17:16, Robert Love wrote:
> On Wed, 2006-02-08 at 08:52 +0100, Ingo Oeser wrote:
> > See, now you can just pass IN_ONESHOT behavior flag without any
> > events to shoot at, which you couldn't do before. But this makes only
> > sense, if we would like to set a multi-shot mask to one-shot now.
> 
> Ack!

Ok, here comes the patch (against Linus' HEAD). 
It turned out, that we needed to change some more places to avoid having zero
events to watch for. If you are ok with it, I'll send it straight to Linus with
your Ack included and in proper patch format.


Regards

Ingo Oeser


diff --git a/fs/inotify.c b/fs/inotify.c
index 3041503..16ec5fb 100644
--- a/fs/inotify.c
+++ b/fs/inotify.c
@@ -935,6 +935,7 @@ asmlinkage long sys_inotify_add_watch(in
 	struct file *filp;
 	int ret, fput_needed;
 	int mask_add = 0;
+	int no_events = 0;
 	unsigned flags = 0;
 
 	filp = fget_light(fd, &fput_needed);
@@ -966,9 +967,13 @@ asmlinkage long sys_inotify_add_watch(in
 	if (mask & IN_MASK_ADD)
 		mask_add = 1;
 
-	/* don't let user-space set invalid bits: we don't want flags set */
+	/* Do we change and events or only multishot/singleshot? */
+	if (!(mask & IN_ALL_EVENTS))
+		no_events = 1;
+
+	/* Don't let user-space set invalid bits: we don't want flags set. */
 	mask &= IN_ALL_EVENTS | IN_ONESHOT;
-	if (unlikely(!mask)) {
+	if (unlikely(no_events && !mask_add)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -987,6 +992,15 @@ asmlinkage long sys_inotify_add_watch(in
 		goto out;
 	}
 
+	/* 
+	 * Want to change only multishot/singleshot, 
+	 * but has no existing watch? -> Illegal -ioe 
+	 */
+	if (unlikely(no_events)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	watch = create_watch(dev, mask, inode);
 	if (unlikely(IS_ERR(watch))) {
 		ret = PTR_ERR(watch);