On Thu, 25 Apr 2013 14:51:15 +0800 Zhao Hongjiang <zhaohongjiang@xxxxxxxxxx> wrote:
> When we run the crackerjack testsuit, inotify_add_watch test is stalled cause the
> invalid mask 0, the task is waiting for the event but it never come. This should
> return -EINVAL and it do is before the commit 676a0675cf9200 ("inotify: remove
> broken mask checks causing unmount to be EINVAL"). The commit remove the invalid
> mask check simply, but the invalid mask check is needed indeed.
>
> Check the mask wether in the ALL_INOTIFY_BITS before the inotify_arg_to_mask call,
> if is not, just return -EINVAL.
>
> Because IN_UNMOUNT is in ALL_INOTIFY_BITS, so this change will not trigger the
> problem that above commit fixed.
>
> ...
>
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -572,7 +572,6 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
> int add = (arg & IN_MASK_ADD);
> int ret;
>
> - /* don't allow invalid bits: we don't want flags set */
> mask = inotify_arg_to_mask(arg);
>
> fsn_mark = fsnotify_find_inode_mark(group, inode);
> @@ -623,7 +622,6 @@ static int inotify_new_watch(struct fsnotify_group *group,
> struct idr *idr = &group->inotify_data.idr;
> spinlock_t *idr_lock = &group->inotify_data.idr_lock;
>
> - /* don't allow invalid bits: we don't want flags set */
> mask = inotify_arg_to_mask(arg);
>
> tmp_i_mark = kmem_cache_alloc(inotify_inode_mark_cachep, GFP_KERNEL);
> @@ -751,6 +749,10 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
> int ret;
> unsigned flags = 0;
>
> + /* don't allow invalid bits: we don't want flags set */
> + if (unlikely(!(arg & ALL_INOTIFY_BITS)))
> + return -EINVAL;
> +
> f = fdget(fd);
> if (unlikely(!f.file))
> return -EBADF;
Doesn't compile and clearly wasn't runtime tested.
--- a/fs/notify/inotify/inotify_user.c~inotify-invalid-mask-should-return-a-error-number-but-not-set-it-fix
+++ a/fs/notify/inotify/inotify_user.c
@@ -750,7 +750,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int,
unsigned flags = 0;
/* don't allow invalid bits: we don't want flags set */
- if (unlikely(!(arg & ALL_INOTIFY_BITS)))
+ if (unlikely(!(mask & ALL_INOTIFY_BITS)))
return -EINVAL;
f = fdget(fd);
_
Please confirm that this fixed patch has been runtime tested.
btw, calling a function argument "arg" is plain dumb. That's not a
name of anything. Someone please go through the file and rename this
to something meaningful. "mask", at a minimum.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
