lkml.org 
[lkml]   [2015]   [Sep]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch()
From
2015-09-10 23:49 GMT+03:00 Andrew Morton <akpm@linux-foundation.org>:
> On Wed, 9 Sep 2015 16:32:37 -0700 Dave Hansen <dave@sr71.net> wrote:
>
>> On 09/09/2015 04:16 PM, Eric Paris wrote:
>> > Looks fine to me. And usually akpm picks them up these days.
>>
>> Is that an Acked-by? :)
>
> I grabbed it for 4.3.

This patch breaks CRIU. When we are dumping an inotify file
descriptors, we read event masks from /proc/self/fdinfo.

[root@fc22-vm criu]# cat /proc/1065/fdinfo/3
pos: 0
flags: 04000
mnt_id: 11
inotify wd:1 ino:101d5c sdev:fc00002 mask:800a708 ignored_mask:0
fhandle-bytes:8 fhandle-type:1 f_handle:5c1d10003996cda2

Here the mask contains the 0x8000000 (FS_EVENT_ON_CHILD) flag, which
is set for all inotify and dnotify watchers. On restore, we call
inotify_add_watch() with this mask, and it fails with this patch:

1375 inotify_add_watch(3, "/proc/self/fd/5",
IN_CLOSE_WRITE|IN_CREATE|IN_DELETE|IN_DELETE_SELF|IN_UNMOUNT|IN_IGNORED|0x8000000)
= -1 EINVAL (Invalid argument)

I am not sure that we have to save backward compatibility here. Maybe
we should not show FS_EVENT_ON_CHILD in fdinfo.

>
> I removed your cc:stable. I don't see anything in here which warrants
> a backport. If there *is* a reason for backporting then your
> changelogological skills are sorely wanting!
>
>
> The changelog is pretty sucky really. What are the reasons for this
> change, apart from "do what the comment said"? What's the benefit?
>
> And the code comment sucks. "don't allow invalid bits": well duh. And
> "we don't want flags set" is also useless: it doesn't explain *why* we
> don't want those flags set.
>
> And given that there is potential to break existing userspace, we need
> some decent reasons for making this change.
>
>
>
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
> Subject: inotify: actually check for invalid bits in sys_inotify_add_watch()
>
> The comment here says that it is checking for invalid bits. But, the mask
> is *actually* checking to ensure that _any_ valid bit is set, which is
> quite different.
>
> Add the actual check which was intended. Retain the existing check
> because it actually does something useful: ensure that some inotify bits
> are being added to the watch. Plus, this is existing behavior which would
> be nice to preserve.
>
> I did a quick sniff test that inotify functions and that my
> 'inotify-tools' package passes 'make check'.
>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: John McCutchan <john@johnmccutchan.com>
> Cc: Robert Love <rlove@rlove.org>
> Cc: Eric Paris <eparis@parisplace.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
> fs/notify/inotify/inotify_user.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff -puN fs/notify/inotify/inotify_user.c~inotify-actually-check-for-invalid-bits-in-sys_inotify_add_watch fs/notify/inotify/inotify_user.c
> --- a/fs/notify/inotify/inotify_user.c~inotify-actually-check-for-invalid-bits-in-sys_inotify_add_watch
> +++ a/fs/notify/inotify/inotify_user.c
> @@ -707,6 +707,9 @@ SYSCALL_DEFINE3(inotify_add_watch, int,
> unsigned flags = 0;
>
> /* don't allow invalid bits: we don't want flags set */
> + if (unlikely(mask & ~ALL_INOTIFY_BITS))
> + return -EINVAL;
> + /* require at least one valid bit set in the mask */
> if (unlikely(!(mask & ALL_INOTIFY_BITS)))
> return -EINVAL;
>
> _
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/


\
 
 \ /
  Last update: 2015-09-21 13:41    [W:0.051 / U:0.616 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site