From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jim Somerville Subject: Re: [PATCH v2] inotify: invalid mask should return a error number but not set it Date: Thu, 25 Apr 2013 10:45:47 -0400 Message-ID: <5179419B.4090406@windriver.com> References: <5178D263.6010409@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Cc: , Paul Gortmaker , , To: Zhao Hongjiang Return-path: Received: from mail1.windriver.com ([147.11.146.13]:41676 "EHLO mail1.windriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756916Ab3DYOoY (ORCPT ); Thu, 25 Apr 2013 10:44:24 -0400 In-Reply-To: <5178D263.6010409@huawei.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 > 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; >