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

Groups > linux.kernel > #1221763 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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

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

On 06/30/2015 10:36 AM, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> 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'.

Did anybody have any comments on this patch?  Who picks up inotify patches?

>  b/fs/notify/inotify/inotify_user.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff -puN fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid-bit fs/notify/inotify/inotify_user.c
> --- a/fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid-bit	2015-06-26 13:33:30.277219285 -0700
> +++ b/fs/notify/inotify/inotify_user.c	2015-06-26 13:35:19.026122033 -0700
> @@ -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/

[toc] | [next] | [standalone]

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

On Wed, Sep 9, 2015 at 5:59 PM, Dave Hansen <dave@sr71.net> wrote:
> On 06/30/2015 10:36 AM, Dave Hansen wrote:
>> From: Dave Hansen <dave.hansen@linux.intel.com>
>>
>> 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'.
>
> Did anybody have any comments on this patch?  Who picks up inotify patches?

Theoretically the people you have CC'd pick them up.  However it seems
for quite some time Andrew has been shepherding them along.

josh

>>  b/fs/notify/inotify/inotify_user.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff -puN fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid-bit fs/notify/inotify/inotify_user.c
>> --- a/fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid-bit  2015-06-26 13:33:30.277219285 -0700
>> +++ b/fs/notify/inotify/inotify_user.c        2015-06-26 13:35:19.026122033 -0700
>> @@ -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 stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/

[toc] | [prev] | [next] | [standalone]

#1221787

Looks fine to me. And usually akpm picks them up these days.

On Wed, 2015-09-09 at 14:59 -0700, Dave Hansen wrote:
> On 06/30/2015 10:36 AM, Dave Hansen wrote:
> > From: Dave Hansen <dave.hansen@linux.intel.com>
> > 
> > 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'.
> 
> Did anybody have any comments on this patch?  Who picks up inotify
> patches?
> 
> >  b/fs/notify/inotify/inotify_user.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff -puN fs/notify/inotify/inotify_user.c~inotify-EINVAL-on
> > -invalid-bit fs/notify/inotify/inotify_user.c
> > --- a/fs/notify/inotify/inotify_user.c~inotify-EINVAL-on-invalid
> > -bit	2015-06-26 13:33:30.277219285 -0700
> > +++ b/fs/notify/inotify/inotify_user.c	2015-06-26
> > 13:35:19.026122033 -0700
> > @@ -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/

[toc] | [prev] | [next] | [standalone]

#1221795

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? :)
--
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/

[toc] | [prev] | [next] | [standalone]

#1222388

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.

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/

[toc] | [prev] | [next] | [standalone]

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

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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web