Re: [PATCH v2] inotify: invalid mask should return a error number but not set it

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 25 Apr 2013 14:51:15 +0800 Zhao Hongjiang <zhaohongjiang@huawei.com> 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.