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

[Jim Somerville <jim.somerville@windriver.com>]

This looks good to me, doing the check once right up at the syscall is 
the right place to do it as Paul G pointed out earlier.  Letting bad 
args flow into the bowels of the code leads to the obfuscation like what 
was in that original broken mask check that I removed.  The commit 
header English could use some cleanup though.

-Jim


On 13-04-25 02:51 AM, Zhao Hongjiang 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.
>
> v2: move the check at the begain of inotify_add_watch syscall.
>
> Signed-off-by: Zhao Hongjiang <zhaohongjiang@huawei.com>
> Cc: stable@vger.kernel.org
> ---
>   fs/notify/inotify/inotify_user.c |    6 ++++--
>   1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index e0f7c12..a8ce1b6 100644
> --- 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;
>