From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zhao Hongjiang Subject: Re: [PATCH] inotify: invalid mask should return a error number but not set it Date: Thu, 25 Apr 2013 14:48:44 +0800 Message-ID: <5178D1CC.7020707@huawei.com> References: <516F7B52.9000707@huawei.com> <5170715F.8010600@windriver.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Cc: , , , To: Paul Gortmaker Return-path: Received: from szxga02-in.huawei.com ([119.145.14.65]:56157 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750758Ab3DYGt2 (ORCPT ); Thu, 25 Apr 2013 02:49:28 -0400 In-Reply-To: <5170715F.8010600@windriver.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 >> 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 >> > > . >