Re: [PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch()

[Andrey Wagin]

2015-09-10 23:49 GMT+03:00 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>:
> On Wed, 9 Sep 2015 16:32:37 -0700 Dave Hansen <dave@xxxxxxxx> 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@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> Cc: John McCutchan <john@xxxxxxxxxxxxxxxxx>
> Cc: Robert Love <rlove@xxxxxxxxx>
> Cc: Eric Paris <eparis@xxxxxxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>
>  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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/