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

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

[Zhao Hongjiang]

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.

Signed-off-by: Zhao Hongjiang <zhaohongjiang@huawei.com>
Cc: stable@vger.kernel.org
---
 fs/notify/inotify/inotify_user.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index e0f7c12..b024bc1 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -573,6 +573,9 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
 	int ret;
 
 	/* don't allow invalid bits: we don't want flags set */
+	if (unlikely(!(arg & ALL_INOTIFY_BITS)))
+		return -EINVAL;
+
 	mask = inotify_arg_to_mask(arg);
 
 	fsn_mark = fsnotify_find_inode_mark(group, inode);
@@ -624,6 +627,9 @@ static int inotify_new_watch(struct fsnotify_group *group,
 	spinlock_t *idr_lock = &group->inotify_data.idr_lock;
 
 	/* don't allow invalid bits: we don't want flags set */
+	if (unlikely(!(arg & ALL_INOTIFY_BITS)))
+		return -EINVAL;
+
 	mask = inotify_arg_to_mask(arg);
 
 	tmp_i_mark = kmem_cache_alloc(inotify_inode_mark_cachep, GFP_KERNEL);
-- 1.7.1

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

[Paul Gortmaker]

On 13-04-18 12:49 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. 

If you want to filter out zero as EINVAL, then it should be done
right at the top of the syscall entry  and not duplicated twice
here in the guts of the code, I think.  I can send a suggested
patch if that description is not 100% self evident.

Paul.
--

> 
> Because IN_UNMOUNT is in ALL_INOTIFY_BITS, so this change will not trigger the 
> problem that above commit fixed.
> 
> Signed-off-by: Zhao Hongjiang <zhaohongjiang@huawei.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/notify/inotify/inotify_user.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index e0f7c12..b024bc1 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -573,6 +573,9 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
>  	int ret;
>  
>  	/* don't allow invalid bits: we don't want flags set */
> +	if (unlikely(!(arg & ALL_INOTIFY_BITS)))
> +		return -EINVAL;
> +
>  	mask = inotify_arg_to_mask(arg);
>  
>  	fsn_mark = fsnotify_find_inode_mark(group, inode);
> @@ -624,6 +627,9 @@ static int inotify_new_watch(struct fsnotify_group *group,
>  	spinlock_t *idr_lock = &group->inotify_data.idr_lock;
>  
>  	/* don't allow invalid bits: we don't want flags set */
> +	if (unlikely(!(arg & ALL_INOTIFY_BITS)))
> +		return -EINVAL;
> +
>  	mask = inotify_arg_to_mask(arg);
>  
>  	tmp_i_mark = kmem_cache_alloc(inotify_inode_mark_cachep, GFP_KERNEL);
> -- 1.7.1
> 

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

[Zhao Hongjiang]

On 2013/4/19 6:19, Paul Gortmaker wrote:
> On 13-04-18 12:49 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. 
> 
> If you want to filter out zero as EINVAL, then it should be done
> right at the top of the syscall entry  and not duplicated twice
> here in the guts of the code, I think.  I can send a suggested
> patch if that description is not 100% self evident.
> 
Not only filter out zero but also other invalid mask like 0x00001000 which are not in
ALL_INOTIFY_BITS. i just leave the check at the place before, but move to the top of
the syscall entry is more reasonable. I'll send the v2 shortly. Thanks.

Zhao Hongjiang.
> Paul.
> --
> 
>>
>> Because IN_UNMOUNT is in ALL_INOTIFY_BITS, so this change will not trigger the 
>> problem that above commit fixed.
>>
>> Signed-off-by: Zhao Hongjiang <zhaohongjiang@huawei.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  fs/notify/inotify/inotify_user.c |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
>> index e0f7c12..b024bc1 100644
>> --- a/fs/notify/inotify/inotify_user.c
>> +++ b/fs/notify/inotify/inotify_user.c
>> @@ -573,6 +573,9 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
>>  	int ret;
>>  
>>  	/* don't allow invalid bits: we don't want flags set */
>> +	if (unlikely(!(arg & ALL_INOTIFY_BITS)))
>> +		return -EINVAL;
>> +
>>  	mask = inotify_arg_to_mask(arg);
>>  
>>  	fsn_mark = fsnotify_find_inode_mark(group, inode);
>> @@ -624,6 +627,9 @@ static int inotify_new_watch(struct fsnotify_group *group,
>>  	spinlock_t *idr_lock = &group->inotify_data.idr_lock;
>>  
>>  	/* don't allow invalid bits: we don't want flags set */
>> +	if (unlikely(!(arg & ALL_INOTIFY_BITS)))
>> +		return -EINVAL;
>> +
>>  	mask = inotify_arg_to_mask(arg);
>>  
>>  	tmp_i_mark = kmem_cache_alloc(inotify_inode_mark_cachep, GFP_KERNEL);
>> -- 1.7.1
>>
> 
> .
>