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 stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html