Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1229225

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

Show all headers | View raw

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/
--
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/

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

Re: [PATCH] inotify: actually check for invalid bits in  sys_inotify_add_watch() Dave Hansen <dave@sr71.net> - 2015-09-10 00:10 +0200
  Re: [PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch() Josh Boyer <jwboyer@fedoraproject.org> - 2015-09-10 00:40 +0200
  Re: [PATCH] inotify: actually check for invalid bits in  sys_inotify_add_watch() Eric Paris <eparis@redhat.com> - 2015-09-10 01:20 +0200
    Re: [PATCH] inotify: actually check for invalid bits in  sys_inotify_add_watch() Dave Hansen <dave@sr71.net> - 2015-09-10 01:40 +0200
      Re: [PATCH] inotify: actually check for invalid bits in  sys_inotify_add_watch() Andrew Morton <akpm@linux-foundation.org> - 2015-09-10 22:50 +0200
        Re: [PATCH] inotify: actually check for invalid bits in sys_inotify_add_watch() Andrey Wagin <avagin@gmail.com> - 2015-09-21 13:30 +0200

csiph-web